Skip to content

build!: Install ystdlib as a package to avoid duplicate ystdlib files. #1038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sitaowang1998 wants to merge 51 commits into from
Closed

build!: Install ystdlib as a package to avoid duplicate ystdlib files. #1038

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
1cc7e76
Install ystdlib as package
sitaowang1998 Jun 26, 2025
8e0e4d5
Bump cmake min version to 3.23
sitaowang1998 Jun 26, 2025
aba106f
Install cmake 3.23.5 for ubuntu jammy
sitaowang1998 Jun 26, 2025
917fd86
Merge branch 'main' into ystdlib-package
sitaowang1998 Jun 26, 2025
9a2c19e
Merge branch 'main' into ystdlib-package
sitaowang1998 Jun 26, 2025
71e96ac
Remove unnecessary cmake variable
sitaowang1998 Jun 26, 2025
430f91c
Use homebrew llvm in macos GH runner
sitaowang1998 Jun 26, 2025
2f1b08d
Revert "Use homebrew llvm in macos GH runner"
sitaowang1998 Jun 26, 2025
f324686
Temporarily remove other workflows
sitaowang1998 Jun 26, 2025
a51b899
Add setting of environment variables
sitaowang1998 Jun 26, 2025
04cc82a
Revert "Temporarily remove other workflows"
sitaowang1998 Jun 26, 2025
0e29499
Bug fix
sitaowang1998 Jun 26, 2025
cfad209
Temporarily remove other GH workflows
sitaowang1998 Jun 26, 2025
753e555
Add check on PATH variable
sitaowang1998 Jun 26, 2025
15f3097
Revert "Add check on PATH variable"
sitaowang1998 Jun 27, 2025
55516f5
Use llvm
sitaowang1998 Jun 27, 2025
2f7963d
Bug fix
sitaowang1998 Jun 27, 2025
0679303
Add llvm library
sitaowang1998 Jun 27, 2025
71c0a05
Remove ld flags
sitaowang1998 Jun 27, 2025
7d534bf
Revert "Temporarily remove other GH workflows"
sitaowang1998 Jun 27, 2025
072482f
Merge branch 'main' into macos-llvm
sitaowang1998 Jun 27, 2025
cf2c37b
Experiment with extra clang-tidy flags
sitaowang1998 Jun 27, 2025
fa5dfc2
Temporarily remove other GH workflows
sitaowang1998 Jun 27, 2025
e2434cb
Revert "Experiment with extra clang-tidy flags"
sitaowang1998 Jun 27, 2025
39961b4
Add more environment flags
sitaowang1998 Jun 27, 2025
5e9d8c6
Experiment with environment variables
sitaowang1998 Jun 27, 2025
9478ae8
Add more experiment with environment variables AR and RANLIB
sitaowang1998 Jun 27, 2025
fca4857
Remove environment flags for flags
sitaowang1998 Jun 27, 2025
67d2363
Experiment with clang-tidy argument
sitaowang1998 Jun 27, 2025
2d82296
Try add llvm to clang-tidy sysroot
sitaowang1998 Jun 28, 2025
260b69d
Revert "Try add llvm to clang-tidy sysroot"
sitaowang1998 Jun 28, 2025
c91dc84
Not use llvm toolchain in clp
sitaowang1998 Jun 28, 2025
693e2d0
Revert "Not use llvm toolchain in clp"
sitaowang1998 Jun 28, 2025
234a1f3
Experiment with extra args
sitaowang1998 Jun 28, 2025
99cdacc
Remove errno_t=int from clang-tidy flags
sitaowang1998 Jun 28, 2025
0f1034a
Bug fix
sitaowang1998 Jun 28, 2025
db7cd48
Revert "Temporarily remove other GH workflows"
sitaowang1998 Jun 28, 2025
9e2c951
Add rsize clang-tidy flag in all clang-tidy tasks
sitaowang1998 Jun 28, 2025
7e528b4
Merge branch 'main' into macos-llvm
sitaowang1998 Jun 28, 2025
aadd28c
Merge branch 'main' into ystdlib-package
sitaowang1998 Jun 30, 2025
d80898d
Merge branch 'main' into macos-llvm
sitaowang1998 Jun 30, 2025
5fd31da
Merge branch 'macos-llvm' into ystdlib-package
sitaowang1998 Jun 30, 2025
ea5813a
Remove unnecessary if guard in cmake
sitaowang1998 Jul 7, 2025
453a9c7
Improve comments in install script
sitaowang1998 Jul 7, 2025
c923e72
Merge branch 'main' into ystdlib-package
sitaowang1998 Jul 7, 2025
18ddf6c
Remove fix for macOS
sitaowang1998 Jul 9, 2025
be467e8
Disable ci on macOS 13 and 14
sitaowang1998 Jul 9, 2025
c2501df
Improve sequence and comment in install script
sitaowang1998 Jul 10, 2025
dcb3e28
Remove PIC flag
sitaowang1998 Jul 10, 2025
06527be
Merge branch 'main' into ystdlib-package
sitaowang1998 Jul 10, 2025
eed0d81
Add default flags to ystdlib build
sitaowang1998 Jul 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions components/core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.21)
cmake_minimum_required(VERSION 3.23)

# Toolchain setup must come before the first project() call in the entire CMake buildsystem.
# If CLP is not the top-level project, the following setup has no effect.
Expand Down Expand Up @@ -275,9 +275,10 @@ if(CLP_NEED_YAMLCPP)
endif()

