Skip to content

Commit ba042e4

Browse files
JoeMattclaude
andcommitted
ci: address Copilot round 3 review
Five real findings: 1. scripts/gen-version-h.sh: shebang said #!/usr/bin/env bash but the script is pure POSIX sh. Switched to /usr/bin/env sh so it runs in environments without bash. Updated the header comment to describe the actual call sites (Makefile parse-time $(shell), Android.mk, c89-lint, install-hooks pre-commit, msvc-check). 2. scripts/c89-lint.sh: now invokes the generator via \`sh\`, not \`bash\` -- so the C89 lint hook works on POSIX-only environments. 3. jni/Android.mk: ndk-build doesn't go through the project Makefile, so the parse-time version.h gen there didn't fire for local Android builds (only c-cpp.yml's CI step had it). Added the \`$(shell sh ... gen-version-h.sh ...)\` invocation to Android.mk. Also dropped the now-redundant -DGIT_VERSION wiring (handled by the generated header). 4. scripts/install-hooks.sh header comment: corrected dist/info/.info path reference to "anything under dist/info/ or Makefile". 5. Makefile \`make benchmark\` target: -ldl is Linux-specific and was hard-coded. macOS clang accepts it as a no-op but other linkers may not. Now conditional via \`$(if $(filter Linux,$(uname)),-ldl)\`. Local sanity: sh scripts/gen-version-h.sh runs clean, c89-lint passes, make benchmark on Apple Silicon produces expected ~240 FPS without spurious -ldl warnings. Co-Authored-By: Claude Opus 4.7 <[email protected]>
1 parent cb385d6 commit ba042e4

5 files changed

Lines changed: 21 additions & 11 deletions

File tree

Makefile

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -872,8 +872,12 @@ BENCH_BLITTER ?= fast
872872
benchmark: $(TARGET)
873873
@# Build the harness inline so this works whether or not TEST_EXPORTS=1
874874
@# was used for $(TARGET); the harness only uses retro_* exports.
875+
@# -ldl is Linux-specific; macOS/BSD provide dl* in libSystem/libc
876+
@# (and Apple's clang silently accepts -ldl as a no-op, but other
877+
@# linkers may not).
875878
$(CC) -O2 -Wall -std=c99 $(INCFLAGS) \
876-
-o test/tools/test_benchmark test/tools/test_benchmark.c -ldl
879+
-o test/tools/test_benchmark test/tools/test_benchmark.c \
880+
$(if $(filter Linux,$(shell uname -s)),-ldl)
877881
./test/tools/test_benchmark ./$(TARGET) $(BENCH_ROM) $(BENCH_FRAMES) \
878882
--warmup $(BENCH_WARMUP) --blitter $(BENCH_BLITTER)
879883

jni/Android.mk

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,10 @@ include $(CORE_DIR)/Makefile.common
66

77
COREFLAGS := -DINLINE="inline" -D__LIBRETRO__ $(INCFLAGS)
88

9-
GIT_VERSION := " $(shell git rev-parse --short HEAD || echo unknown)"
10-
ifneq ($(GIT_VERSION)," unknown")
11-
COREFLAGS += -DGIT_VERSION=\"$(GIT_VERSION)\"
12-
endif
9+
# libretro.c includes the generated src/core/version.h. Generate it
10+
# at parse time -- ndk-build doesn't go through the project Makefile,
11+
# so the parse-time $(shell ...) there doesn't fire for us.
12+
_VERSION_GEN := $(shell sh $(CORE_DIR)/scripts/gen-version-h.sh && echo ok)
1313

1414
include $(CLEAR_VARS)
1515
LOCAL_MODULE := retro

scripts/c89-lint.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ set -e
1212
# exists before we run -fsyntax-only -- this script is invoked from CI
1313
# and pre-commit hooks where `make` may not have run yet.
1414
ROOT=$(cd "$(dirname "$0")/.." && pwd)
15-
[ -f "$ROOT/src/core/version.h" ] || bash "$ROOT/scripts/gen-version-h.sh"
15+
[ -f "$ROOT/src/core/version.h" ] || sh "$ROOT/scripts/gen-version-h.sh"
1616

1717
CC="${CC:-gcc}"
1818
CFLAGS="-fsyntax-only -std=gnu89 -Werror=declaration-after-statement"

scripts/gen-version-h.sh

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,17 @@
1-
#!/usr/bin/env bash
1+
#!/usr/bin/env sh
22
# Generates src/core/version.h from CORE_BASE_VERSION pinned in
3-
# Makefile + the current short git rev. Called by the Makefile on
4-
# every build (FORCE rule) and by the c-cpp.yml msvc-check step which
5-
# invokes cl.exe directly without going through make.
3+
# Makefile + the current short git rev. Called by:
4+
# - The project Makefile at parse time via $(shell ...) (gated to
5+
# skip read-only goals like clean / print-* / lint).
6+
# - jni/Android.mk via $(shell ...) so ndk-build picks it up.
7+
# - scripts/c89-lint.sh, scripts/install-hooks.sh's pre-commit, and
8+
# the c-cpp.yml msvc-check step (which invokes cl.exe directly).
69
#
710
# Output is gitignored. Re-runs are content-aware: if the new
811
# contents match the existing file, the mtime is left alone so
912
# incremental builds don't needlessly rebuild libretro.o.
13+
#
14+
# POSIX sh compatible -- no bashisms.
1015

1116
set -e
1217

scripts/install-hooks.sh

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
#
44
# Currently installs a pre-commit that runs:
55
# - scripts/c89-lint.sh on staged .c files (catches MSVC C89 violations)
6-
# - scripts/check-info-version.sh if dist/info/.info or Makefile is staged
6+
# - scripts/check-info-version.sh if anything under dist/info/ or
7+
# Makefile is staged (verifies display_version stays in sync)
78
#
89
# Skip with `git commit --no-verify` if you really need to (e.g., a WIP
910
# squash); CI will catch it later anyway.

0 commit comments

Comments
 (0)