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 all commits
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
92 changes: 73 additions & 19 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,15 @@ UUID = "forge@jmmaranan.com"
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 - use POSIX /bin/sh for better portability
SHELL := /bin/sh
.SHELLFLAGS := -ec
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
.SHELLFLAGS := -ec
.SHELLFLAGS := -euc

Copilot uses AI. Check for mistakes.

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

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

all: build install enable restart

Expand All @@ -19,12 +27,25 @@ 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) \
Copy link

Copilot AI Feb 5, 2026

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 -F'\t' '{ \
rest = $$2; \
n = index(rest, " <"); \
if (n == 0) next; \
name = substr(rest, 1, n - 1); \
email_part = substr(rest, n + 2); \
gsub(/>$$/, "", email_part); \
if (email_part in seen) next; \
seen[email_part] = 1; \
gsub(/"/, "\\\"", name); \
printf " \"%s <%s>\",\n", name, email_part; \
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}' >> lib/prefs/metadata.js
@echo "];" >> lib/prefs/metadata.js

build: clean metadata.json schemas compilemsgs metadata
rm -rf temp
Expand All @@ -39,37 +60,70 @@ 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 \
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;
Comment on lines 62 to 70
Copy link

Copilot AI Feb 5, 2026

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
msgfmt -c $< -o $@

debug:
sed -i 's/export const production = true/export const production = false/' temp/lib/shared/settings.js
#sed -i 's|1.*-alpha|4999|' temp/metadata.json

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
Copy link

Copilot AI Feb 5, 2026

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)
./po/%.mo: ./po/%.po
msgfmt -c $< -o $@

compilemsgs: potfile $(MSGSRC:.po=.mo)
for msg in $(MSGSRC); do \
msgmerge -U $$msg ./po/forge.pot; \
msgmerge -U "$$msg" ./po/forge.pot; \
done;
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"
Copy link

Copilot AI Jan 8, 2026

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 2>&1 || (echo "ERROR: glib-compile-schemas not found. Install glib2-devel or libglib2.0-dev" && exit 1)
@command -v git >/dev/null 2>&1 || (echo "ERROR: git not found" && exit 1)
@command -v zip >/dev/null 2>&1 || echo "WARNING: zip not found, 'make dist' will fail"
@command -v xgettext >/dev/null 2>&1 || echo "WARNING: xgettext not found, translations will be skipped"
@command -v msgfmt >/dev/null 2>&1 || echo "WARNING: msgfmt not found, translations will be skipped"
@echo "All required dependencies found. Optional tools may be missing; see any warnings above."

enable:
gnome-extensions enable "$(UUID)"
@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
Comment on lines +120 to +126
Copy link

Copilot AI Feb 5, 2026

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 All @@ -90,7 +144,7 @@ dist: build
zip -qr "../${UUID}.zip" .

restart:
if bash -c 'xprop -root &> /dev/null'; then \
if xprop -root >/dev/null 2>&1; then \
killall -HUP gnome-shell; \
else \
gnome-session-quit --logout; \
Expand All @@ -115,7 +169,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