Enhance Makefile with improved portability and dependency handling#500
Enhance Makefile with improved portability and dependency handling#500jcrussell wants to merge 2 commits intoforge-ext:mainfrom
Conversation
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>
There was a problem hiding this comment.
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-depstarget for dependency verification - Enhanced
enabletarget with better user feedback
Comments suppressed due to low confidence (1)
Makefile:71
- The
./po/%.morule at line 71 usesmsgfmtto compile .po files to .mo files, but this rule will fail whenmsgfmtis not available. The conditional compilation logic forcompilemsgs(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.
| clean: | ||
| rm -f lib/prefs/metadata.js | ||
| rm "$(UUID).zip" || echo "Nothing to delete" | ||
| rm -f lib/prefs/metadata.js "$(UUID).zip" |
There was a problem hiding this comment.
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.
| 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 |
| 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 |
There was a problem hiding this comment.
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.
| for msg in $(MSGSRC); do \ | ||
| msgmerge -U $$msg ./po/forge.pot; \ | ||
| done; |
There was a problem hiding this comment.
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.
- Restore quotes around UUID variable - Use POSIX /bin/sh instead of /bin/bash for better portability - Replace bash-specific &> redirects with POSIX >/dev/null 2>&1 - Fix awk script to use POSIX-compatible parsing (index/substr) - Quote shell variables in loops to prevent injection - Move msgfmt pattern rule inside HAS_MSGFMT conditional - Use grep -Fqx for fixed-string matching in enable target - Clarify check-deps message about optional dependencies Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @echo "Generating developer metadata..." | ||
| @echo "export const developers = [" > lib/prefs/metadata.js | ||
| @git shortlog -sne --all \ | ||
| | (grep -vE 'dependabot|noreply' || true) \ |
There was a problem hiding this comment.
The grep command uses '-vE' flag which is not strictly POSIX (POSIX grep doesn't require -E support for basic patterns like 'dependabot|noreply'). For better portability with the stated goal of using POSIX /bin/sh, consider using 'grep -v -E' or 'egrep -v' as separate flags, or use a POSIX-compatible pattern. However, most modern systems support this syntax, so this is a minor portability consideration.
| | (grep -vE 'dependabot|noreply' || true) \ | |
| | (grep -v 'dependabot' | grep -v 'noreply' || true) \ |
| @if gnome-extensions list | grep -Fqx "$(UUID)"; then \ | ||
| gnome-extensions enable "$(UUID)" && echo "Extension enabled successfully"; \ | ||
| else \ | ||
| echo "WARNING: Extension not detected by GNOME Shell yet"; \ | ||
| echo "On Wayland: Log out and log back in, then run 'make enable'"; \ | ||
| echo "On X11: Press Alt+F2, type 'r', press Enter, then run 'make enable'"; \ | ||
| fi |
There was a problem hiding this comment.
The enable target now provides helpful user feedback, which is good. However, the logic assumes that if the extension is in the list, it can be enabled. There could be a case where the extension is listed but enabling fails for other reasons. Consider checking the exit status of the enable command separately from the list check, or handling the case where 'gnome-extensions enable' fails even when the extension is listed.
| if (email_part in seen) next; \ | ||
| seen[email_part] = 1; \ | ||
| gsub(/"/, "\\\"", name); \ | ||
| printf " \"%s <%s>\",\n", name, email_part; \ |
There was a problem hiding this comment.
The awk script will generate a trailing comma after the last developer entry. JavaScript arrays are generally tolerant of trailing commas in modern environments, but this could potentially cause issues with older parsers. Consider using awk's END block or tracking whether it's the first entry to avoid the trailing comma, or verify that all target environments support trailing commas in arrays.
| for msg in $(MSGSRC:.po=.mo); do \ | ||
| 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"; \ | ||
| fi; \ | ||
| done; |
There was a problem hiding this comment.
The build target depends on 'compilemsgs', but when HAS_MSGFMT is 'no', the compilemsgs target only prints a warning and doesn't create any .mo files. This means the build will succeed even without translations, but the for loop at lines 62-70 may try to copy non-existent .mo files. While the if check at line 63 handles this gracefully, the build target's dependency on compilemsgs suggests translations are required. Consider whether the build should fail if required translation tools are missing, or if the current graceful degradation is intentional.
| .PHONY: all clean install schemas uninstall enable disable log debug patchcss | ||
| # Shell configuration - use POSIX /bin/sh for better portability | ||
| SHELL := /bin/sh | ||
| .SHELLFLAGS := -ec |
There was a problem hiding this comment.
Setting .SHELLFLAGS to '-ec' means that any command failure will cause the shell script to exit immediately, and undefined variables won't be caught. The '-e' flag is good for catching errors, but consider if '-u' should also be added to catch undefined variable usage, or if this would break existing recipes. This is a behavior change that could affect error handling throughout the Makefile.
| .SHELLFLAGS := -ec | |
| .SHELLFLAGS := -euc |
| @echo "WARNING: xgettext not found, skipping pot file generation" | ||
| @echo "Install gettext package for translation support" | ||
| @mkdir -p po | ||
| @touch ./po/forge.pot |
There was a problem hiding this comment.
When xgettext is not available, the target creates an empty forge.pot file with 'touch'. This could cause issues with msgmerge in the compilemsgs target (line 98) which expects a valid POT file format. If HAS_XGETTEXT is 'no' but HAS_MSGFMT is 'yes', msgmerge will fail when trying to merge with an empty POT file. Consider creating a minimal valid POT file header or ensuring compilemsgs also checks for xgettext availability.
| @touch ./po/forge.pot | |
| @printf '%s\n' \ | |
| '# SOME DESCRIPTIVE TITLE.' \ | |
| '# Copyright (C) YEAR THE PACKAGE'\''S COPYRIGHT HOLDER' \ | |
| '# This file is distributed under the same license as the Forge package.' \ | |
| '# FIRST AUTHOR <EMAIL@ADDRESS>, YEAR.' \ | |
| '#' \ | |
| 'msgid ""' \ | |
| 'msgstr ""' \ | |
| '"Project-Id-Version: Forge\\n"' \ | |
| '"MIME-Version: 1.0\\n"' \ | |
| '"Content-Type: text/plain; charset=UTF-8\\n"' \ | |
| '"Content-Transfer-Encoding: 8bit\\n"' \ | |
| > ./po/forge.pot |
1 participant
I wanted to try out forge but had issues with the Makefile -- my version of awk doesn't have
-i inplaceand 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 simplifymetadata.jsso 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