Skip to content

build(deps): Add task-based installation for zlib; Add missing liblzma dependency docs. #1229

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

Merged
merged 11 commits into from
Aug 20, 2025
Merged

build(deps): Add task-based installation for zlib; Add missing liblzma dependency docs. #1229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
96e93ee
add zlib taskfile
Bill-hbrhbr Aug 18, 2025
020151c
Alphabetcize
Bill-hbrhbr Aug 18, 2025
add4e47
Merge branch 'main' into modernize-zlib-install
Bill-hbrhbr Aug 18, 2025
2f4505f
Merge branch 'main' into modernize-zlib-install
Bill-hbrhbr Aug 19, 2025
4bb18da
Remove unnecessary cmake options
Bill-hbrhbr Aug 19, 2025
875bdb0
Merge branch 'main' into modernize-zlib-install
Bill-hbrhbr Aug 19, 2025
1e4d626
Add back cmake lazy install message
Bill-hbrhbr Aug 19, 2025
e0fb139
Update docs
Bill-hbrhbr Aug 19, 2025
f6e5276
Update taskfiles/deps/main.yaml
Bill-hbrhbr Aug 19, 2025
be0b788
Merge branch 'main' into modernize-zlib-install
Bill-hbrhbr Aug 19, 2025
378db7e
Merge branch 'main' into modernize-zlib-install
Bill-hbrhbr Aug 20, 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
2 changes: 2 additions & 0 deletions docs/src/dev-guide/components-core/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ The task will download, build, and install (within the build directory) the foll
| [fmt](https://github.com/fmtlib/fmt) | v10.2.1 |
| [json](https://github.com/nlohmann/json.git) | v3.11.3 |
| [log-surgeon](https://github.com/y-scope/log-surgeon) | f801a3f |
| [liblzma](https://github.com/tukaani-project/xz) | v5.8.1 |
| [lz4](https://github.com/lz4/lz4) | v1.10.0 |
| [mongo-cxx-driver](https://github.com/mongodb/mongo-cxx-driver) | r3.10.2 |
| [simdjson](https://github.com/simdjson/simdjson) | v3.13.0 |
Expand All @@ -51,6 +52,7 @@ The task will download, build, and install (within the build directory) the foll
| [yaml-cpp](https://github.com/jbeder/yaml-cpp.git) | v0.7.0 |
| [yscope-log-viewer](https://github.com/y-scope/yscope-log-viewer.git) | 969ff35 |
| [ystdlib-cpp](https://github.com/y-scope/ystdlib-cpp.git) | d80cf86 |
| [zlib](https://github.com/madler/zlib) | v1.3.1 |
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

zlib dependency entry looks good; verify docs ↔ taskfile version alignment

The zlib row matches the new task added in taskfiles/deps/main.yaml (v1.3.1). Let’s assert the docs version matches the pinned tarball in the Taskfile and that zlib is wired into deps:core.

Run this script from the repo root:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Checking zlib task presence and wiring..."
# Show LIB_NAME, TARBALL_URL, and any zlib task references
rg -n -C3 -P '(?m)^\s*LIB_NAME:\s*"zlib"|TARBALL_URL:\s*.*zlib-[0-9]+\.[0-9]+\.[0-9]+\.tar\.gz|task:\s*"zlib"' taskfiles/deps/main.yaml || true

echo
echo "Checking zlib appears in core-all-parallel..."
rg -nP -C3 '(?s)core-all-parallel:.*?-\s*task:\s*"zlib"' taskfiles/deps/main.yaml || { echo "zlib not found in core-all-parallel"; exit 1; }

echo
echo "Comparing docs zlib version with taskfile tarball version..."
doc_ver="$(rg -nPo '\|\s*\[zlib\]\([^)]+\)\s*\|\s*v([0-9]+\.[0-9]+\.[0-9]+)\s*\|' docs/src/dev-guide/components-core/index.md -r '$1' | head -n1 || true)"
task_ver="$(rg -nPo 'zlib-([0-9]+\.[0-9]+\.[0-9]+)\.tar\.gz' taskfiles/deps/main.yaml -r '$1' | head -n1 || true)"
printf "Docs version: %s\nTaskfile version: %s\n" "${doc_ver:-<none>}" "${task_ver:-<none>}"

if [[ -z "${doc_ver}" || -z "${task_ver}" ]]; then
  echo "Unable to determine one or both versions. Inspect the patterns or files." >&2
  exit 1
fi

if [[ "$doc_ver" != "$task_ver" ]]; then
  echo "Mismatch: update docs or Taskfile to keep versions in sync." >&2
  exit 1
fi

echo "OK: versions match and zlib is wired into deps:core."

Length of output: 1152

zlib wiring missing in core-all-parallel

The docs entry and Taskfile version both pin zlib to v1.3.1, but taskfiles/deps/main.yaml does not include zlib under the core-all-parallel group. Please add it so zlib is built as part of the core dependencies:

core-all-parallel:
  # …existing entries…
  - task: "zlib"

File to update:

  • taskfiles/deps/main.yaml (under the core-all-parallel: section)
🤖 Prompt for AI Agents 
taskfiles/deps/main.yaml (in the core-all-parallel: section) — The zlib task is
missing from the core-all-parallel group even though docs and Taskfile pin
v1.3.1; add an entry for zlib under the core-all-parallel list (e.g., add a new
list item for the zlib task with the same indentation as the other tasks: -
task: "zlib"), save the file, and run/verify the taskfile linter or a dry run to
ensure YAML indentation and formatting are correct.

Copy link
Contributor Author

@Bill-hbrhbr Bill-hbrhbr Aug 19, 2025

Choose a reason for hiding this comment

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

I think it's not missing.

Copy link
Contributor

@coderabbitai coderabbitai bot Aug 19, 2025

Choose a reason for hiding this comment

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

🧩 Analysis chain

Let me verify the current state of the taskfiles to see the complete core-all-parallel task definition.

@Bill-hbrhbr, you're absolutely right! I apologize for the confusion. Looking at the taskfile more carefully, I can see that zlib is indeed properly included in the core-all-parallel task. My initial analysis was incorrect - the zlib task is properly wired into the core dependencies as expected. The documentation entry for zlib v1.3.1 correctly reflects the dependency that's already configured in the taskfile.

Thank you for the correction!

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Showing complete core-all-parallel task definition ==="
# Extract the entire core-all-parallel task block
sed -n '/^[[:space:]]*core-all-parallel:/,/^[[:space:]]*[^[:space:]-]/p' taskfiles/deps/main.yaml | head -n -1

echo
echo "=== Searching for all zlib references in the taskfile ==="
rg -n "zlib" taskfiles/deps/main.yaml

echo
echo "=== Showing context around core-all-parallel ==="
rg -n -A 20 "core-all-parallel:" taskfiles/deps/main.yaml

Length of output: 1337

@Bill-hbrhbr, you are absolutely correct, and I apologize for my error! The script output clearly shows that zlib is properly included in the core-all-parallel task at line 81:

- task: "zlib"

The zlib dependency is correctly wired into the core dependencies, and the documentation entry for zlib v1.3.1 accurately reflects what's configured in the taskfile. My initial analysis was flawed - thank you for pointing this out!

The zlib task-based installation is properly set up and integrated into the build system as intended for this PR.

🧠 Learnings used 
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.

Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.

| [zstd](https://github.com/facebook/zstd) | v1.5.7 |

### Environment
Expand Down
19 changes: 19 additions & 0 deletions taskfiles/deps/main.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ tasks:
- task: "utfcpp"
- task: "yaml-cpp"
- task: "ystdlib"
- task: "zlib"
- task: "zstd"

absl:
Expand Down Expand Up @@ -484,6 +485,24 @@ tasks:
CLP_YSTDLIB_SOURCE_DIRECTORY \"{{.YSTDLIB_OUTPUT_DIR}}\"
)" > "{{.G_DEPS_CORE_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake"

zlib:
internal: true
vars:
LIB_NAME: "zlib"
run: "once"
cmds:
- task: "utils:install-remote-cmake-lib"
vars:
CMAKE_GEN_ARGS:
- "-DBUILD_SHARED_LIBS=ON"
- "-DCMAKE_BUILD_TYPE=Release"
- "-DCMAKE_INSTALL_MESSAGE=LAZY"
- "-DCMAKE_INSTALL_PREFIX={{.G_DEPS_CORE_DIR}}/{{.LIB_NAME}}-install"
- "-DZLIB_BUILD_EXAMPLES=OFF"
LIB_NAME: "{{.LIB_NAME}}"
TARBALL_SHA256: "9a93b2b7dfdac77ceba5a558a580e74667dd6fede4585b91eefb60f03b72df23"
TARBALL_URL: "https://github.com/madler/zlib/releases/download/v1.3.1/zlib-1.3.1.tar.gz"

zstd:
internal: true
run: "once"
Expand Down
Loading