Skip to content

Enhance Makefile with improved portability and dependency handling #500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jcrussell wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Enhance Makefile with improved portability and dependency handling #500

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
798ce0f
Enhance Makefile with improved portability and dependency handling
jcrussell Dec 31, 2025
1f531d0
Address PR review feedback for Makefile portability
jcrussell Jan 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 68 additions & 16 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
UUID = "forge@jmmaranan.com"
UUID = forge@jmmaranan.com
Comment thread
jcrussell marked this conversation as resolved.
Outdated
INSTALL_PATH = $(HOME)/.local/share/gnome-shell/extensions/$(UUID)
MSGSRC = $(wildcard po/*.po)

.PHONY: all clean install schemas uninstall enable disable log debug patchcss
# 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)
Comment thread
jcrussell marked this conversation as resolved.
Outdated

.PHONY: all clean install schemas uninstall enable disable log debug patchcss check-deps

all: build install enable restart

Expand All @@ -19,12 +27,24 @@ schemas/gschemas.compiled: schemas/*.gschema.xml
patchcss:
# TODO: add the script to update css tag when delivering theme.js


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) \

Copilot AI Feb 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
| (grep -vE 'dependabot|noreply' || true) \
| (grep -v 'dependabot' | grep -v 'noreply' || true) \

Copilot uses AI. Check for mistakes.
| 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; \
Comment thread
jcrussell marked this conversation as resolved.
Outdated
}' >> lib/prefs/metadata.js
@echo "];" >> lib/prefs/metadata.js
Comment on lines 22 to +48

Copilot AI Jan 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

build: clean metadata.json schemas compilemsgs metadata
rm -rf temp
Expand All @@ -39,10 +59,12 @@ build: clean metadata.json schemas compilemsgs metadata
cp LICENSE temp
mkdir -p temp/locale
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 \
msgf=temp/locale/`basename $$msg .mo`; \
mkdir -p $$msgf; \
mkdir -p $$msgf/LC_MESSAGES; \
cp $$msg $$msgf/LC_MESSAGES/forge.mo; \
Comment thread
jcrussell marked this conversation as resolved.
Outdated
fi; \
done;
Comment on lines 62 to 70

Copilot AI Feb 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

./po/%.mo: ./po/%.po
Expand All @@ -54,22 +76,52 @@ debug:

potfile: ./po/forge.pot

# Conditional potfile generation based on xgettext availability
ifeq ($(HAS_XGETTEXT),yes)
./po/forge.pot: metadata ./prefs.js ./extension.js ./lib/**/*.js
mkdir -p po
xgettext --from-code=UTF-8 --output=po/forge.pot --package-name "Forge" ./prefs.js ./extension.js ./lib/**/*.js

else
./po/forge.pot:
@echo "WARNING: xgettext not found, skipping pot file generation"
@echo "Install gettext package for translation support"
@mkdir -p po
@touch ./po/forge.pot

Copilot AI Feb 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
@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

Copilot uses AI. Check for mistakes.
endif

# Conditional compilation of messages based on msgfmt availability
ifeq ($(HAS_MSGFMT),yes)
compilemsgs: potfile $(MSGSRC:.po=.mo)
for msg in $(MSGSRC); do \
msgmerge -U $$msg ./po/forge.pot; \
done;
Comment thread
jcrussell marked this conversation as resolved.
Comment on lines 97 to 99

Copilot AI Jan 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
else
compilemsgs:
@echo "WARNING: msgfmt not found, skipping translation compilation"
@echo "Install gettext package for translation support"
endif

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

Copilot AI Jan 8, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
rm -rf temp schemas/gschemas.compiled

check-deps:
@echo "Checking build dependencies..."
@command -v glib-compile-schemas &>/dev/null || (echo "ERROR: glib-compile-schemas not found. Install glib2-devel or libglib2.0-dev" && exit 1)
@command -v git &>/dev/null || (echo "ERROR: git not found" && exit 1)
@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!"
Comment thread
jcrussell marked this conversation as resolved.
Outdated

enable:
gnome-extensions enable "$(UUID)"
@if gnome-extensions list | grep -q "^$(UUID)$$"; then \
Comment thread
jcrussell marked this conversation as resolved.
Outdated
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
Comment on lines +120 to +126

Copilot AI Feb 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

disable:
gnome-extensions disable "$(UUID)" || echo "Nothing to disable"
Expand Down Expand Up @@ -115,7 +167,7 @@ test-nested: horizontal-line
WAYLAND_DISPLAY=wayland-forge \
dbus-run-session -- gnome-shell --nested --wayland --wayland-display=wayland-forge

# Usage:
# Usage:
# make test-open &
# make test-open CMD=gnome-text-editor
# make test-open CMD=gnome-terminal ARGS='--app-id app.x'
Expand Down