Enhance Makefile with improved portability and dependency handling #500

jcrussell · 2026-01-01T18:29:08Z

I wanted to try out forge but had issues with the Makefile -- my version of awk doesn't have -i inplace and I didn't have all the dependencies installed. I've been playing with Claude so I asked it to fix it -- this worked for me and all the changes are sane. I also had it simplify metadata.js so that it defines a static list.

Adds shell configuration for consistent behavior across distributions, implements conditional compilation based on available tools (xgettext, msgfmt), improves metadata generation, adds dependency checking, and enhances user feedback during the build process.

🤖 Generated with Claude Code

Adds shell configuration for consistent behavior across distributions, implements conditional compilation based on available tools (xgettext, msgfmt), improves metadata generation, adds dependency checking, and enhances user feedback during the build process. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

This pull request aims to enhance the Makefile with improved portability and dependency handling to address issues where certain tools (like awk -i inplace, xgettext, msgfmt) may not be available. The changes introduce conditional compilation, dependency checking, and improved user feedback during the build process.

Key changes include:

Added shell configuration and tool detection for conditional builds
Refactored metadata generation to avoid awk -i inplace
Implemented conditional compilation for translation tools (xgettext, msgfmt)
Added new check-deps target for dependency verification
Enhanced enable target with better user feedback

Comments suppressed due to low confidence (1)

Makefile:71

The ./po/%.mo rule at line 71 uses msgfmt to compile .po files to .mo files, but this rule will fail when msgfmt is not available. The conditional compilation logic for compilemsgs (lines 93-102) won't prevent this rule from being invoked if the prerequisite .mo files are needed. When HAS_MSGFMT is "no", the pattern rule should be disabled or modified to prevent build failures.

./po/%.mo: ./po/%.po
	msgfmt -c $< -o $@

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-08T01:27:12Z

Makefile

@@ -1,8 +1,16 @@
-UUID = "forge@jmmaranan.com"
+UUID = forge@jmmaranan.com


The UUID variable has lost its quotes around the value. This should be UUID = "forge@jmmaranan.com" to maintain consistency with Make best practices and prevent potential issues with special characters in variable expansion. Without quotes, the UUID could be misinterpreted if it contained certain special characters.

Suggested change

UUID = forge@jmmaranan.com

UUID = "forge@jmmaranan.com"

Copilot · 2026-01-08T01:27:12Z

Makefile

+	| awk '{ \
+		email = $$NF; \
+		if (email in seen) next; \
+		seen[email] = 1; \
+		name = ""; \
+		for (i = 2; i < NF; i++) { \
+			name = name (i == 2 ? "" : " ") $$i; \
+		} \
+		gsub(/"/, "\\\"", name); \
+		printf "  \"%s %s\",\n", name, email; \


The awk script extracts the email as $$NF (last field) but this assumes the email is always the last field in git shortlog output. However, git shortlog -sne produces output in the format "countname", where the email includes angle brackets. The current awk logic doesn't properly extract the name because it iterates from field 2 to NF-1, which would miss the actual name parsing. Additionally, the email variable will contain angle brackets that should be removed. Consider using a more robust parsing approach that properly handles the git shortlog format.

Suggested change

| awk '{ \

email = $$NF; \

if (email in seen) next; \

seen[email] = 1; \

name = ""; \

for (i = 2; i < NF; i++) { \

name = name (i == 2 ? "" : " ") $$i; \

} \

gsub(/"/, "\\\"", name); \

printf " \"%s %s\",\n", name, email; \

| awk -F'\t' '{ \

rest = $$2; \

if (!match(rest, /^(.*) <([^>]*)>$$/, m)) next; \

name = m[1]; \

email = m[2]; \

if (email in seen) next; \

seen[email] = 1; \

gsub(/"/, "\\\"", name); \

printf " \"%s <%s>\",\n", name, email; \

Copilot · 2026-01-08T01:27:12Z

Makefile

+	@command -v zip &>/dev/null || echo "WARNING: zip not found, 'make dist' will fail"
+	@command -v xgettext &>/dev/null || echo "WARNING: xgettext not found, translations will be skipped"
+	@command -v msgfmt &>/dev/null || echo "WARNING: msgfmt not found, translations will be skipped"
+	@echo "All required dependencies found!"


The check-deps target shows "All required dependencies found!" even when only warnings are issued for optional dependencies. This message is misleading because it suggests all dependencies are present when some may be missing. The message should distinguish between required dependencies being met versus all dependencies being present, or should not be displayed when warnings have been issued.

Suggested change

@echo "All required dependencies found!"

@echo "All required dependencies found. Optional tools may be missing; see any warnings above."

Copilot · 2026-01-08T01:27:13Z

Makefile

 clean:
-	rm -f lib/prefs/metadata.js
-	rm "$(UUID).zip" || echo "Nothing to delete"
+	rm -f lib/prefs/metadata.js "$(UUID).zip"


