chore(ci) : expand lib targets - #50
Conversation
|
@LuciferYang @jja725 could you PTAL? Also, should we have a formatter for C/C++ like |
|
@LuciferYang the build is still failing. I will fix them and ping you once it is ready |
2087e62 to
946a7e0
Compare
|
@LuciferYang can you please take a look now? I have removed zig. Instead manylinux2_28 images are used for GLIBC 2.28 support. PR is a lot simpler now |
|
@LuciferYang gentle ping |
jja725
left a comment
There was a problem hiding this comment.
Heads up before anything else: I opened #53 yesterday doing a subset of what this does (Debian 12 container for a glibc 2.36 floor, release workflow only). I didn't check for open PRs first, which was sloppy of me. This one is older, goes lower (2.28), and covers CI and the smoke test too, so it supersedes mine. Happy to close #53 if this lands. They also touch the same lines in release.yml, so they can't both go in as-is.
Separately, this is showing as conflicting against main and hasn't been updated since early July, so it needs a rebase either way.
The glibc work itself is the right idea and I want it. Most of what's below is about the other four things that came along with it.
Things I'd want changed before merging
Pin the container images. quay.io/pypa/manylinux_2_28_x86_64 with no tag means latest, and pypa rebuilds those regularly. In CI that's mildly annoying. In release.yml it means the binaries we publish aren't reproducible, and an upstream image push can break a release with no change on our side. What's odd is that clang is pinned to 20.1.8.0 exactly, so the toolchain is nailed down but the image handing it to us isn't. A digest pin would settle it.
Why C++20? I went through lance.hpp and it includes <array> <cstdint> <memory> <optional> <stdexcept> <string> <utility> <vector>, with no concepts, ranges, <format>, <span>, coroutines, or <=> anywhere. As far as I can tell nothing in the public headers needs it. Meanwhile CMAKE_CXX_STANDARD 20 plus the bumps to the consumer example, the Conan test_package, and compile_and_run_test.rs push C++20 onto everyone downstream.
That sits awkwardly against the rest of the PR: we widen the glibc floor so more people can consume this, then narrow the language floor so fewer can. There's also a wrinkle specific to manylinux, whose docs note its clang still uses the libgcc/libstdc++ from the image, so the C++20 library surface isn't fully there anyway. If there's a concrete reason I'm missing I'm happy to be talked into it, but right now C++17 looks like it keeps more doors open.
The visibility presets don't do anything. CMAKE_C_VISIBILITY_PRESET, CMAKE_CXX_VISIBILITY_PRESET and CMAKE_VISIBILITY_INLINES_HIDDEN only apply to targets CMake compiles. liblance_c comes out of cargo via Corrosion, and everything else here is INTERFACE or IMPORTED, so no target in the project is affected. It reads like symbol hardening but changes nothing in the shipped library. If hiding symbols is the actual goal it has to happen on the Rust side.
Two with: blocks quietly disappear from release.yml: target: on setup-rust-toolchain and key: on rust-cache, neither mentioned in the description. The target one is probably harmless since every runner is native for its triple. The cache key I'm less sure about: both Linux rows are runner.os == Linux under the same job id, and I think rust-cache disambiguates on the rustc host triple, but I haven't verified that. Putting the key back costs nothing and removes the question.
Smaller things
.github/baseline.env is now a hard build dependency. baseline.cmake raises FATAL_ERROR if it's missing, on every platform, for a value only macOS uses. I checked and this won't break packaging today: both vcpkg_from_github and Conan's get() pull the GitHub source tarball, and there's no .gitattributes export-ignore, so .github/ ships. But a required build input living in a directory everyone reads as "CI stuff" is asking for someone to prune it later. cmake/baseline.env would say what it means. I'd also make the missing-file case non-fatal off macOS.
Related: that file now has two parsers with different rules. CI does set -a; source, CMake does a hand-rolled regex. Shell handles export FOO=bar, quoted values, ${VAR}, and trailing comments. The CMake side handles none of them. Write MACOSX_DEPLOYMENT_TARGET="11" and shell gives you 11 while CMake gives you "11", quotes included. That's a silent divergence in the one value the file exists to keep in sync. Either document the accepted format in the file itself, or generate one side from the other.
The protoc install block is copy-pasted six times (four jobs in ci.yml, plus the smoke test and release). A composite action under .github/actions/ would make the next version bump a one-line change.
Deleting the roadmap feels like its own PR. It's 50 lines of README in a change titled "expand lib targets", and #46 and #48 exist purely to tick boxes in it, so somebody clearly cares about keeping it current. Could it go in a collapsed <details> block instead of away entirely?
We now advertise "GLIBC baseline 2.28" in the README with nothing in CI checking it, so the first regression shows up on a user's machine rather than in a build. I had a go at this on my branch (readelf --dyn-syms on the .so, nm --undefined-only on the .a, take the highest GLIBC_ version, fail if it's above the floor) and can hand it over if it's useful.
Minor, but the rustfmt and license-header jobs now pull a multi-gigabyte manylinux image to run cargo fmt and a grep loop. Neither produces a binary, so that's pure wall-clock cost.
A few nits:
- the arch
casehas no default branch, so an unexpecteduname -mleavesPAempty and you get a 404 onprotoc-25.3-.zipinstead of a useful error - the protoc zip isn't checksummed, which is a little inconsistent given how precisely clang is pinned
manylinux-install-clangis marked BETA upstream, worth knowing we're leaning on it- the
Install Clang 20 (Linux)body is indented 12 spaces where its siblings use 10
Things I liked
The shasum/sha256sum fallback is a good catch. macOS has no sha256sum and manylinux has no shasum, so you genuinely need both arms and it's the kind of thing that only bites you at release time. Pinning protoc instead of taking whatever the distro ships is worth having on its own. timeout-minutes on publish closes a real gap. And keeping the deployment target in one place so CMake and CI can't drift is the right instinct, even if I'd move the file.
The motivation behind bumping C++ version is to utilise the above mentioned features and C++17 is almost a decade old. Even the new fundamental features like That said, since manylinux libstdc++ doesn't ship all the features, we can reduce it back to 17
I have only added it for posterity. Rust exports only the ones explicitly marked
official doc:
I think it uses job id for cache key
sure will do it in a separate PR
As long as we are using
will change to official GH images I will address rest of the comments in the next commit |
…54) Fixes the two failing `debian12` rows in the [v0.1.5 release run](https://github.com/lance-format/lance-c/actions/runs/31073039760). Regression from #53, which was mine. ## What broke Both `debian12` rows died at `Configure + build` about 14 minutes in, while the `ubuntu-24.04` rows succeeded: ``` error: failed to run custom build command for `lance-encoding v7.0.0-beta.7` Caused by: Error: Custom { kind: Other, error: "protoc failed: google/protobuf/empty.proto: File not found. encodings_v2_0.proto:8:1: Import \"google/protobuf/empty.proto\" was not found or had errors. encodings_v2_0.proto:343:5: \"google.protobuf.Empty\" is not defined." } ``` `protoc` installed fine. What was missing were the well-known type definitions under `/usr/include/google/protobuf/`, which `lance-encoding`'s build script imports. ## Root cause On Debian and Ubuntu those `.proto` files ship in `libprotobuf-dev`, and `protobuf-compiler` only *Recommends* it rather than depending on it. The `ubuntu-24.04` rows run a plain `apt-get install -y protobuf-compiler`, which honors Recommends and picks it up implicitly. The container step I added in #53 uses `--no-install-recommends`, which silently dropped it. Isolated by comparing all four combinations in real containers: | environment | `empty.proto` | import compiles | |---|---|---| | debian:12, `protobuf-compiler --no-install-recommends` | missing | **FAILS** | | debian:12, `protobuf-compiler` (recommends on) | present | OK | | debian:12, `protobuf-compiler libprotobuf-dev` | present | OK | | ubuntu:24.04, `protobuf-compiler` | present | OK | Row 1 reproduces the CI failure with the same two error strings. Rows 2 and 3 both resolve it. ## Why this fix Adding `libprotobuf-dev` explicitly rather than dropping `--no-install-recommends`. It names the actual dependency, and it keeps the other eleven packages from pulling in their own recommends. ## Test plan The controlled comparison above was run in real `debian:12` and `ubuntu:24.04` containers. `actionlint` reports no new findings (the two SC2035/SC2129 hits in the publish job predate #53). Full end-to-end proof is the release workflow itself. Worth re-dispatching `Release` at `v0.1.5` once this lands, before cutting anything new. ## Note for #50 If #50 lands, this becomes moot for the same reason: its protoc install extracts `include/*` from the upstream release zip, which is exactly the well-known types. That approach sidesteps the Recommends trap entirely and is arguably the more robust one. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The manylinux direction fixes the demonstrated root cause: the existing v0.1.2 Linux artifact reaches GLIBC_2.39. The remaining blockers share two release contracts: shipped artifacts must prove their advertised runtime floors, and executable inputs to a write-capable release job must be immutable and integrity-checked.
A viable revision would use digest-pinned build roots, restrict the build job to read-only repository access, verify downloaded tools, and gate upload on Linux/Mach-O artifact plus packaged static/shared consumer checks.
Please mark this PR with the breaking-change label.
| runner: ubuntu-24.04-arm | ||
| os_label: linux | ||
| - os: ubuntu-24.04 | ||
| container: quay.io/pypa/manylinux_2_28_x86_64 |
There was a problem hiding this comment.
These release builders are untagged references, so they resolve a mutable latest. An upstream rebuild can silently change the toolchain, platform guarantee, or shipped bytes. Because the workflow grants contents: write, this also gives mutable third-party code unnecessary release authority. Pin both images by digest and give the build job contents: read, reserving write permission for publishing.
| run: | | ||
| ARCH=$(uname -m) | ||
| case "${ARCH}" in x86_64) PA="linux-x86_64" ;; aarch64) PA="linux-aarch_64" ;; esac | ||
| curl -fsSL "https://github.com/protocolbuffers/protobuf/releases/download/v25.3/protoc-25.3-${PA}.zip" -o /tmp/protoc.zip |
There was a problem hiding this comment.
v25.3 identifies a release but does not authenticate this executable. A replaced or corrupted asset is installed into /usr/local and executed during the release build. Verify a fixed SHA-256 for each architecture before installing it.
| @@ -74,8 +87,13 @@ jobs: | |||
| TARGET=${{ matrix.target }} | |||
| ARCHIVE="lance-c-v${VERSION}-${TARGET}.tar.xz" | |||
| tar -C stage -cJf "${ARCHIVE}" . | |||
There was a problem hiding this comment.
The compatibility claim is not asserted on the files packaged here, so a toolchain or dependency regression can still ship an incompatible release. I inspected v0.1.2 and its x86_64 .so requires symbols through GLIBC_2.39; this environment could not execute the proposed container, leaving the current-head ceiling unverified. Before upload, reject Linux symbols above 2.28 and unexpected dependencies, inspect macOS minos, and link/run packaged static and shared consumers for both Linux architectures.
|
Occupied with something else. Will get back to it by EOW or next week |
11.0