Use is-ci.sh from plume-lib
#728
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRenamed the Makefile target Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
784-791: Target renamed but maintains same directory as before.The target has been renamed from
update-plume-scriptstoupdate-plume-scripts-in-utilsto be more explicit about which directory it maintains. However, scripts/test-misc.sh line 23 callsmake plume-scripts-updatewhich doesn't exist, suggesting incomplete coordination of this refactoring.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
Makefile(2 hunks)java/Makefile(2 hunks)scripts/is-ci.sh(0 hunks)scripts/test-misc.sh(3 hunks)
💤 Files with no reviewable changes (1)
- scripts/is-ci.sh
⏰ 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). (7)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
🔇 Additional comments (4)
java/Makefile (1)
1343-1343: Good refactoring to use the PLUMESCRIPTS variable.Using the
${PLUMESCRIPTS}variable instead of a hard-coded path improves maintainability and consistency.scripts/test-misc.sh (3)
24-30: Good addition of dynamic parallelism based on CI detection.The logic to detect CI environments and adjust parallelism accordingly is a sensible approach to avoid out-of-memory errors in resource-constrained CI environments while taking advantage of available cores locally.
61-61: Path.plume-scripts/ci-lint-diffis properly initialized before use.The
.plume-scriptsdirectory is cloned by the root Makefile (lines 324-325) during themake compile daikon.jarcall at line 14, which executes before.plume-scripts/ci-lint-diffis referenced at lines 61 and 67. The dependency chain ensures the directory exists when needed. The path reference is consistent throughout the script.
25-29: No issue found with this code. The.plume-scripts/is-ci.shscript, which obtains information about a CI job, is part of the plume-scripts repository and will be available by line 25. Here's why: whenmake compile daikon.jarexecutes at line 17, it triggers immediate parsing of the main Makefile. During this parsing phase, lines 324–327 execute unconditionally: the shell clone command at line 325 populates.plume-scriptsif it doesn't exist, followed by the include at line 327. This happens before any make targets are built, and therefore before the script reaches line 25. The code is safe as written.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
java/Makefile (1)
14-14: Resolve the plume-scripts directory inconsistency.This change maintains the inconsistency flagged in previous review. The main Makefile defines
PLUMESCRIPTS ?= ${DAIKONDIR}/utils/plume-scriptsand has anupdate-plume-scripts-in-utilstarget, but this file overrides it to point to.plume-scripts, creating two separate plume-scripts directories in the codebase.Per the previous review suggestion: either remove this line to inherit the canonical location from the main Makefile, or change it to match:
PLUMESCRIPTS ?= ${DAIKONDIR}/utils/plume-scripts.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/Makefile(2 hunks)
⏰ 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). (7)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
java/Makefile (2)
7-9: Diagnostic output already flagged in previous review.These diagnostic lines were already commented on in the previous review, suggesting they should be made conditional or removed after debugging.
15-15: PLUMESCRIPTS directory inconsistency already flagged.The change to
.plume-scriptswas already flagged in the previous review as potentially inconsistent with the main Makefile's directory structure.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/Makefile(2 hunks)
⏰ 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). (7)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
🔇 Additional comments (1)
java/Makefile (1)
5-5: Inconsistency between AI summary and code.The AI summary states "Removed inclusion of Makefile.user", but line 5 still contains
-include Makefile.user(unchanged, as indicated by the absence of a~marker). The inclusion ofMakefile.userhas not been removed in this PR.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
java/Makefile (2)
7-8: Diagnostic output should be conditional (duplicate of past review).These diagnostic lines print on every Make invocation. Per previous review feedback, consider making them conditional (e.g., only when DEBUG=1) or removing them if they were temporary debugging aids.
17-17: PLUMESCRIPTS directory inconsistency (duplicate of past review).This line sets PLUMESCRIPTS to
.plume-scripts, which conflicts with the main Makefile's use ofutils/plume-scripts. Per previous review feedback, this inconsistency should be resolved.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
java/Makefile(2 hunks)
⏰ 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). (7)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
🔇 Additional comments (1)
java/Makefile (1)
1348-1348: LGTM: Correctly uses variable references.The change to use
${PLUMESCRIPTS}instead of a hardcoded path improves maintainability. Both${JAVADOC_DEST}and${PLUMESCRIPTS}are correctly referenced here.Note: This assessment assumes the PLUMESCRIPTS directory inconsistency flagged at line 17 will be resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
java/Makefile (1)
17-17: Duplicate: PLUMESCRIPTS directory inconsistency already flagged.This line has already been flagged in previous review comments as creating inconsistency between
.plume-scriptsandutils/plume-scriptsdirectories. Please refer to the existing review comment for details.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
java/Makefile(2 hunks)scripts/test-typecheck-with-bundled-cf.sh(2 hunks)scripts/test-typecheck-with-latest-cf.sh(1 hunks)
⏰ 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). (7)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_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 (5)
java/Makefile (2)
9-10: Good fix: diagnostic commands now commented out.The previously flagged critical issue with incorrect
$(set)and$(env)syntax has been resolved by commenting out these lines. If these diagnostics are needed in the future, use$(shell set)and$(shell env)instead.
1348-1348: LGTM: Correct variable usage.The preplace invocation correctly uses
${JAVADOC_DEST}(properly spelled) and${PLUMESCRIPTS}variables. This appears to resolve the typo issue mentioned in previous reviews.Note: This comment assumes the PLUMESCRIPTS directory inconsistency (flagged at line 17) will be resolved separately.
scripts/test-typecheck-with-bundled-cf.sh (1)
17-19: LGTM: Improved script organization.Moving the SCRIPT_DIR calculation earlier in the script improves consistency with other CI-related scripts and follows bash best practices.
scripts/test-typecheck-with-latest-cf.sh (2)
32-37: LGTM: Correct parallelism logic and indentation.The CPU-based parallelism logic correctly limits to 1 job in CI to avoid out-of-memory errors, and uses CPU count otherwise. The indentation is also correct with the
makecommand properly outside the if/else block.
25-25: Theupdate-git-scriptsMake target exists and is properly configured.The target is defined in the root Makefile (line 766) as a phony target and correctly maintains the
utils/git-scriptsdirectory by either pulling updates if it already exists or cloning it from the plume-lib repository if it doesn't. The implementation includes retry logic for robustness.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
scripts/test-typecheck-with-bundled-cf.sh(2 hunks)
⏰ 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). (7)
- GitHub Check: codespecs.daikon (typecheck_latest_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nontxt_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (misc_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (quick_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (nonquick_ubuntu_jdk25)
🔇 Additional comments (2)
scripts/test-typecheck-with-bundled-cf.sh (2)
2-2: LGTM!The updated comment accurately explains the bash requirement for
set -o pipefail.
17-19: LGTM!The SCRIPT_DIR calculation and directory change are correctly implemented with proper quoting and error suppression.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
scripts/test-typecheck-with-bundled-cf.sh(2 hunks)scripts/test-typecheck-with-latest-cf.sh(1 hunks)tests/Makefile(1 hunks)tests/dyncomp-tests/Makefile(1 hunks)tests/kvasir-tests/Makefile(2 hunks)
⏰ 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). (7)
- GitHub Check: codespecs.daikon (typecheck_bundled_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (kvasir_ubuntu_jdk25)
- GitHub Check: codespecs.daikon (typecheck_latest_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)
🔇 Additional comments (7)
scripts/test-typecheck-with-bundled-cf.sh (1)
17-18: LGTM! Good refactoring of directory handling.Setting up
SCRIPT_DIRand immediately changing to the repository root simplifies the rest of the script by making all subsequent paths relative to the repo root. This is a clean pattern.tests/dyncomp-tests/Makefile (1)
65-65: LGTM! Path update is correct.The script path has been properly updated to reference the centralized
.plume-scriptsdirectory.tests/Makefile (1)
53-53: LGTM! Path update is correct.The script path has been properly updated to reference the centralized
.plume-scriptsdirectory, consistent with the other test Makefiles.tests/kvasir-tests/Makefile (2)
27-27: LGTM! Path update is correct.The script path has been properly updated to reference the centralized
.plume-scriptsdirectory.
81-81: Path is correctly updated and consistently applied.Both
regression-testsandclean-alltargets use the same path to theis-ci.shscript:"${DAIKONDIR}"/.plume-scripts/is-ci.sh. This aligns with the repository's design, where the plume-scripts directory is automatically cloned from https://github.com/plume-lib/plume-scripts.git to.plume-scriptswhen needed (as managed by the root Makefile).scripts/test-typecheck-with-latest-cf.sh (2)
37-37: LGTM: Proper parallelism control based on CI detection.The typecheck invocation correctly uses the computed
num_jobsvalue, which appropriately limits parallelism to 1 on CI environments to avoid out-of-memory errors while leveraging multiple cores locally.
32-36: The.plume-scriptsdirectory is properly ensured by the Makefile before use.The script relies on
.plume-scripts/is-ci.shat line 32, but this is safe because the Makefile automatically clones the plume-scripts repository into.plume-scriptswhen the Makefile is first evaluated (lines 324-325), which occurs whenmake compile daikon.jarruns at line 21.However, the git clone operation happens in a silent Makefile variable assignment (
dummy := $(shell git clone ...)), so if the clone fails, the failure is not propagated. The script would then fail with an unhelpful error when trying to execute the missing script. Consider adding explicit error handling to the Makefile's clone operation or verifying the clone succeeded.
| if [ -n "$(.plume-scripts/is-ci.sh)" ]; then | ||
| num_jobs=1 | ||
| else | ||
| num_jobs="$(nproc || sysctl -n hw.ncpu || getconf _NPROCESSORS_ONLN || echo 1)" | ||
| make -C java --jobs="$num_jobs" typecheck | ||
| fi | ||
| make -C java --jobs="$num_jobs" typecheck |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that .plume-scripts/is-ci.sh exists and is executable
echo "Checking for .plume-scripts/is-ci.sh..."
if [ -f .plume-scripts/is-ci.sh ]; then
echo "✓ File exists"
ls -lh .plume-scripts/is-ci.sh
if [ -x .plume-scripts/is-ci.sh ]; then
echo "✓ File is executable"
else
echo "✗ File is NOT executable"
fi
else
echo "✗ File does NOT exist"
echo ""
echo "Searching for is-ci.sh in the repository..."
fd -t f 'is-ci.sh' 2>/dev/null || find . -name 'is-ci.sh' -type f 2>/dev/null | head -20
fiRepository: codespecs/daikon
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check directory structure and search for CI detection logic
echo "=== Checking for .plume-scripts directory ==="
if [ -d .plume-scripts ]; then
echo "✓ .plume-scripts directory exists"
ls -la .plume-scripts/
else
echo "✗ .plume-scripts directory does NOT exist"
fi
echo ""
echo "=== Looking for scripts directory ==="
fd -t d 'scripts' | head -10
echo ""
echo "=== Searching for any is-ci related files ==="
fd 'is-ci' 2>/dev/null || echo "No is-ci files found"
echo ""
echo "=== Checking the actual test-typecheck script ==="
head -35 scripts/test-typecheck-with-bundled-cf.shRepository: codespecs/daikon
Length of output: 1245
🏁 Script executed:
#!/bin/bash
# Check scripts directory and search for CI patterns
echo "=== Contents of scripts/ directory ==="
ls -la scripts/
echo ""
echo "=== Searching for CI environment variable patterns ==="
rg 'CI|GITHUB|GITLAB|TRAVIS|CIRCLECI' scripts/ --type sh 2>/dev/null | head -20
echo ""
echo "=== Searching for any executable scripts that might do CI detection ==="
find scripts/ -type f -executable 2>/dev/null
echo ""
echo "=== Checking for CI-related patterns in all shell scripts ==="
rg '\$CI|CI=' scripts/ --type sh 2>/dev/null | head -20Repository: codespecs/daikon
Length of output: 9951
🏁 Script executed:
#!/bin/bash
# Check other test scripts to see how they handle CI detection
echo "=== test-typecheck-with-latest-cf.sh CI detection ==="
grep -A 3 "Under CI" scripts/test-typecheck-with-latest-cf.sh
echo ""
echo "=== test-misc.sh CI detection ==="
grep -A 3 "Under CI" scripts/test-misc.sh
echo ""
echo "=== Full context around is-ci in test-typecheck-with-bundled-cf.sh ==="
grep -B 2 -A 3 "is-ci" scripts/test-typecheck-with-bundled-cf.shRepository: codespecs/daikon
Length of output: 725
🏁 Script executed:
#!/bin/bash
# Check root directory and look for any .plume scripts
echo "=== Root directory listing (looking for .plume*) ==="
ls -la | grep plume
echo ""
echo "=== All hidden directories at root ==="
ls -da .*/
echo ""
echo "=== Search entire repo for is-ci ==="
find . -name '*is-ci*' -o -name '*is_ci*' 2>/dev/null
echo ""
echo "=== Check git for any deleted or untracked .plume-scripts ==="
git ls-files | grep plumeRepository: codespecs/daikon
Length of output: 347
.plume-scripts/is-ci.sh does not exist and will cause runtime failure.
The script references .plume-scripts/is-ci.sh on line 27, but this file does not exist in the repository. The .plume-scripts directory itself does not exist. When executed, this line will fail with a "command not found" error.
Either create the missing .plume-scripts/is-ci.sh script or replace the CI detection logic with standard environment variable checks (e.g., $CI, $GITHUB_ACTIONS, $GITLAB_CI, etc.).
🤖 Prompt for AI Agents
In scripts/test-typecheck-with-bundled-cf.sh around lines 27 to 32, the script
calls .plume-scripts/is-ci.sh which doesn't exist and will cause a runtime
failure; replace that call with standard CI environment checks (e.g., test -n
"$CI" || test -n "$GITHUB_ACTIONS" || test -n "$GITLAB_CI") or add a small
inline detection like: if [ -n "$CI" ] || [ -n "$GITHUB_ACTIONS" ] || [ -n
"$GITLAB_CI" ]; then num_jobs=1; else ... fi, ensuring no external missing
script is referenced.
| make update-git-scripts | ||
| utils/git-scripts/git-clone-related typetools checker-framework |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for update-git-scripts in Makefile
rg -n "update-git-scripts" MakefileRepository: codespecs/daikon
Length of output: 437
🏁 Script executed:
# Read the script to see lines 25-26 and surrounding context
cat -n scripts/test-typecheck-with-latest-cf.sh | sed -n '1,45p'Repository: codespecs/daikon
Length of output: 1662
🏁 Script executed:
# Check if .plume-scripts/is-ci.sh exists in the repository
fd "is-ci.sh" . --type fRepository: codespecs/daikon
Length of output: 42
.plume-scripts/is-ci.sh does not exist in the repository.
Line 32 attempts to execute .plume-scripts/is-ci.sh, but this file is not present. With set -e on line 7, the script will fail when it reaches this line. Ensure the file exists or is created during an earlier setup step (e.g., via a plume-scripts initialization target).
1 participant
No description provided.