The clean target now attempts to remove "$(UUID).zip" without checking if it exists first. While the -f flag prevents errors if the file doesn't exist, combining multiple rm operations on one line means if the first file removal has any unexpected issues, the entire command could fail. The old version had explicit error handling with || echo "Nothing to delete". Consider whether silent failure with -f is the desired behavior or if explicit feedback is preferred.

Suggested change

rm -f lib/prefs/metadata.js "$(UUID).zip"

@if [ -e "lib/prefs/metadata.js" ]; then rm -f "lib/prefs/metadata.js"; else echo "lib/prefs/metadata.js: Nothing to delete"; fi

@if [ -e "$(UUID).zip" ]; then rm -f "$(UUID).zip"; else echo "$(UUID).zip: Nothing to delete"; fi

Copilot · 2026-01-08T01:27:13Z

Makefile

+
 enable:
-	gnome-extensions enable "$(UUID)"
+	@if gnome-extensions list | grep -q "^$(UUID)$$"; then \


The enable target uses grep -q "^$(UUID)$$" to check if the extension is in the list, but the UUID variable contains special regex characters (the @ symbol and dots). While @ is not a special regex character, the dots (.) will match any character rather than literal dots. This could potentially match unintended extension IDs. The UUID should be escaped for regex matching or use grep -F (fixed string matching) for exact matching.

Suggested change

@if gnome-extensions list | grep -q "^$(UUID)$$"; then \

@if gnome-extensions list | grep -Fqx "$(UUID)"; then \

Copilot · 2026-01-08T01:27:13Z

Makefile

 compilemsgs: potfile $(MSGSRC:.po=.mo)
 	for msg in $(MSGSRC); do \
 		msgmerge -U $$msg ./po/forge.pot; \
 	done;


The compilemsgs target uses msgmerge which is also part of the gettext package, but there's no check for its availability. When HAS_MSGFMT is "yes", the target will execute the for loop that calls msgmerge -U, but if msgmerge is not available (unlikely but possible in misconfigured systems), it will fail. Consider adding a check for msgmerge availability or documenting that msgmerge is assumed to be present when msgfmt is available.

Copilot · 2026-01-08T01:27:14Z

Makefile

+# Shell configuration - explicitly use bash for portability across distros
+SHELL := /bin/bash
+.SHELLFLAGS := -eo pipefail -c
+
+# Tool detection (using bash-specific &> redirect)
+HAS_XGETTEXT := $(shell command -v xgettext &>/dev/null && echo yes || echo no)
+HAS_MSGFMT := $(shell command -v msgfmt &>/dev/null && echo yes || echo no)


The shell configuration sets SHELL to /bin/bash and uses bash-specific features like &> redirection (line 10), but this reduces portability rather than enhancing it. The /bin/bash path may not exist on all systems (e.g., BSD systems, NixOS, or minimal containers where bash might be at /usr/bin/bash or elsewhere). While the comment claims this is "for portability across distros", hardcoding /bin/bash and using bash-specific syntax actually makes the Makefile less portable. Consider using POSIX-compliant syntax with /bin/sh, or use more portable redirection like >/dev/null 2>&1 instead of &>.

Suggested change

# Shell configuration - explicitly use bash for portability across distros

SHELL := /bin/bash

.SHELLFLAGS := -eo pipefail -c

# Tool detection (using bash-specific &> redirect)

HAS_XGETTEXT := $(shell command -v xgettext &>/dev/null && echo yes || echo no)

HAS_MSGFMT := $(shell command -v msgfmt &>/dev/null && echo yes || echo no)

# Shell configuration - use POSIX /bin/sh for better portability

SHELL := /bin/sh

.SHELLFLAGS := -ec

# Tool detection (using POSIX redirection)

HAS_XGETTEXT := $(shell command -v xgettext >/dev/null 2>&1 && echo yes || echo no)

HAS_MSGFMT := $(shell command -v msgfmt >/dev/null 2>&1 && echo yes || echo no)

Copilot · 2026-01-08T01:27:14Z

Makefile

 metadata:
-	echo "export const developers = Object.entries([" > lib/prefs/metadata.js
-	git shortlog -sne || echo "" >> lib/prefs/metadata.js
-	awk -i inplace '!/dependabot|noreply/' lib/prefs/metadata.js
-	sed -i 's/^[[:space:]]*[0-9]*[[:space:]]*\(.*\) <\(.*\)>/  {name:"\1", email:"\2"},/g' lib/prefs/metadata.js
-	echo "].reduce((acc, x) => ({ ...acc, [x.email]: acc[x.email] ?? x.name }), {})).map(([email, name]) => name + ' <' + email + '>')" >> lib/prefs/metadata.js
+	@echo "Generating developer metadata..."
+	@echo "export const developers = [" > lib/prefs/metadata.js
+	@git shortlog -sne --all \
+	| (grep -vE 'dependabot|noreply' || true) \
+	| awk '{ \
+		email = $$NF; \
+		if (email in seen) next; \
+		seen[email] = 1; \
+		name = ""; \
+		for (i = 2; i < NF; i++) { \
+			name = name (i == 2 ? "" : " ") $$i; \
+		} \
+		gsub(/"/, "\\\"", name); \
+		printf "  \"%s %s\",\n", name, email; \
+	}' >> lib/prefs/metadata.js
+	@echo "];" >> lib/prefs/metadata.js


