build: Add Antlr jar and C++ runtime dependencies. #169
Conversation
WalkthroughAdds ANTLR support and Java JDK to the install flow: new globals for ANTLR version and jar path, two internal run-once tasks to download the ANTLR jar and build/install the ANTLR4 C++ runtime with checksum handling and integration into install flows; also adds openjdk-11-jdk to the Linux dev install script. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant Tasks as install-all-run
participant Jar as install-antlr-jar
participant Rt as install-antlr-runtime
participant Net as Remote Repos
participant FS as Filesystem
participant CMake as CMake/Build
Dev->>Tasks: invoke
activate Tasks
Tasks->>Jar: run (once)
activate Jar
Jar->>FS: check existing checksum/jar
alt checksum invalid or missing
Jar->>Net: download ANTLR jar (versioned URL)
Jar->>FS: save jar
Jar->>FS: compute/store checksum
end
deactivate Jar
Tasks->>Rt: run (once)
activate Rt
Rt->>FS: validate install prefix/dirs (checksum)
alt prefix invalid or missing
Rt->>Net: download ANTLR runtime tarball (GitHub release)
Rt->>FS: extract runtime/Cpp subtree to SOURCE_DIR
Rt->>CMake: configure (Release, no tests, CMP0135, lazy install)
CMake-->>FS: build artifacts
Rt->>CMake: install to INSTALL_PREFIX
Rt->>FS: compute/store checksum of installed files
end
deactivate Rt
deactivate Tasks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
lint-tasks.yaml (1)
48-56: Exclude auto-generated DSL code from clang-format sourcesThe
sourcesanchor now sweeps{{.G_SRC_SPIDER_DIR}}/**/*,{{.G_EXAMPLES_DIR}}/**/*, and{{.G_TEST_DIR}}/**/*. Per our earlier learning, the ANTLR-generated C++ living underG_SRC_DSL_DIR(src/stdl) must never be linted. Ifsrc/stdlsits insidesrc/spider, these glob patterns will inadvertently pick it up.Consider adding an explicit negative pattern or moving the generated folder outside the globbed roots.
♻️ Duplicate comments (1)
.github/workflows/code-linting-checks.yaml (1)
50-52: Same parallelism comment as in unit-tests workflowConfirm that
SPIDER_DEP_BUILD_PARALLELISM="1"is really what you want for linting CI; bumping it to the runner CPU count can dramatically cut wall time.
🧹 Nitpick comments (2)
lint-tasks.yaml (1)
62-66: Anchor naming collisions can confuse future editsThe list of ROOT_PATHS is anchored as
&cpp_source_files, which is semantically close to the earlier&cpp_lint_source_files. The near-identical names make it easy to misuse one for the other later.Renaming one of the anchors to something unambiguous (e.g.,
&cpp_format_root_paths) will reduce cognitive load.dep-tasks.yaml (1)
118-119: DefaultG_DEP_BUILD_PARALLELISMwhen unset
JOBS: "{{.G_DEP_BUILD_PARALLELISM}}"is great for CI throttling, but on a developer machine the variable may be undefined, causing serial builds. Add a Taskfile fallback, e.g.,vars: G_DEP_BUILD_PARALLELISM: "{{default .NUM_CPU 1}}"so contributors automatically get sensible parallelism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/code-linting-checks.yaml(1 hunks).github/workflows/unit-tests.yaml(1 hunks)dep-tasks.yaml(6 hunks)lint-tasks.yaml(3 hunks)taskfile.yaml(1 hunks)tools/scripts/lib_install/linux/install-dev.sh(1 hunks)tools/yscope-dev-utils(1 hunks)
🧠 Learnings (1)
lint-tasks.yaml (1)
Learnt from: sitaowang1998
PR: #168
File: lint-tasks.yaml:62-65
Timestamp: 2025-07-17T19:44:06.132Z
Learning: ANTLR-generated C++ code in the G_SRC_DSL_DIR (src/stdl) should not be included in linting tasks because it's auto-generated code that doesn't follow manual coding standards.
🧰 Additional context used
🧠 Learnings (1)
lint-tasks.yaml (1)
Learnt from: sitaowang1998
PR: #168
File: lint-tasks.yaml:62-65
Timestamp: 2025-07-17T19:44:06.132Z
Learning: ANTLR-generated C++ code in the G_SRC_DSL_DIR (src/stdl) should not be included in linting tasks because it's auto-generated code that doesn't follow manual coding standards.
⏰ 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). (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (5)
tools/yscope-dev-utils (1)
1-1: Submodule reference update looks goodThe new commit hash is cleanly recorded. Provided the corresponding commit exists on the remote and CI runs
git submodule update --init --recursive, no further action is required..github/workflows/unit-tests.yaml (1)
53-55: Verify that forcingSPIDER_DEP_BUILD_PARALLELISM=1achieves the intended throttlingSetting the variable to
"1"limits every CMake dependency build to a single job. Confirm this is deliberate for CI resource control; otherwise you may be paying with longer runtimes.taskfile.yaml (1)
23-25: EmptyG_DEP_BUILD_PARALLELISMmay lead to malformed-jflagsIf downstream tasks do
make -j{{.G_DEP_BUILD_PARALLELISM}}and this variable is empty, the command becomes-j, which GNU make interprets as unlimited jobs – opposite of your CI throttling goal.Ensure dep-tasks guard against an empty string (e.g., use
{{if ne .G_DEP_BUILD_PARALLELISM ""}}-j{{.G_DEP_BUILD_PARALLELISM}}{{end}}).dep-tasks.yaml (2)
74-90: Directory-level checksum may yield false negatives
INCLUDE_PATTERNS: ["{{.INSTALL_PREFIX}}"]points at a directory, not individual files.
Ifutils:checksum:validatewalks files in an undefined order, timestamp-only changes (e.g., from reinstalling without rebuild) could alter the directory mtime and force an unnecessary rebuild despite an identical payload.Consider hashing the file set (e.g.,
*.a,*.so,*.cmake,*.h) rather than the directory node itself, or reuse the CMake install manifest for deterministic input.
111-114: Policy CMP0135 requires CMake ≥ 3.24Setting
-DCMAKE_POLICY_DEFAULT_CMP0135=NEWwill fail on hosts with an older CMake, breaking local builds that don’t mirror the CI container. Please either:
- Guard the flag with a CMake version check, or
- Document the minimum supported CMake version in the BUILDING guide.
| sources: *cpp_lint_source_files | ||
| deps: [":config-cmake-project", "cpp-configs", "venv"] | ||
| cmds: | ||
| - task: ":utils:cpp-lint:clang-tidy-find" | ||
| vars: | ||
| FLAGS: | ||
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | ||
| - "-p '{{.G_SPIDER_COMPILE_COMMANDS_DB}}'" | ||
| INCLUDE_PATTERNS: | ||
| - "{{.G_SRC_SPIDER_DIR}}/**" | ||
| - "{{.G_TEST_DIR}}/**" | ||
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | ||
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | ||
| ROOT_PATHS: *cpp_source_files | ||
| ROOT_PATHS: | ||
| - "{{.G_SRC_SPIDER_DIR}}" | ||
| - "{{.G_TEST_DIR}}" | ||
| VENV_DIR: "{{.G_LINT_VENV_DIR}}" | ||
| - task: ":utils:cpp-lint:clang-tidy-find" | ||
| vars: | ||
| FLAGS: | ||
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | ||
| - "-p '{{.G_EXAMPLES_COMPILE_COMMANDS_DB}}'" | ||
| INCLUDE_PATTERNS: | ||
| - "{{.G_EXAMPLES_DIR}}/**" | ||
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | ||
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | ||
| ROOT_PATHS: *cpp_source_files | ||
| ROOT_PATHS: | ||
| - "{{.G_EXAMPLES_DIR}}" | ||
| VENV_DIR: "{{.G_LINT_VENV_DIR}}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate ROOT_PATHS fragments – use the anchor you already defined
cpp-static-check hard-codes the same directory lists that were captured in &cpp_source_files, causing duplication and a maintenance hazard if paths change.
- ROOT_PATHS:
- - "{{.G_SRC_SPIDER_DIR}}"
- - "{{.G_TEST_DIR}}"
+ ROOT_PATHS: *cpp_source_filesApply similarly to the second clang-tidy-find invocation.
📝 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.
| sources: *cpp_lint_source_files | |
| deps: [":config-cmake-project", "cpp-configs", "venv"] | |
| cmds: | |
| - task: ":utils:cpp-lint:clang-tidy-find" | |
| vars: | |
| FLAGS: | |
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | |
| - "-p '{{.G_SPIDER_COMPILE_COMMANDS_DB}}'" | |
| INCLUDE_PATTERNS: | |
| - "{{.G_SRC_SPIDER_DIR}}/**" | |
| - "{{.G_TEST_DIR}}/**" | |
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | |
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | |
| ROOT_PATHS: *cpp_source_files | |
| ROOT_PATHS: | |
| - "{{.G_SRC_SPIDER_DIR}}" | |
| - "{{.G_TEST_DIR}}" | |
| VENV_DIR: "{{.G_LINT_VENV_DIR}}" | |
| - task: ":utils:cpp-lint:clang-tidy-find" | |
| vars: | |
| FLAGS: | |
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | |
| - "-p '{{.G_EXAMPLES_COMPILE_COMMANDS_DB}}'" | |
| INCLUDE_PATTERNS: | |
| - "{{.G_EXAMPLES_DIR}}/**" | |
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | |
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | |
| ROOT_PATHS: *cpp_source_files | |
| ROOT_PATHS: | |
| - "{{.G_EXAMPLES_DIR}}" | |
| VENV_DIR: "{{.G_LINT_VENV_DIR}}" | |
| sources: *cpp_lint_source_files | |
| deps: [":config-cmake-project", "cpp-configs", "venv"] | |
| cmds: | |
| - task: ":utils:cpp-lint:clang-tidy-find" | |
| vars: | |
| FLAGS: | |
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | |
| - "-p '{{.G_SPIDER_COMPILE_COMMANDS_DB}}'" | |
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | |
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | |
| ROOT_PATHS: *cpp_source_files | |
| VENV_DIR: "{{.G_LINT_VENV_DIR}}" | |
| - task: ":utils:cpp-lint:clang-tidy-find" | |
| vars: | |
| FLAGS: | |
| - "--config-file '{{.ROOT_DIR}}/.clang-tidy'" | |
| - "-p '{{.G_EXAMPLES_COMPILE_COMMANDS_DB}}'" | |
| INCLUDE_FILENAME_PATTERNS: ["*.cpp", "*.h", "*.hpp", "*.inc"] | |
| OUTPUT_DIR: "{{.G_LINT_CLANG_TIDY_DIR}}" | |
| ROOT_PATHS: | |
| - "{{.G_EXAMPLES_DIR}}" | |
| VENV_DIR: "{{.G_LINT_VENV_DIR}}" |
🤖 Prompt for AI Agents
In lint-tasks.yaml around lines 85 to 108, the ROOT_PATHS lists are duplicated
in both clang-tidy-find tasks, which risks maintenance issues. Refactor by
replacing the hard-coded ROOT_PATHS arrays with references to the existing
anchor &cpp_source_files or a similar anchor that captures these directory
lists. Apply this change to both clang-tidy-find invocations to avoid
duplication and ensure consistency.
|
On hold. Include changes in #166. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tools/scripts/lib_install/linux/install-dev.sh (1)
41-43:curl | shis a supply-chain risk — fetch, verify checksum, then execute.This was flagged previously and remains unresolved. Please avoid piping remote scripts directly to a shell.
Apply a fetch-and-verify flow with a pinned version. Example:
-# Install uv -curl -LsSf https://astral.sh/uv/install.sh | sh +# Install uv (fetch, verify, then execute; pin version) +UV_VERSION="0.4.20" # example: set to a vetted version +UV_INSTALLER="/tmp/uv-install.sh" +UV_INSTALLER_SHA256="<EXPECTED_SHA256_FOR_${UV_VERSION}>" + +curl -LsSf "https://astral.sh/uv/install.sh" -o "${UV_INSTALLER}" +echo "${UV_INSTALLER_SHA256} ${UV_INSTALLER}" | sha256sum -c - +bash "${UV_INSTALLER}" --version "${UV_VERSION}" +rm -f "${UV_INSTALLER}"Notes:
- Replace
<EXPECTED_SHA256_FOR_${UV_VERSION}>with the official checksum published by the vendor for the selected version.- Pinning the version and verifying the checksum prevents MITM and compromised upstream incidents.
🧹 Nitpick comments (1)
tools/scripts/lib_install/linux/install-dev.sh (1)
29-29: Use a smaller, distro-portable Java runtimeWe didn’t find any references to
javac,javadocor other JDK-only tools in the repo. For runtime-only needs, prefer a headless JRE to reduce image size and track the distro’s LTS:
- File:
tools/scripts/lib_install/linux/install-dev.sh, line 29- openjdk-11-jdk \ + default-jre-headless \If you later need the full JDK (e.g. for compilation), swap in:
- openjdk-11-jdk \ + default-jdk \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tools/scripts/lib_install/linux/install-dev.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). (3)
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
sitaowang1998
changed the title
2 participants
Description
Add tasks to install
Antlrjar and C++ runtime.Checklist
breaking change.
Validation performed
Antlrjar and C++ runtime are installedfind_package(antlr4-runtime)Summary by CodeRabbit