build!: Install ystdlib as a CMake package to avoid issues when projects use both Spider and ystdlib through add_subdirectory. - #160
Conversation
WalkthroughThe changes update the CMake minimum version, switch ystdlib integration from a subdirectory build to using Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant install-dev.sh
participant install-cmake.sh
participant System
User->>install-dev.sh: Run script
install-dev.sh->>install-cmake.sh: Call with version 3.23.5
install-cmake.sh->>System: Download and extract CMake source
install-cmake.sh->>System: Build and install CMake
install-cmake.sh->>install-dev.sh: Return control
install-dev.sh->>System: Run check-cmake-version.sh
sequenceDiagram
participant CMake
participant System
participant ystdlib (pre-installed)
CMake->>System: Run configure
CMake->>ystdlib (pre-installed): find_package(ystdlib)
ystdlib (pre-installed)-->>CMake: Provide package info (if found)
CMake->>System: Print status message
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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 comments)
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
🧹 Nitpick comments (7)
tools/scripts/lib_install/install-cmake.sh (4)
6-10: Exit with a non-zero status when printing usage
exitwithout an explicit code may propagate a previous success status, misleading CI checks.- exit + exit 1
13-16:sudo echois a fragile privilege test
sudo echocan still prompt for a password (breaking unattended runs) and produces no actionable side-effect. Prefer a one-liner that validates sudo rights up-front:if ! sudo -n true 2>/dev/null; then echo "Password-less sudo not available; the build may stall." fi
18-20: Usenprocfallback for CPU count
/proc/cpuinfois Linux-only and may misbehave in containers with CPU limits.nprocis more portable:-num_cpus=$(grep -c ^processor /proc/cpuinfo) +num_cpus=$(nproc 2>/dev/null || grep -c ^processor /proc/cpuinfo)
46-47: Ensure cleanup even on early exitIf the build fails, the temp directory persists. Add a trap:
trap 'rm -rf "$temp_dir"' EXITCMakeLists.txt (1)
181-185: Missing version hint forystdlibIf multiple incompatible
ystdlibversions are published,find_package(ystdlib REQUIRED)may resolve an older one first on users’ systems. Consider:find_package(ystdlib 0.4 REQUIRED) # replace with actual minimumThis safeguards subtle ABI breaks.
dep-tasks.yaml (2)
15-26: Duplicateinstall-boostdependency
install-all-runalready depends oninstall-boost; adding it again insideinstall-ystdlibis redundant and lengthens no-op DAG traversal. Removing the extra edge keeps the task graph cleaner.
57-76: Hard-coded commit hash without commentThe URL pins
ystdlibto commit0ae886cbut there is no inline rationale (tag, security fix, etc.). Future maintainers will struggle to know when it is safe to upgrade. Append a brief comment, e.g.:# 0ae886c = v1.2.0 + cmake-package support
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CMakeLists.txt(2 hunks)dep-tasks.yaml(2 hunks)tools/scripts/lib_install/install-cmake.sh(1 hunks)tools/scripts/lib_install/linux/install-dev.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (2)
tools/scripts/lib_install/linux/install-dev.sh (1)
34-37: Possible double-sudo invocation
privileged_command_prefixmay already besudo, yetinstall-cmake.shinternally invokessudowhen EUID ≠ 0. Runningsudo sudo …is harmless on most systems but unnecessary and can confuse log analysis. You could call the installer unprivileged and let it elevate itself:-${privileged_command_prefix} "$lib_install_scripts_dir"/install-cmake.sh 3.23.5 +"$lib_install_scripts_dir"/install-cmake.sh 3.23.5CMakeLists.txt (1)
1-1: Bump to 3.23 may break downstream consumersRaising
cmake_minimum_requiredto 3.23 is fine for the dev container, but external users on LTS distros (e.g., Ubuntu 22.04 ships 3.22.x) will now fail to configure. Confirm that packaging/distribution notes and CI matrices reflect this new floor version.
| tar_filename=cmake-${version}.tar.gz | ||
| curl -fsSL https://github.com/Kitware/CMake/releases/download/v${version}/${tar_filename} -o ${tar_filename} | ||
| tar xzf ${tar_filename} | ||
| cd cmake-${version} |
There was a problem hiding this comment.
No integrity check on downloaded tarball
Blindly piping an archive into the build chain is a supply-chain risk. Fetch the official SHA256 from the same release and validate:
curl -fsSL -O https://github.com/Kitware/CMake/releases/download/v${version}/${tar_filename}.sha256
sha256sum -c ${tar_filename}.sha256🤖 Prompt for AI Agents
In tools/scripts/lib_install/install-cmake.sh around lines 30 to 33, the script
downloads and extracts the CMake tarball without verifying its integrity. To fix
this, add steps to download the corresponding SHA256 checksum file from the same
release URL and verify the downloaded tarball using sha256sum -c before
extracting it. This ensures the tarball is authentic and prevents supply-chain
risks.
| temp_dir=/tmp/${package_name}-installation | ||
| mkdir -p $temp_dir |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Race-safe temporary directory
A fixed /tmp/cmake-installation can collide with parallel runs. Leverage mktemp:
-temp_dir=/tmp/${package_name}-installation
-mkdir -p $temp_dir
+temp_dir=$(mktemp -d /tmp/${package_name}-installation-XXXXXX)🤖 Prompt for AI Agents
In tools/scripts/lib_install/install-cmake.sh around lines 24 to 25, the
temporary directory is set to a fixed path which can cause collisions in
parallel runs. Replace the fixed directory assignment with a call to mktemp to
create a unique, race-safe temporary directory. Use mktemp with appropriate
options to generate a unique directory under /tmp and assign it to temp_dir.
ystdlib as a package to avoid duplicate ystdlib files.ystdlib as a package to avoid duplicate ystdlib files.
| CMAKE_SETTINGS_DIR: "{{.G_DEPS_CMAKE_SETTINGS_DIR}}" | ||
| GEN_ARGS: | ||
| - "-DCMAKE_POSITION_INDEPENDENT_CODE=ON" | ||
| - "-Dystdlib_BUILD_TESTING=OFF" | ||
| - "-C {{.G_DEPS_CMAKE_SETTINGS_DIR}}/boost.cmake" | ||
| - "-DCMAKE_POLICY_DEFAULT_CMP0144=NEW" |
There was a problem hiding this comment.
Previously we discussed whether CMAKE_POSITION_INDEPENDENT_CODE and CMAKE_POLICY_DEFAULT_CMP0144. Iirc it wasn't clear they were necessary.
I remember I couldn't reproduce needing CMAKE_POLICY_DEFAULT_CMP0144 and iirc I don't think you could either.
If they are necessary the reasons need to be documented in the description.
There was a problem hiding this comment.
PIC is required because Spider produces a shared library.
CMP0144 is removed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dep-tasks.yaml (1)
74-77: Explain or drop hard-coding of CMP0144 policy
-DCMAKE_POLICY_DEFAULT_CMP0144=NEWis still being forced even though earlier discussion concluded we could not reproduce the need for it. This policy changes howproject()sets variables and can silently mask configuration issues in downstream packages. If there is now concrete evidence that ystdlib requires it, please document the reason; otherwise remove the flag to keep the build surface minimal.
🧹 Nitpick comments (2)
dep-tasks.yaml (2)
62-64: Redundant dependency declaration
install-ystdlibis already executed afterinstall-boostvia theinstall-all-runchain, so keepinginstall-boostin the localdeps:list is unnecessary duplication and slightly stretches the DAG. Consider dropping it for clarity.
59-70: PassJOBSto align with other install tasksAll other
install-*tasks propagateJOBS: "{{.G_DEPS_MAX_PARALLELISM_PER_TASK}}"to:utils:cmake:install-remote-tar, enabling parallel builds. ystdlib omits this, which will fall back to the tool’s default and slow CI-runtime.GEN_ARGS: - "-DCMAKE_POSITION_INDEPENDENT_CODE=ON" - "-Dystdlib_BUILD_TESTING=OFF" - "-C {{.G_DEPS_CMAKE_SETTINGS_DIR}}/boost.cmake" - "-DCMAKE_POLICY_DEFAULT_CMP0144=NEW" + JOBS: "{{.G_DEPS_MAX_PARALLELISM_PER_TASK}}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dep-tasks.yaml(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). (3)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
sitaowang1998
changed the title
ystdlib as a package to avoid duplicate ystdlib files.
sitaowang1998
changed the title
sitaowang1998
changed the title
add_subdirectory.
sitaowang1998
changed the title
add_subdirectory.add_subdirectory.
2 participants
Description
Both
clpandspideraddystdlibas subdirectory. When integratingspiderintoclp, compilation fails because duplicatedystdlibfiles.This PR updates
ystdlibversion to use the latest version that supports installystdlibas a package. This PR changes the dependency install tasks to installystdlibas a package.Because the updated
ystdlibrequiresCMakeminimum version 3.23, this PR also updates minimumCMakeversion required and updates GitHub workflows to installCMake3.23.5.This PR adds
POSITION_INDEPENDENT_CODEto the dependency build tasks becauseSpiderproduces shared libraryspider_client.so.Checklist
breaking change.
Validation performed
ystdlibinstall task works in dev container.ystdlibpackage in dev container.Summary by CodeRabbit