Run more quietly #750
Run more quietly #750
Conversation
📝 WalkthroughWalkthroughUpdates documentation build and CI/test scripts: adds Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
🤖 Fix all issues with AI agents
In @doc/Makefile:
- Around line 102-104: The developer/index.html target uses ${CRONIC} but
doesn't list ../utils/plume-scripts as a prerequisite; update the
developer/index.html rule to add ../utils/plume-scripts to its dependency list
so Make will fetch/build plume-scripts before invoking ${CRONIC} and
${TEXI2HTML} (i.e., modify the prerequisite line for the developer/index.html
target to include ../utils/plume-scripts while keeping the existing
developer.texinfo and ../utils/html-tools entries).
- Around line 117-118: The developer.pdf target invokes ${CRONIC} but does not
declare a prerequisite on ../utils/plume-scripts, risking build races; update
the Makefile so the developer.pdf rule (target name "developer.pdf") adds
../utils/plume-scripts as a dependency and ensure the command still uses
${CRONIC} so the plume-scripts directory is fetched before running makeinfo.
- Around line 91-93: The daikon/index.html target declares dependencies but
omits the required ../utils/plume-scripts directory even though the recipe uses
${CRONIC}; add "../utils/plume-scripts" to the dependency list for the
daikon/index.html target so the build will ensure plume-scripts is present
before running the ${CRONIC} ${TEXI2HTML} recipe.
- Around line 114-115: The daikon.pdf Makefile target uses ${CRONIC} but doesn't
declare a dependency on ../utils/plume-scripts; add ../utils/plume-scripts to
the dependency list for the daikon.pdf target so the plume-scripts directory is
fetched/prepared before running ${CRONIC} makeinfo, ensuring the rule for
daikon.pdf (the daikon.pdf: daikon.texinfo config-options.texinfo
invariants-doc.texinfo target) lists ../utils/plume-scripts alongside the other
prerequisites.
- Line 7: Targets that invoke the CRONIC variable are missing a prerequisite on
the ../utils/plume-scripts directory; update the four targets that use
${CRONIC}—daikon/index.html, developer/index.html, daikon.pdf, and
developer.pdf—to include ../utils/plume-scripts alongside ../utils/html-tools as
a dependency (e.g., modify the rule that currently lists ../utils/html-tools to
also list ../utils/plume-scripts so the CRONIC script is present before those
targets run).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
doc/Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @doc/Makefile:
- Around line 91-93: The daikon/index.html rule references a non-existent cronic
script via ${CRONIC} and a missing prerequisite ../utils/plume-scripts (and the
invoked make -C .. update-plume-scripts target), making the target unrunnable;
fix by either committing the cronic script into the repo at the expected path,
or update the ${CRONIC} variable to point to a valid local wrapper (or simply
remove ${CRONIC} and call ${TEXI2HTML} directly), and remove or replace the
../utils/plume-scripts prerequisite and the make -C .. update-plume-scripts
invocation with a real update mechanism (or a no-op/phony target that ensures
the required tools exist). Ensure changes target the daikon/index.html rule and
the CRONIC variable and the update-plume-scripts prerequisite so the rule can
run without relying on missing ../utils files.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
doc/Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
🔇 Additional comments (3)
doc/Makefile (3)
7-7: LGTM!The CRONIC variable declaration follows the same pattern as other tool variables in the file (TEXI2HTML, HTMLTOOLS) and uses the conditional assignment operator correctly.
102-104: LGTM!The changes correctly mirror the pattern used for the
daikon/index.htmlrule, ensuring consistent behavior across similar targets.
114-118: LGTM! Note the selective application of CRONIC.The PDF generation rules correctly add prerequisites and wrap commands with CRONIC, consistent with the HTML rule changes.
Observation: The changes selectively apply CRONIC to the verbose commands (chapter-split HTML and PDF generation) but not to other commands like the
.infofile generation (lines 14, 17) or single-file HTML (lines 76, 82). This appears intentional and reasonable, as it targets the most verbose operations.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
doc/Makefile (1)
1324-1338: Fix incorrect temporary file reference.The temporary file naming was updated from
tmpfileto$@.tmp, but Line 1337 still references the oldtmpfilename. This will cause the perl command to fail or process the wrong file.🐛 Proposed fix
${JAVADOC_COMMAND} ${extra-javadoc-args} -J-Xmx7g -J-ea -docletpath "${DOCLETPATH}" -classpath "${DAIKON_CLASSPATH}" -doclet daikon.config.ParameterDoclet ${DOCLET_FLAGS} \ --texinfo $@.tmp \ @javadoc-files.txt - perl -pi -e 's/@cindex/\@cindex/g' tmpfile + perl -pi -e 's/@cindex/\@cindex/g' $@.tmp mv -f $@.tmp $@ @rm -f javadoc-files.txtjava/Makefile (1)
1324-1338: Fix incorrect temporary file reference (duplicate issue).Same issue as in
doc/Makefile: Line 1337 still referencestmpfileinstead of$@.tmp, causing the perl command to process the wrong file.🐛 Proposed fix
${JAVADOC_COMMAND} ${extra-javadoc-args} -J-Xmx7g -J-ea -docletpath "${DOCLETPATH}" -classpath "${DAIKON_CLASSPATH}" -doclet daikon.config.InvariantDoclet ${DOCLET_FLAGS} \ --texinfo $@.tmp \ @javadoc-files.txt - perl -pi -e 's/@cindex/\@cindex/g' tmpfile + perl -pi -e 's/@cindex/\@cindex/g' $@.tmp mv -f $@.tmp $@ @rm -f javadoc-files.txtNote: This same issue also exists in
doc/Makefileat Line 1337.
🤖 Fix all issues with AI agents
In @java/Makefile:
- Around line 647-649: The .PHONY declaration lists update-plume-scripts but the
actual make target is update-plume-scripts-in-utils; update the .PHONY line to
include the correct phony target name (e.g., change .PHONY: update-plume-scripts
to .PHONY: update-plume-scripts-in-utils or add both names) so Make treats
update-plume-scripts-in-utils as a phony target; ensure the symbol
update-plume-scripts-in-utils in the Makefile matches the .PHONY entry.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
.circleci/Makefile.circleci/config.yml.circleci/config.yml.m4doc/Makefilejava/Makefile
💤 Files with no reviewable changes (2)
- .circleci/config.yml.m4
- .circleci/config.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
🔇 Additional comments (6)
.circleci/Makefile (1)
2-3: LGTM!Adding
validateas a default dependency ensures CircleCI configuration is validated automatically on every build, improving reliability.doc/Makefile (4)
7-7: LGTM!The
CRONICvariable introduction supports the "run more quietly" objective by suppressing command output unless there's an error. The prerequisite dependencies on../utils/plume-scriptsin the doc generation targets ensure this tool is available when needed.
71-71: LGTM!The target name update from
update-plume-scriptstoupdate-plume-scripts-in-utilsproperly aligns with the corresponding changes injava/Makefile.
91-91: LGTM!Adding
../utils/plume-scriptsas a prerequisite for documentation generation targets correctly ensures thecronictool is available before it's invoked via${CRONIC}.Also applies to: 102-102, 114-114, 117-117
93-93: LGTM!Wrapping documentation generation commands with
${CRONIC}successfully implements the "run more quietly" objective by suppressing verbose output except when errors occur.Also applies to: 104-104, 115-115, 118-118
java/Makefile (1)
641-642: LGTM!The target correctly delegates to the parent Makefile using the new
update-plume-scripts-in-utilstarget name.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @java/Makefile:
- Around line 647-649: The .PHONY declaration currently lists
update-plume-scripts but the actual make target is
update-plume-scripts-in-utils; update the .PHONY line to reference the renamed
target (update-plume-scripts-in-utils) or include both names so the new target
is properly marked phony, ensuring the target definition
update-plume-scripts-in-utils and the .PHONY declaration stay consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
🔇 Additional comments (2)
java/Makefile (2)
1321-1327: LGTM! Target-specific temporary files prevent race conditions.The change from using a shared
tmpfileto target-specific$@.tmpis a good improvement. This prevents potential race conditions if multiple documentation targets are built concurrently.
1332-1339: LGTM! Consistent target-specific temporary file usage.The changes consistently apply target-specific temporary files (
$@.tmp) throughout the config-options.texinfo generation, including the perl in-place edit operation on line 1337. This matches the pattern used in the invariants-doc.texinfo target.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @java/Makefile:
- Around line 1332-1339: The config-options.texinfo make rule currently writes
to $@.tmp then edits and moves it, which can leave stale temp files and isn’t
atomic; modify the ../doc/config-options.texinfo recipe (the rule that runs
${JAVADOC_COMMAND} ... --texinfo $@.tmp and the subsequent perl -pi and mv
steps) to remove any existing $@.tmp before generating (e.g., rm -f $@.tmp) and
ensure the sequence is generate → edit → mv so the final mv replaces the target
atomically, keeping the perl edit and final mv intact.
- Around line 1321-1327: The rule that writes ${@}.tmp (the target using
${JAVADOC_COMMAND} and javadoc-files.txt producing $@.tmp) should remove any
stale temp before generating output; add a pre-step to rm -f $@.tmp (or
otherwise ensure $@.tmp is removed) immediately before invoking
${JAVADOC_COMMAND}, so the mv -f $@.tmp $@ never promotes leftover artifacts
from a prior failed run.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
🔇 Additional comments (1)
java/Makefile (1)
641-650: No recursion risk; however, root Makefile has incomplete refactoring.The java/Makefile delegation at line 648-650 does not create recursion. The root Makefile's
update-plume-scripts-in-utilstarget does not call back into java/Makefile; it only updatesutils/plume-scripts.However, the root Makefile declares
update-plume-scripts(without-in-utils) as.PHONYon line 747 and includes it as a dependency inupdate-libs(line 741), but this target has no implementation. This is an issue in the root Makefile's refactoring, not in the java/Makefile change itself.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.circleci/defs.m4:
- Around line 58-60: Add a short explanatory comment next to the COMSPEC
workaround in .circleci/defs.m4 (the line exporting COMSPEC and ComSpec before
running ./scripts/test-misc.sh) that states it is to work around a JDK 25/Rocky
Linux environment issue (reference the original commit if available);
alternatively add a CircleCI `name:` field for that run step with the same
explanation so future maintainers understand why COMSPEC/ComSpec are being
cleared.
In @doc/Makefile:
- Around line 93-95: The daikon/index.html make rule uses
../.utils/plume-scripts as a prerequisite instead of the consistent ${CRONIC};
update the dependency list for the daikon/index.html target to replace
../.utils/plume-scripts with ${CRONIC} so the rule requires the cronic helper
like the other targets (match the rule name daikon/index.html and keep the rest
of prerequisites and commands unchanged).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.circleci/config.yml.circleci/defs.m4doc/Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
🔇 Additional comments (7)
doc/Makefile (5)
7-8: LGTM!Clear comment explaining
cronic's purpose, and the variable follows the existing pattern of using?=for overridable defaults.
37-37: Cosmetic comment update.No functional change.
71-73: LGTM!The prerequisite chain correctly ensures
plume-scriptsis fetched before${CRONIC}is used.
104-106: LGTM!Proper use of
${CRONIC}as both prerequisite and command wrapper.
116-120: LGTM!Both PDF rules consistently use
${CRONIC}as prerequisite and wrapper..circleci/config.yml (2)
331-333: Changes are consistent across all misc jobs.The COMSPEC/ComSpec environment variable exports are applied uniformly to all misc jobs (Ubuntu and Rocky Linux), matching the m4 template. This defensive approach ensures consistent behavior across all environments.
942-965: Rocky Linux misc jobs correctly updated.Both
misc-rockylinux-jdk8andmisc-rockylinux-jdk25have the COMSPEC workaround, consistent with the Ubuntu jobs and the m4 template.
| - run: | ||
| command: ./scripts/test-misc.sh | ||
| command: export COMSPEC="" && export ComSpec="" && ./scripts/test-misc.sh | ||
| no_output_timeout: 20m])dnl |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a comment explaining the COMSPEC workaround.
The environment variable exports work around a JDK 25/Rocky Linux issue (per commit message), but future maintainers won't know why this is here. A brief inline comment would help.
📝 Suggested documentation
- run:
+ # Clear COMSPEC/ComSpec to work around JDK 25 issue on Rocky Linux
command: export COMSPEC="" && export ComSpec="" && ./scripts/test-misc.sh
no_output_timeout: 20m])dnlNote: CircleCI YAML supports comments, so this could be added as a name: field instead if preferred.
📝 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.
| - run: | |
| command: ./scripts/test-misc.sh | |
| command: export COMSPEC="" && export ComSpec="" && ./scripts/test-misc.sh | |
| no_output_timeout: 20m])dnl | |
| - run: | |
| # Clear COMSPEC/ComSpec to work around JDK 25 issue on Rocky Linux | |
| command: export COMSPEC="" && export ComSpec="" && ./scripts/test-misc.sh | |
| no_output_timeout: 20m])dnl |
🤖 Prompt for AI Agents
In @.circleci/defs.m4 around lines 58 - 60, Add a short explanatory comment next
to the COMSPEC workaround in .circleci/defs.m4 (the line exporting COMSPEC and
ComSpec before running ./scripts/test-misc.sh) that states it is to work around
a JDK 25/Rocky Linux environment issue (reference the original commit if
available); alternatively add a CircleCI `name:` field for that run step with
the same explanation so future maintainers understand why COMSPEC/ComSpec are
being cleared.
| daikon/index.html: daikon.texinfo config-options.texinfo invariants-doc.texinfo images/dfepl-flow.jpg ../.utils/html-tools ../.utils/plume-scripts | ||
| rm -rf daikon/* | ||
| ${TEXI2HTML} --split=chapter -o daikon $< | ||
| ${CRONIC} ${TEXI2HTML} --split=chapter -o daikon $< |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent prerequisite: use ${CRONIC} instead of ../.utils/plume-scripts.
Other rules (developer/index.html, daikon.pdf, developer.pdf) use ${CRONIC} as a prerequisite, which ensures the cronic script file exists. This rule uses the directory directly, creating an inconsistency.
♻️ Suggested fix
-daikon/index.html: daikon.texinfo config-options.texinfo invariants-doc.texinfo images/dfepl-flow.jpg ../.utils/html-tools ../.utils/plume-scripts
+daikon/index.html: daikon.texinfo config-options.texinfo invariants-doc.texinfo images/dfepl-flow.jpg ../.utils/html-tools ${CRONIC}📝 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.
| daikon/index.html: daikon.texinfo config-options.texinfo invariants-doc.texinfo images/dfepl-flow.jpg ../.utils/html-tools ../.utils/plume-scripts | |
| rm -rf daikon/* | |
| ${TEXI2HTML} --split=chapter -o daikon $< | |
| ${CRONIC} ${TEXI2HTML} --split=chapter -o daikon $< | |
| daikon/index.html: daikon.texinfo config-options.texinfo invariants-doc.texinfo images/dfepl-flow.jpg ../.utils/html-tools ${CRONIC} | |
| rm -rf daikon/* | |
| ${CRONIC} ${TEXI2HTML} --split=chapter -o daikon $< |
🤖 Prompt for AI Agents
In @doc/Makefile around lines 93 - 95, The daikon/index.html make rule uses
../.utils/plume-scripts as a prerequisite instead of the consistent ${CRONIC};
update the dependency list for the daikon/index.html target to replace
../.utils/plume-scripts with ${CRONIC} so the rule requires the cronic helper
like the other targets (match the rule name daikon/index.html and keep the rest
of prerequisites and commands unchanged).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @doc/Makefile:
- Around line 119-120: The developer.pdf target declares ${CRONIC} as a
prerequisite but doesn't use it in the recipe; either remove ${CRONIC} from the
prerequisite list or invoke it in the recipe by prefixing the command with
${CRONIC} (i.e., change the recipe from "makeinfo --pdf $<" to use "${CRONIC}
makeinfo --pdf $<") so the declared dependency is actually applied; update the
developer.pdf rule accordingly referencing the target name developer.pdf, the
variable ${CRONIC}, and the existing command makeinfo --pdf $<.
- Around line 116-117: The daikon.pdf target lists ${CRONIC} as a dependency but
the recipe still calls makeinfo directly; update the daikon.pdf recipe to invoke
makeinfo via the ${CRONIC} wrapper (e.g., use "${CRONIC} makeinfo --pdf $<") so
the ${CRONIC} dependency is actually used and the build runs quietly while
keeping the target and prerequisites (daikon.texinfo, config-options.texinfo,
invariants-doc.texinfo) unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
doc/Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: codespecs.daikon (typecheck_bundled_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part3_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part1_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_part2_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
🔇 Additional comments (5)
doc/Makefile (5)
7-8: LGTM!Clear definition with a helpful explanatory comment. Using
?=allows overriding the path, consistent with the existingHTMLTOOLSpattern.
37-37: LGTM on the comment clarity improvement.
71-73: LGTM!The dependency chain correctly ensures
plume-scriptsis fetched before any target requiring${CRONIC}.
93-95: LGTM!Correctly wraps
${TEXI2HTML}with${CRONIC}and adds the necessaryplume-scriptsprerequisite.
104-106: LGTM!Correctly wraps
${TEXI2HTML}with${CRONIC}and adds${CRONIC}as a prerequisite.
| daikon.pdf: daikon.texinfo config-options.texinfo invariants-doc.texinfo ${CRONIC} | ||
| makeinfo --pdf $< |
There was a problem hiding this comment.
${CRONIC} dependency is added but not used in the recipe.
The target depends on ${CRONIC} but the recipe still runs makeinfo --pdf $< directly. Based on the pattern used for HTML targets and the PR objective to run more quietly, this appears to be an oversight.
Proposed fix
daikon.pdf: daikon.texinfo config-options.texinfo invariants-doc.texinfo ${CRONIC}
- makeinfo --pdf $<
+ ${CRONIC} makeinfo --pdf $<📝 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.
| daikon.pdf: daikon.texinfo config-options.texinfo invariants-doc.texinfo ${CRONIC} | |
| makeinfo --pdf $< | |
| daikon.pdf: daikon.texinfo config-options.texinfo invariants-doc.texinfo ${CRONIC} | |
| ${CRONIC} makeinfo --pdf $< |
🤖 Prompt for AI Agents
In @doc/Makefile around lines 116 - 117, The daikon.pdf target lists ${CRONIC}
as a dependency but the recipe still calls makeinfo directly; update the
daikon.pdf recipe to invoke makeinfo via the ${CRONIC} wrapper (e.g., use
"${CRONIC} makeinfo --pdf $<") so the ${CRONIC} dependency is actually used and
the build runs quietly while keeping the target and prerequisites
(daikon.texinfo, config-options.texinfo, invariants-doc.texinfo) unchanged.
| developer.pdf: developer.texinfo ${CRONIC} | ||
| makeinfo --pdf $< |
There was a problem hiding this comment.
Same issue: ${CRONIC} dependency added but not used in the recipe.
Proposed fix
developer.pdf: developer.texinfo ${CRONIC}
- makeinfo --pdf $<
+ ${CRONIC} makeinfo --pdf $<📝 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.
| developer.pdf: developer.texinfo ${CRONIC} | |
| makeinfo --pdf $< | |
| developer.pdf: developer.texinfo ${CRONIC} | |
| ${CRONIC} makeinfo --pdf $< |
🤖 Prompt for AI Agents
In @doc/Makefile around lines 119 - 120, The developer.pdf target declares
${CRONIC} as a prerequisite but doesn't use it in the recipe; either remove
${CRONIC} from the prerequisite list or invoke it in the recipe by prefixing the
command with ${CRONIC} (i.e., change the recipe from "makeinfo --pdf $<" to use
"${CRONIC} makeinfo --pdf $<") so the declared dependency is actually applied;
update the developer.pdf rule accordingly referencing the target name
developer.pdf, the variable ${CRONIC}, and the existing command makeinfo --pdf
$<.
1 participant
No description provided.