Use only one copy of plume-scripts#782
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request moves the plume-scripts helper from 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
java/Makefile (2)
755-760:⚠️ Potential issue | 🟡 MinorMark
tags-nojtbphony.As written, a stray file named
tags-nojtbwill suppress the recipe. Add a phony declaration so the tag generation always runs when requested.Suggested fix
+.PHONY: tags-nojtb tags-nojtb: ${PLUMESCRIPTS}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@java/Makefile` around lines 755 - 760, The Makefile target "tags-nojtb" can be skipped if a file with that name exists; mark it phony to force execution: add a .PHONY declaration that includes tags-nojtb so the recipe (which uses ${ETAGS} and ${TAG_FILES_NO_JTB} and depends on ${PLUMESCRIPTS}) always runs when invoked. Ensure the .PHONY entry is placed near other phony targets or before the tags-nojtb rule.
644-755: 🛠️ Refactor suggestion | 🟠 MajorFinish centralizing the plume-scripts path in this Makefile.
The new
${PLUMESCRIPTS}variable is only used in a few places here. The rest of the file still hardcodes../.utils/plume-scripts, so the location is not actually single-sourced yet. Please switch the remaining plume-scripts references to${PLUMESCRIPTS}as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@java/Makefile` around lines 644 - 755, Search the Makefile for any hardcoded "../.utils/plume-scripts" occurrences and replace them with the centralized variable ${PLUMESCRIPTS}; update targets that reference the hardcoded path (e.g., the tags target that currently says "tags: ../.utils/plume-scripts") to use ${PLUMESCRIPTS} instead, and ensure existing rules that already use ${PLUMESCRIPTS} (such as the ${PLUMESCRIPTS} target and any prerequisites like ${PLUMESCRIPTS} in tags-nojtb or tags-without-generated-files) remain consistent; run a quick grep for "../.utils/plume-scripts" to confirm no remaining hardcoded references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 12-15: The Makefile currently does a parse-time git clone using
the PLUMESCRIPTS variable which can fail if the parent directory (.utils/)
doesn't exist and ignores NONETWORK mode; change this so the parent directory is
created and cloning only happens at build-time and only when network access is
allowed. Replace the parse-time assignment (dummy := $(shell git clone ...
${PLUMESCRIPTS})) with a proper target (e.g., a bootstrap-plume-scripts or
ensure-plumescripts target) that first runs mkdir -p $(dir ${PLUMESCRIPTS}) and
then, guarded by ifneq ($(NONETWORK),1), runs git clone --depth=1 -q ...
${PLUMESCRIPTS}; keep SORT_DIRECTORY_ORDER :=
${PLUMESCRIPTS}/sort-directory-order unchanged but make other targets depend on
the new bootstrap target so builds fail cleanly offline instead of cloning
during parsing.
---
Outside diff comments:
In `@java/Makefile`:
- Around line 755-760: The Makefile target "tags-nojtb" can be skipped if a file
with that name exists; mark it phony to force execution: add a .PHONY
declaration that includes tags-nojtb so the recipe (which uses ${ETAGS} and
${TAG_FILES_NO_JTB} and depends on ${PLUMESCRIPTS}) always runs when invoked.
Ensure the .PHONY entry is placed near other phony targets or before the
tags-nojtb rule.
- Around line 644-755: Search the Makefile for any hardcoded
"../.utils/plume-scripts" occurrences and replace them with the centralized
variable ${PLUMESCRIPTS}; update targets that reference the hardcoded path
(e.g., the tags target that currently says "tags: ../.utils/plume-scripts") to
use ${PLUMESCRIPTS} instead, and ensure existing rules that already use
${PLUMESCRIPTS} (such as the ${PLUMESCRIPTS} target and any prerequisites like
${PLUMESCRIPTS} in tags-nojtb or tags-without-generated-files) remain
consistent; run a quick grep for "../.utils/plume-scripts" to confirm no
remaining hardcoded references.
🪄 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: 5fb25337-ee3a-4abc-aba7-3c72d904e40d
📒 Files selected for processing (10)
.gitignoreMakefiledoc/Makefilejava/Makefilescripts/test-misc.shscripts/test-typecheck-with-bundled-cf.shscripts/test-typecheck-with-latest-cf.shtests/Makefiletests/dyncomp-tests/Makefiletests/kvasir-tests/Makefile
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Makefile (1)
12-14:⚠️ Potential issue | 🟠 MajorParse-time plume-scripts clone still has the same bootstrap/offline failure mode.
Line 13 still performs a parse-time
git clone, still ignoresNONETWORK, andmkdir ${DAIKONDIR}/.utilscan fail when.utilsalready exists (which skips clone).🔧 Proposed fix
ifeq (,$(wildcard ${PLUME_SCRIPTS})) - dummy := $(shell mkdir ${DAIKONDIR}/.utils && git clone --depth=1 -q https://github.com/plume-lib/plume-scripts.git ${PLUME_SCRIPTS}) +ifndef NONETWORK + dummy := $(shell mkdir -p ${DAIKONDIR}/.utils && git clone --depth=1 -q https://github.com/plume-lib/plume-scripts.git ${PLUME_SCRIPTS}) +else + $(error ${PLUME_SCRIPTS} is missing and NONETWORK is set; run 'make update-plume-scripts-in-utils' with network access first) +endif endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 12 - 14, The Makefile currently runs a parse-time shell to clone ${PLUME_SCRIPTS} using the dummy := $(shell ...) line which ignores NONETWORK, runs mkdir without -p (causing failure if ${DAIKONDIR}/.utils exists), and can break offline/bootstrapping; move this logic out of variable assignment into an explicit recipe or prerequisite rule (e.g., a fetch-plume-scripts target) that checks NONETWORK before network actions, uses mkdir -p for ${DAIKONDIR}/.utils, and performs a conditional git clone only if ${PLUME_SCRIPTS} does not already exist; update any targets that relied on the parse-time side effect to depend on this new recipe so the clone happens at build time, not at parse time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/www/pubs-sources/Makefile`:
- Line 5: The Makefile currently defines the variable PLUME_SCRIPTS (?=
${DAIKONDIR}/.utils/plume-scripts) but never uses it; remove that definition
from the Makefile (or, if it must be preserved for external consumers, add a
comment explaining why it is exported/inherited) so the file no longer contains
an unused PLUME_SCRIPTS assignment.
In `@scripts/daikon-dev.bashrc`:
- Line 8: The export line uses Makefile-style '?=' which is invalid in Bash and
leaves PLUME_SCRIPTS unset; locate the occurrences of the invalid statement (the
line that starts with export PLUME_SCRIPTS?= and references
${DAIKONDIR}/.utils/plume-scripts) and replace it with a proper Bash conditional
assignment that sets PLUME_SCRIPTS only if unset and then exports it (use Bash
parameter expansion or a separate assignment followed by export) in both places
where that invalid export appears.
In `@scripts/daikon.bashrc`:
- Around line 50-52: The current line uses invalid Makefile syntax `export
PLUME_SCRIPTS?=...`; replace it with a valid bash default-assignment and export
for PLUME_SCRIPTS (use either parameter expansion to set PLUME_SCRIPTS to
"${DAIKONDIR}/.utils/plume-scripts" only if unset, or assign then export),
updating the statement that references the PLUME_SCRIPTS variable so sourcing
the script no longer triggers a shell syntax error; target the
PLUME_SCRIPTS/DAIKONDIR assignment in scripts/daikon.bashrc.
---
Duplicate comments:
In `@Makefile`:
- Around line 12-14: The Makefile currently runs a parse-time shell to clone
${PLUME_SCRIPTS} using the dummy := $(shell ...) line which ignores NONETWORK,
runs mkdir without -p (causing failure if ${DAIKONDIR}/.utils exists), and can
break offline/bootstrapping; move this logic out of variable assignment into an
explicit recipe or prerequisite rule (e.g., a fetch-plume-scripts target) that
checks NONETWORK before network actions, uses mkdir -p for ${DAIKONDIR}/.utils,
and performs a conditional git clone only if ${PLUME_SCRIPTS} does not already
exist; update any targets that relied on the parse-time side effect to depend on
this new recipe so the clone happens at build time, not at parse time.
🪄 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: 55e052e4-d528-4c44-8a44-f8084956886a
📒 Files selected for processing (13)
.travis-build.shMakefiledoc/Makefiledoc/www/pubs-sources/Makefilejava/Makefilescripts/daikon-dev.bashrcscripts/daikon.bashrcscripts/test-misc.shscripts/test-typecheck-with-bundled-cf.shscripts/test-typecheck-with-latest-cf.shtests/Makefiletests/dyncomp-tests/Makefiletests/kvasir-tests/Makefile
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
No description provided.