The metadata generation has been simplified from generating a complex object with deduplication logic to a simple array of strings. However, looking at how developers is used in lib/prefs/settings.js (line 31), it's passed directly to Adw.AboutWindow which expects an array of strings in the format "Name ". The new format should work, but the old format appeared to deduplicate by email and handle multiple commits per developer. Verify that the new format produces the expected output and properly handles edge cases like developers with multiple email addresses or special characters in names.

Copilot · 2026-01-08T01:27:14Z

Makefile

+		if [ -f $$msg ]; then \
+			msgf=temp/locale/`basename $$msg .mo`; \
+			mkdir -p $$msgf; \
+			mkdir -p $$msgf/LC_MESSAGES; \
+			cp $$msg $$msgf/LC_MESSAGES/forge.mo; \


The for msg in $(MSGSRC:.po=.mo) loop interpolates file paths derived from MSGSRC into the shell script without quoting, so a .po/.mo file with a crafted name containing shell metacharacters (e.g., ; or backticks) can break out of the loop and inject arbitrary commands when this target is built. An attacker who can introduce such a translation file (for example via a malicious repository clone or translation workflow) can achieve code execution on the developer or CI machine running make. To mitigate this, avoid expanding untrusted filenames directly into shell syntax and ensure msg/msgf are always treated as opaque data (e.g., by properly quoting the variables in the loop body and when constructing paths).

Suggested change

if [ -f $$msg ]; then \

msgf=temp/locale/`basename $$msg .mo`; \

mkdir -p $$msgf; \

mkdir -p $$msgf/LC_MESSAGES; \

cp $$msg $$msgf/LC_MESSAGES/forge.mo; \

if [ -f "$$msg" ]; then \

msg_base="$$(basename "$$msg" .mo)"; \

msgf="temp/locale/$$msg_base"; \

mkdir -p "$$msgf"; \

mkdir -p "$$msgf/LC_MESSAGES"; \

cp "$$msg" "$$msgf/LC_MESSAGES/forge.mo"; \

Copilot · 2026-01-08T01:27:14Z

Makefile

 	for msg in $(MSGSRC); do \
 		msgmerge -U $$msg ./po/forge.pot; \
 	done;


The for msg in $(MSGSRC) loop inside compilemsgs similarly expands .po filenames directly into the shell without quoting, so a malicious .po file name containing shell metacharacters can terminate the list and inject arbitrary commands (e.g., ;curl ...|sh) when make compilemsgs or dependent targets are run. This creates a code execution vector for anyone building the project from a repository or translation source that can supply arbitrary file names under po/. To close this, treat msg as data rather than shell syntax by ensuring filenames are properly quoted/escaped in the loop and when passed to msgmerge.

jmmaranan requested a review from Copilot January 8, 2026 01:20

Copilot started reviewing on behalf of jmmaranan January 8, 2026 01:21 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enhance Makefile with improved portability and dependency handling #500

Enhance Makefile with improved portability and dependency handling #500

Uh oh!

jcrussell commented Jan 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		@@ -1,8 +1,16 @@
		UUID = "forge@jmmaranan.com"
		UUID = forge@jmmaranan.com

	@echo "All required dependencies found!"
	@echo "All required dependencies found. Optional tools may be missing; see any warnings above."

	rm -f lib/prefs/metadata.js "$(UUID).zip"
	@if [ -e "lib/prefs/metadata.js" ]; then rm -f "lib/prefs/metadata.js"; else echo "lib/prefs/metadata.js: Nothing to delete"; fi
	@if [ -e "$(UUID).zip" ]; then rm -f "$(UUID).zip"; else echo "$(UUID).zip: Nothing to delete"; fi

	@if gnome-extensions list \| grep -q "^$(UUID)$$"; then \
	@if gnome-extensions list \| grep -Fqx "$(UUID)"; then \

-		if [ -f $$msg ]; then \
-			msgf=temp/locale/`basename $$msg .mo`; \
-			mkdir -p $$msgf; \
-			mkdir -p $$msgf/LC_MESSAGES; \
-			cp $$msg $$msgf/LC_MESSAGES/forge.mo; \
+		if [ -f "$$msg" ]; then \
+			msg_base="$$(basename "$$msg" .mo)"; \
+			msgf="temp/locale/$$msg_base"; \
+			mkdir -p "$$msgf"; \
+			mkdir -p "$$msgf/LC_MESSAGES"; \
+			cp "$$msg" "$$msgf/LC_MESSAGES/forge.mo"; \

Enhance Makefile with improved portability and dependency handling #500

Are you sure you want to change the base?

Enhance Makefile with improved portability and dependency handling #500

Uh oh!

Conversation

jcrussell commented Jan 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant