Setup GitHub action ci#269
Conversation
📝 WalkthroughWalkthroughThe changes introduce a GitHub Actions CI/CD workflow infrastructure using m4 macro templating. A new Makefile generates Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml.m4:
- Line 1: Add a CI sanity step in the .github/workflows/ci.yml.m4 workflow that
runs the generator and fails the job if the generated ci.yml is out of date:
invoke make -C .github/workflows to regenerate artifacts and then run git diff
--exit-code .github/workflows/ci.yml; place this step in the appropriate
validation job (e.g., the existing lint/validate job) so PRs fail when committed
.github/workflows/ci.yml does not match the output of the .m4 source.
- Around line 79-81: The git global configs (user.email, user.name,
core.longpaths, core.protectNTFS) are duplicated between the
clone-related-jdk21u step and the git merge step; remove the redundancy by
moving those git config --global lines into a single earlier step (e.g., leave
them only in clone-related-jdk21u) or extract them into a small reusable
composite/action or shell snippet and call it from both places; update the
.github/workflows/ci.yml.m4 to reference the consolidated step instead of
repeating the four git config --global settings.
- Around line 86-105: The workflow currently uses bare `set` in the "git merge
plan" and "git merge" steps which dumps the entire shell environment to public
CI logs; remove those bare `set` calls and instead print only the specific debug
variables you actually need (e.g., echo or printf the named vars like
CI_BRANCH_NAME, CI_ORGANIZATION, GITHUB_REF, or use `declare -p VAR_NAME` for
each) or enable controlled shell tracing with `set -x`/`set +x` around the exact
commands to trace; update the occurrences of `set` in those steps to one of
these targeted approaches so the full environment is no longer exposed.
- Around line 120-130: The workflow currently calls cftests_job(inference,
cftests-inference, 21) and 25 but omits any cftests_job(inference,
cftests-inference, 17); either add a brief inline comment next to the existing
cftests_job entries explaining why the inference test is intentionally excluded
for JDK 17 (e.g., "test-cftests-inference.sh requires JDK 21+") or, if the
omission was accidental, add the missing cftests_job(inference,
cftests-inference, 17) line so JDK 17 runs inference tests—place the comment
adjacent to the cftests_job lines referencing inference to make the reasoning
obvious.
In @.github/workflows/defs.m4:
- Around line 4-7: The macros canary_os and canary_test are defined but unused,
causing confusion; either remove these unused definitions or thread them into
the job templates so they actually control runner selection. Update the job
templates referenced (cftests_job, daikon_job, plume_lib_job) to use the macros
(e.g., replace hardcoded runs-on: ubuntu-latest with runs-on: canary_os-latest
or expand canary_test appropriately) or delete the define([canary_os],
[ubuntu])dnl and define([canary_test], [canary_os[]canary_version])dnl lines
from the m4 file to avoid stale metadata. Ensure any change keeps
canary_version/latest_version behavior consistent with the other macros.
- Line 15: The workflow uses mutable image tags "mdernst/cf-ubuntu-jdk$3:latest"
(and the two other identical occurrences) which should be pinned; update each
occurrence of the literal string "mdernst/cf-ubuntu-jdk$3:latest" to use an
immutable digest (e.g. "@sha256:...") or a stable version/date tag that matches
the image variants referenced by the Azure pipeline, ensuring all three
occurrences are replaced so CI runs are deterministic and not dependent on
:latest.
- Around line 47-63: The macro plume_lib_job currently ignores any arguments and
hardcodes the container image to mdernst/cf-ubuntu-jdk17:latest; update it to
either accept a JDK/version parameter (e.g., use $1 inside the plume_lib_job
definition to build the container image tag, mirroring cftests_job) so calls
like plume_lib_job(canary_version) actually control the JDK, or remove the
argument from the call site where plume_lib_job is invoked so it's clear no
parameter is used; alternatively, if JDK17 is intentional, add a brief comment
inside the plume_lib_job macro stating that plume-lib requires JDK17 to prevent
future confusion.
In @.github/workflows/Makefile:
- Around line 1-16: Add Makefile phony and delete-on-error declarations: declare
.PHONY for the targets all, default, and clean (so files named those won't
short-circuit recipes) and add a .DELETE_ON_ERROR: directive to ensure partial
outputs (e.g., created by the m4 -> $@ redirection in the pattern rule that
generates ci.yml) are removed if the recipe fails; place these declarations near
the top of the Makefile before the rules for all/default/% and clean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fdcd7f16-e435-493a-90c2-c6c72d1842f8
📒 Files selected for processing (4)
.github/workflows/Makefile.github/workflows/ci.yml.github/workflows/ci.yml.m4.github/workflows/defs.m4
| @@ -0,0 +1,142 @@ | |||
| # DO NOT EDIT ci.yml. Edit ci.yml.m4 and defs.m4 instead. | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a CI sanity check that the committed ci.yml matches make output.
Both ci.yml and ci.yml.m4 are committed, but nothing prevents a contributor from editing ci.yml directly (despite the DO NOT EDIT banner) or forgetting to regenerate after editing the .m4 source. A small workflow step that runs make -C .github/workflows && git diff --exit-code .github/workflows/ci.yml would fail PRs whose generated YAML doesn't match the source — making the banner enforceable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml.m4 at line 1, Add a CI sanity step in the
.github/workflows/ci.yml.m4 workflow that runs the generator and fails the job
if the generated ci.yml is out of date: invoke make -C .github/workflows to
regenerate artifacts and then run git diff --exit-code .github/workflows/ci.yml;
place this step in the appropriate validation job (e.g., the existing
lint/validate job) so PRs fail when committed .github/workflows/ci.yml does not
match the output of the .m4 source.
| git config --global user.email "you@example.com" | ||
| git config --global user.name "Your Name" | ||
| git config --global core.longpaths true |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
De-duplicate git config --global settings between the two steps.
user.email, user.name, core.longpaths, and core.protectNTFS are set in the clone-related-jdk21u step and then set again in the git merge step. Since --global config persists across steps within the same job (the runner home is shared), the second block is redundant. Either consolidate into one early step, or extract into a small reusable composite step / shell snippet.
Also applies to: 95-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml.m4 around lines 79 - 81, The git global configs
(user.email, user.name, core.longpaths, core.protectNTFS) are duplicated between
the clone-related-jdk21u step and the git merge step; remove the redundancy by
moving those git config --global lines into a single earlier step (e.g., leave
them only in clone-related-jdk21u) or extract them into a small reusable
composite/action or shell snippet and call it from both places; update the
.github/workflows/ci.yml.m4 to reference the consolidated step instead of
repeating the four git config --global settings.
| - name: git merge plan | ||
| run: | | ||
| cd ../jdk21u && git status | ||
| eval $(/tmp/$USER/plume-scripts/ci-info typetools) | ||
| set | ||
| echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}" | ||
| - name: git merge | ||
| run: | | ||
| set -ex | ||
| git config --global user.email "you@example.com" | ||
| git config --global user.name "Your Name" | ||
| git config --global pull.ff true | ||
| git config --global pull.rebase false | ||
| git config --global core.longpaths true | ||
| git config --global core.protectNTFS false | ||
| cd ../jdk21u && git status | ||
| eval $(/tmp/$USER/plume-scripts/ci-info typetools) | ||
| set | ||
| echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}" | ||
| cd ../jdk21u && git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME} || (git --version && git show | head -100 && git status && git diff | head -1000 && echo "Merge failed; see 'Pull request merge conflicts' at https://github.com/typetools/jdk/blob/master/README.md " && false) |
There was a problem hiding this comment.
Bare set dumps shell environment to public CI logs.
Lines 90 and 103 run set with no arguments, which prints every shell variable (including the inherited environment). On a public repo this log is world-readable. GitHub-injected secrets are auto-masked, but anything else — GITHUB_TOKEN indirect data, CI_* values from ci-info, custom env, paths, runtime tokens — gets leaked. If the goal is debug visibility, scope it to the variables you actually need.
🔒 Suggested change
- eval $(/tmp/$USER/plume-scripts/ci-info typetools)
- set
- echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}"
+ eval $(/tmp/$USER/plume-scripts/ci-info typetools)
+ echo "CI_ORGANIZATION=${CI_ORGANIZATION} CI_BRANCH_NAME=${CI_BRANCH_NAME}"
+ echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: git merge plan | |
| run: | | |
| cd ../jdk21u && git status | |
| eval $(/tmp/$USER/plume-scripts/ci-info typetools) | |
| set | |
| echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}" | |
| - name: git merge | |
| run: | | |
| set -ex | |
| git config --global user.email "you@example.com" | |
| git config --global user.name "Your Name" | |
| git config --global pull.ff true | |
| git config --global pull.rebase false | |
| git config --global core.longpaths true | |
| git config --global core.protectNTFS false | |
| cd ../jdk21u && git status | |
| eval $(/tmp/$USER/plume-scripts/ci-info typetools) | |
| set | |
| echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}" | |
| cd ../jdk21u && git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME} || (git --version && git show | head -100 && git status && git diff | head -1000 && echo "Merge failed; see 'Pull request merge conflicts' at https://github.com/typetools/jdk/blob/master/README.md " && false) | |
| - name: git merge plan | |
| run: | | |
| cd ../jdk21u && git status | |
| eval $(/tmp/$USER/plume-scripts/ci-info typetools) | |
| echo "CI_ORGANIZATION=${CI_ORGANIZATION} CI_BRANCH_NAME=${CI_BRANCH_NAME}" | |
| echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}" | |
| - name: git merge | |
| run: | | |
| set -ex | |
| git config --global user.email "you@example.com" | |
| git config --global user.name "Your Name" | |
| git config --global pull.ff true | |
| git config --global pull.rebase false | |
| git config --global core.longpaths true | |
| git config --global core.protectNTFS false | |
| cd ../jdk21u && git status | |
| eval $(/tmp/$USER/plume-scripts/ci-info typetools) | |
| echo "CI_ORGANIZATION=${CI_ORGANIZATION} CI_BRANCH_NAME=${CI_BRANCH_NAME}" | |
| echo "About to run: git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME}" | |
| cd ../jdk21u && git pull --no-edit https://github.com/${CI_ORGANIZATION}/jdk ${CI_BRANCH_NAME} || (git --version && git show | head -100 && git status && git diff | head -1000 && echo "Merge failed; see 'Pull request merge conflicts' at https://github.com/typetools/jdk/blob/master/README.md " && false) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml.m4 around lines 86 - 105, The workflow currently
uses bare `set` in the "git merge plan" and "git merge" steps which dumps the
entire shell environment to public CI logs; remove those bare `set` calls and
instead print only the specific debug variables you actually need (e.g., echo or
printf the named vars like CI_BRANCH_NAME, CI_ORGANIZATION, GITHUB_REF, or use
`declare -p VAR_NAME` for each) or enable controlled shell tracing with `set
-x`/`set +x` around the exact commands to trace; update the occurrences of `set`
in those steps to one of these targeted approaches so the full environment is no
longer exposed.
| cftests_job(junit, cftests-junit, 17) | ||
| cftests_job(nonjunit, cftests-nonjunit, 17) | ||
| cftests_job(typecheck, typecheck, 17) | ||
| cftests_job(junit, cftests-junit, 21) | ||
| cftests_job(nonjunit, cftests-nonjunit, 21) | ||
| cftests_job(inference, cftests-inference, 21) | ||
| cftests_job(typecheck, typecheck, 21) | ||
| cftests_job(junit, cftests-junit, 25) | ||
| cftests_job(nonjunit, cftests-nonjunit, 25) | ||
| cftests_job(inference, cftests-inference, 25) | ||
| cftests_job(typecheck, typecheck, 25) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look at how the analogous Azure pipeline (already in the repo) configures cftests jobs to compare coverage.
fd -i 'azure-pipelines.yml.m4' --exec rg -n 'cftests' {}
# And check whether the inference script is gated on JDK version in checker-framework, if available locally.
rg -nP --iglob '*test-cftests-inference*' -C2Repository: typetools/jdk
Length of output: 454
🏁 Script executed:
#!/bin/bash
# Search for azure-pipelines file and examine cftests configuration
echo "=== Azure Pipeline cftests configuration ==="
fd -i 'azure-pipelines\.yml\.m4' --exec cat {} \; 2>/dev/null | rg -n 'cftests' -C2
# Search for the test-cftests-inference script
echo ""
echo "=== test-cftests-inference script ==="
fd -i 'test-cftests-inference' --type f 2>/dev/null
# Look at the ci.yml.m4 file around lines 100-140 for context/comments
echo ""
echo "=== ci.yml.m4 file structure and comments around cftests jobs ==="
rg -n 'cftests_job' .github/workflows/ci.yml.m4 -B5 -A5Repository: typetools/jdk
Length of output: 871
🏁 Script executed:
#!/bin/bash
# Find the cftests_job macro definition
echo "=== cftests_job macro definition ==="
rg -n 'define.*cftests_job|cftests_job.*(' .github/workflows/ci.yml.m4 -B2 -A10 | head -50
# Look for any comments in ci.yml.m4 about JDK versions or inference
echo ""
echo "=== Comments in ci.yml.m4 related to JDK or inference ==="
rg -n '#+.*jdk|#+.*inference|#+.*version' .github/workflows/ci.yml.m4 -i
# Search for any documentation or comments about test requirements
echo ""
echo "=== Files that might document test requirements ==="
fd -i 'readme|test|doc' --type f | grep -E '\.(md|txt|sh)$' | head -20
# Check if there's a makefile that runs cftests-inference
echo ""
echo "=== Makefiles or scripts that run cftests ==="
fd -i 'makefile|test.*cftests' --type f | head -20
</parameter>Repository: typetools/jdk
Length of output: 2400
🏁 Script executed:
#!/bin/bash
# Let's look at the actual ci.yml.m4 file to understand the macro structure
echo "=== First 150 lines of ci.yml.m4 ==="
head -150 .github/workflows/ci.yml.m4
echo ""
echo "=== Search for cftests_job definition ==="
grep -n 'cftests_job' .github/workflows/ci.yml.m4 | head -20
echo ""
echo "=== Look for m4 define patterns ==="
grep -n 'define' .github/workflows/ci.yml.m4 | head -20Repository: typetools/jdk
Length of output: 5487
🏁 Script executed:
#!/bin/bash
# Find and examine defs.m4 which contains the macro definitions
echo "=== defs.m4 file location ==="
fd -name 'defs.m4' .github/
echo ""
echo "=== Full defs.m4 file ==="
cat .github/workflows/defs.m4
echo ""
echo "=== Look for any scripts/documentation about cftests ==="
find . -name '*cftests*' -o -name '*test-cftests*' 2>/dev/null | head -20Repository: typetools/jdk
Length of output: 2746
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about JDK 17 and inference
echo "=== Search for README or documentation ==="
fd -type f -name 'README*' 2>/dev/null | xargs grep -l 'jdk.*17\|inference' 2>/dev/null | head -10
echo ""
echo "=== Search for any comments in workflows about this decision ==="
grep -n 'inference\|jdk.*17\|jdk 17' .github/workflows/ci.yml* -i
echo ""
echo "=== Check git history for when inference was added to JDK 21/25 ==="
git log --oneline --all -- .github/workflows/ci.yml.m4 | head -20
echo ""
echo "=== Check if there's any pattern in how tests are selected by version ==="
grep -E 'if|cftests_job|define' .github/workflows/defs.m4 -A2 -B2Repository: typetools/jdk
Length of output: 2383
Add a comment documenting why JDK 17 deliberately omits the inference test.
The omission is evident in ci.yml.m4: cftests_job(inference, ...) is explicitly called for JDK 21 and 25 but not JDK 17. However, there is no comment explaining why. If this is intentional (e.g., because test-cftests-inference.sh requires JDK 21+), add a comment in the file documenting the reason; otherwise, verify whether the inference test should be included for JDK 17.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml.m4 around lines 120 - 130, The workflow currently
calls cftests_job(inference, cftests-inference, 21) and 25 but omits any
cftests_job(inference, cftests-inference, 17); either add a brief inline comment
next to the existing cftests_job entries explaining why the inference test is
intentionally excluded for JDK 17 (e.g., "test-cftests-inference.sh requires JDK
21+") or, if the omission was accidental, add the missing cftests_job(inference,
cftests-inference, 17) line so JDK 17 runs inference tests—place the comment
adjacent to the cftests_job lines referencing inference to make the reasoning
obvious.
| define([canary_os], [ubuntu])dnl | ||
| define([canary_version], [25])dnl | ||
| define([latest_version], [25])dnl | ||
| define([canary_test], [canary_os[]canary_version])dnl |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Defined-but-unused metadata macros.
canary_os and canary_test are defined here but not referenced inside cftests_job / daikon_job / plume_lib_job (all of which hardcode runs-on: ubuntu-latest). Either thread these through the job templates (e.g., runs-on: canary_os-latest) or remove them to avoid the impression that changing them affects job configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/defs.m4 around lines 4 - 7, The macros canary_os and
canary_test are defined but unused, causing confusion; either remove these
unused definitions or thread them into the job templates so they actually
control runner selection. Update the job templates referenced (cftests_job,
daikon_job, plume_lib_job) to use the macros (e.g., replace hardcoded runs-on:
ubuntu-latest with runs-on: canary_os-latest or expand canary_test
appropriately) or delete the define([canary_os], [ubuntu])dnl and
define([canary_test], [canary_os[]canary_version])dnl lines from the m4 file to
avoid stale metadata. Ensure any change keeps canary_version/latest_version
behavior consistent with the other macros.
| - canary_jobs | ||
| timeout-minutes: 120 | ||
| runs-on: ubuntu-latest | ||
| container: mdernst/cf-ubuntu-jdk$3:latest |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Pin container images to a digest or stable tag instead of :latest.
All three macros pull mdernst/cf-ubuntu-jdkNN:latest. :latest is mutable, so two CI runs of the same commit can produce different results, and a compromised registry tag would silently affect every job. Pin to either an immutable digest (@sha256:…) or a date/version tag, ideally aligned with how the existing Azure pipeline references these images.
Also applies to: 34-34, 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/defs.m4 at line 15, The workflow uses mutable image tags
"mdernst/cf-ubuntu-jdk$3:latest" (and the two other identical occurrences) which
should be pinned; update each occurrence of the literal string
"mdernst/cf-ubuntu-jdk$3:latest" to use an immutable digest (e.g. "@sha256:...")
or a stable version/date tag that matches the image variants referenced by the
Azure pipeline, ensuring all three occurrences are replaced so CI runs are
deterministic and not dependent on :latest.
| define([plume_lib_job], [dnl | ||
| test_plume_lib: | ||
| needs: | ||
| - canary_jobs | ||
| runs-on: ubuntu-latest | ||
| container: mdernst/cf-ubuntu-jdk17:latest | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 25 | ||
| - name: clone git-scripts | ||
| run: mkdir -p /tmp/$USER && git -C /tmp/$USER clone --depth=1 -q https://github.com/plume-lib/git-scripts.git | ||
| - name: clone checker-framework | ||
| run: /tmp/$USER/git-scripts/git-clone-related typetools checker-framework | ||
| - name: test-plume-lib.sh | ||
| run: (cd ../checker-framework && checker/bin-devel/test-plume-lib.sh)])dnl |
There was a problem hiding this comment.
plume_lib_job ignores its argument — JDK version inconsistency.
plume_lib_job takes no parameters, but it's invoked in ci.yml.m4 as plume_lib_job(canary_version) (line 136). m4 silently discards the argument, and the container is hardcoded to mdernst/cf-ubuntu-jdk17:latest regardless of canary_version (currently 25). Either:
- Make the macro accept a JDK version (
$1) and parameterize the container — matchingcftests_job's convention and honoringcanary_version, or - Drop the argument from the call site to make it explicit that the JDK is fixed at 17.
If the JDK pin is intentional (e.g., plume-lib only supports 17), a comment in this macro stating that would prevent future confusion.
♻️ Option A — accept a JDK version parameter
-define([plume_lib_job], [dnl
+define([plume_lib_job], [dnl
test_plume_lib:
needs:
- canary_jobs
runs-on: ubuntu-latest
- container: mdernst/cf-ubuntu-jdk17:latest
+ container: mdernst/cf-ubuntu-jdk$1:latest
steps:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/defs.m4 around lines 47 - 63, The macro plume_lib_job
currently ignores any arguments and hardcodes the container image to
mdernst/cf-ubuntu-jdk17:latest; update it to either accept a JDK/version
parameter (e.g., use $1 inside the plume_lib_job definition to build the
container image tag, mirroring cftests_job) so calls like
plume_lib_job(canary_version) actually control the JDK, or remove the argument
from the call site where plume_lib_job is invoked so it's clear no parameter is
used; alternatively, if JDK17 is intentional, add a brief comment inside the
plume_lib_job macro stating that plume-lib requires JDK17 to prevent future
confusion.
| all default: ci.yml | ||
|
|
||
| %: %.m4 defs.m4 | ||
| @chmod -f +w $@ || true | ||
| m4 $< > $@ | ||
| # Squeeze multiple blank lines into one. | ||
| cat -s $@ > $@.tmp | ||
| # Remove leading blank lines. | ||
| sed '/[^[:blank:]]/,$$!d' $@.tmp > $@ | ||
| # Remove trailing blank lines. | ||
| sed -e :a -e '/^\s*$$/{$$d;N;ba' -e '}' $@ > $@.tmp | ||
| mv -f $@.tmp $@ | ||
| @chmod -w $@ | ||
|
|
||
| clean: | ||
| rm -f ci.yml |
There was a problem hiding this comment.
Add .PHONY declarations and .DELETE_ON_ERROR to harden the Makefile.
Two issues in the same recipe surface area:
-
Missing
.PHONYforall,default,clean(also flagged bycheckmake). If a file namedall,default, orcleanever exists in this directory, make will treat the target as up-to-date and silently skip the recipe. -
Partial-output corruption on m4 failure.
m4 $< > $@creates$@via shell redirection before m4 runs. If m4 fails, make aborts but leaves a stale/emptyci.ymlwhose mtime is newer thanci.yml.m4/defs.m4, so the nextmakeinvocation will consider it up-to-date and not rebuild..DELETE_ON_ERROR:removes the target on recipe failure and avoids this trap.
🛠️ Proposed fix
all default: ci.yml
+
+.PHONY: all default clean
+.DELETE_ON_ERROR:
%: %.m4 defs.m4
`@chmod` -f +w $@ || true
m4 $< > $@📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| all default: ci.yml | |
| %: %.m4 defs.m4 | |
| @chmod -f +w $@ || true | |
| m4 $< > $@ | |
| # Squeeze multiple blank lines into one. | |
| cat -s $@ > $@.tmp | |
| # Remove leading blank lines. | |
| sed '/[^[:blank:]]/,$$!d' $@.tmp > $@ | |
| # Remove trailing blank lines. | |
| sed -e :a -e '/^\s*$$/{$$d;N;ba' -e '}' $@ > $@.tmp | |
| mv -f $@.tmp $@ | |
| @chmod -w $@ | |
| clean: | |
| rm -f ci.yml | |
| all default: ci.yml | |
| .PHONY: all default clean | |
| .DELETE_ON_ERROR: | |
| %: %.m4 defs.m4 | |
| `@chmod` -f +w $@ || true | |
| m4 $< > $@ | |
| # Squeeze multiple blank lines into one. | |
| cat -s $@ > $@.tmp | |
| # Remove leading blank lines. | |
| sed '/[^[:blank:]]/,$$!d' $@.tmp > $@ | |
| # Remove trailing blank lines. | |
| sed -e :a -e '/^\s*$$/{$$d;N;ba' -e '}' $@ > $@.tmp | |
| mv -f $@.tmp $@ | |
| `@chmod` -w $@ | |
| clean: | |
| rm -f ci.yml |
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 15-15: Required target "all" is missing from the Makefile.
(minphony)
[warning] 15-15: Required target "clean" must be declared PHONY.
(minphony)
[warning] 15-15: Required target "test" is missing from the Makefile.
(minphony)
[warning] 1-1: Target "all default" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/Makefile around lines 1 - 16, Add Makefile phony and
delete-on-error declarations: declare .PHONY for the targets all, default, and
clean (so files named those won't short-circuit recipes) and add a
.DELETE_ON_ERROR: directive to ensure partial outputs (e.g., created by the m4
-> $@ redirection in the pattern rule that generates ci.yml) are removed if the
recipe fails; place these declarations near the top of the Makefile before the
rules for all/default/% and clean.
1 participant
No description provided.