# Add ystdlib
if(CLP_NEED_YSTDLIB)
set(YSTDLIB_CPP_BUILD_TESTING OFF)
add_subdirectory("${CLP_YSTDLIB_SOURCE_DIRECTORY}" "${CMAKE_BINARY_DIR}/ystdlib" EXCLUDE_FROM_ALL)
set(ystdlib_BUILD_TESTING OFF)
find_package(ystdlib REQUIRED)
if (ystdlib_FOUND)
message(STATUS "Found ystdlib ${ystdlib_VERSION}")
endif()

@coderabbitai coderabbitai Bot Jun 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Unconditionally requiring ystdlib may break consumer projects

All other third-party libraries are gated behind CLP_NEED_* flags, but ystdlib is now unconditional. Projects embedding clp as a sub-project and disabling unused dependencies will be forced to provide ystdlib even if they don’t need it.

-# Add ystdlib
-set(ystdlib_BUILD_TESTING OFF)
-find_package(ystdlib REQUIRED)
+# Add ystdlib (guarded like other optional deps)
+if(CLP_NEED_YSTDLIB)
+    set(ystdlib_BUILD_TESTING OFF)  # Effective only when ystdlib is added via add_subdirectory
+    find_package(ystdlib REQUIRED)
+    if(ystdlib_FOUND)
+        message(STATUS "Found ystdlib ${ystdlib_VERSION}")
+    endif()
+endif()

Also note that set(ystdlib_BUILD_TESTING OFF) has no effect when you consume a pre-built package; the option only matters when you build ystdlib from source. Passing the flag via CMake configure in the dependency task (which you already do) is sufficient.

🤖 Prompt for AI Agents 
In components/core/CMakeLists.txt around lines 277 to 282, the ystdlib
dependency is required unconditionally, unlike other third-party libraries gated
by CLP_NEED_* flags. Modify the CMakeLists.txt to conditionally
find_package(ystdlib) only if a corresponding flag (e.g., CLP_NEED_YSTDLIB) is
set, aligning with the pattern used for other dependencies. Also, remove the
set(ystdlib_BUILD_TESTING OFF) line here since it has no effect when consuming a
pre-built package and is already handled during ystdlib's build configuration.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Comment thread
sitaowang1998 marked this conversation as resolved.
Outdated

# Find and setup ZStd Library
Expand Down
4 changes: 3 additions & 1 deletion components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ set -u
script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
lib_install_scripts_dir=$script_dir/..

# NOTE: boost must be installed first since the remaining packages depend on it
# NOTE: cmake must be installed first since the remaining packages depend on it to build
"$lib_install_scripts_dir"/install-cmake.sh 3.23.5
# NOTE: boost must be installed second since the remaining packages depend on it
Comment thread
sitaowang1998 marked this conversation as resolved.
Outdated
"$lib_install_scripts_dir"/install-boost.sh 1.87.0

@coderabbitai coderabbitai Bot Jun 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add set -o pipefail to harden the script

set -e and set -u are great, but without set -o pipefail, a failing command in a pipeline may go unnoticed, which could leave an unusable tool-chain silently installed.

  # Exit on any error
  set -e
+ # Fail the whole pipeline if any element fails
+ set -o pipefail

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In
components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh
around lines 12 to 15, the script uses set -e and set -u but lacks set -o
pipefail, which can cause failures in pipelines to be ignored. Add set -o
pipefail near the top of the script along with the existing set commands to
ensure any failure in a pipeline causes the script to exit immediately,
improving robustness.


"$lib_install_scripts_dir"/libarchive.sh 3.5.1
Expand Down
1 change: 0 additions & 1 deletion components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ apt-get update
DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \
ca-certificates \
checkinstall \
cmake \
curl \
build-essential \
git \
Expand Down
23 changes: 9 additions & 14 deletions taskfiles/deps/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,20 +374,15 @@ tasks:

ystdlib:
internal: true
vars:
LIB_NAME: "ystdlib"
YSTDLIB_OUTPUT_DIR: "{{.G_DEPS_CORE_DIR}}/{{.LIB_NAME}}-src"
run: "once"
deps:
- task: "utils:init"
cmds:
- task: "yscope-dev-utils:remote:download-and-extract-tar"
- task: "utils:install-remote-cmake-lib"
vars:
CHECKSUM_FILE: "{{.G_DEPS_CORE_CHECKSUMS_DIR}}/{{.LIB_NAME}}.md5"
FILE_SHA256: "36fa0e9d96b7307ca92482343d6ba1091c5576370676e6d423cce32c20e34a3d"
OUTPUT_DIR: "{{.YSTDLIB_OUTPUT_DIR}}"
URL: "https://github.com/y-scope/ystdlib-cpp/archive/d80cf86.tar.gz"
- >-
echo "set(
CLP_YSTDLIB_SOURCE_DIRECTORY \"{{.YSTDLIB_OUTPUT_DIR}}\"
)" > "{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake"
CMAKE_GEN_ARGS:
- "-DCMAKE_POSITION_INDEPENDENT_CODE=ON"
- "-Dystdlib_BUILD_TESTING=OFF"
- "-DCMAKE_POLICY_DEFAULT_CMP0144=NEW"
Comment thread
sitaowang1998 marked this conversation as resolved.
LIB_NAME: "ystdlib"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
WORK_DIR: "{{.G_DEPS_DIR}}/ystdlib"
TARBALL_SHA256: "4c027c884506e1775070192f9d1d58238a6d5b078608211fa442477393676738"
TARBALL_URL: "https://github.com/y-scope/ystdlib-cpp/archive/0ae886c.tar.gz"