Packaging and workflows improvements#218
Merged
Merged
Conversation
actions/checkout@v4 already pins HEAD to github.sha by default
(it sets ref and commit from the event context across push,
pull_request, and workflow_dispatch). The explicit pin in
package-test-nix.yml and build.yml's nixos job was dead weight;
package-test-nix.yml's `?rev=${{ github.sha }}` override still
resolves since github.sha is at HEAD either way.
Assisted-by: Claude:claude-opus-4-7
Switch from the vendored nix/catch2_3.nix to nixpkgs' upstream catch2_3, overridden with the consumer's stdenv. The override is the important: Catch2 ships a compiled archive, so its stdlib ABI (libstdc++ vs libc++) must match libfn's. Rebuilding catch2_3 against the same stdenv which libfn is built with keeps clang + libc++ working. Assisted-by: Claude:claude-opus-4-7
libstdc++'s std::expected<void, E> has an _M_unex subobject whose initialization clang's constexpr evaluator can't track when transitioning from error to value state (works on libc++, on gcc, and on newer clang). Four asserts in the void specialization's assign-from-rvalue, assign-from- const-lvalue, emplace, and swap paths trip it. Gate just those statements (and at the emplace site the lambda too, since gating its only use would leave it -Wunused-variable -Werror'd). gcc+libstdc++ and clang+libc++ coverage of these asserts is preserved. Assisted-by: Claude:claude-opus-4-7
The clang-libstdcxx images (clang built against libstdc++ rather than libc++) have existed in ci-build.yml for both :21 and :22 since #214, but were not exercised in build.yml. Now that the constexpr gates for clang+libstdc++ have landed in tests/pfn/expected.cpp, the lane builds and tests clean — add both image versions to the matrix to keep that coverage honest on every PR. Assisted-by: Claude:claude-opus-4-7
Each used to run three test configurations sequentially in one job (cxx23, cxx20, cxx20-std23), and vcpkg additionally needed a `vcpkg remove` step between two of them. Replace with a matrix of mode-named entries (one per configuration); each job installs fresh and runs one mode, dropping the inter-step cleanup. For vcpkg's cxx23 mode the fn_quine binary is run via a separate step gated by `if: matrix.name == 'cxx23'`. Also expand single line `run: nix ...` into multiline `run: |`, for improved readability. Assisted-by: Claude:claude-opus-4-7
The linux and macos jobs each had a "Build and test all" step that ran only for one chosen compiler+config combination, piggy-backed on that combo's mode-specific build via `cmake --build . --target clean` to undo it first. Replace with an `include` entry that gives "all" its own matrix slot; gate the two test steps with `if: matrix.mode != 'all'` and `if: matrix.mode == 'all'` respectively. The clean+rebuild dance goes away, and the prior gcc:14 cxx23 (linux) or appleclang 15 cxx23 (macos) jobs no longer do double duty. Assisted-by: Claude:claude-opus-4-7
Capture the four conventions we landed via the recent packaging- improvements PR: don't pin `ref:` on actions/checkout@v4 without reason, matrix-ify install/build/test variations instead of running them sequentially in one job, give the "build everything" run its own matrix slot (no clean+rebuild dance), and use literal-block `run: |` with shell `\` continuation for multi-line commands. Assisted-by: Claude:claude-opus-4-7
GitHub will flip windows-2025's default from VS 2022 to VS 2026 during the 2026-06-08..15 migration, so the previous `windows-2025` + "Visual Studio 17 2022" pairing would have silently broken on June 15. Restructure the windows job around a `vs` axis pairing generator with runner: VS 2022 → windows-2022 (long-term LTS), VS 2026 → windows-2025-vs2026 (early-access label today; will alias windows-2025 post-migration, no follow-up edit needed). Mode stays cxx20-only on Windows; enabling fn / C++23 on VS 2026 is a separate near-term PR. Assisted-by: Claude:claude-opus-4-7
libfn is header-only (`package_type = "header-library"`) and conan's
`settings` (`os`, `arch`, `compiler`, `build_type`) only ever fed into
`check_min_cppstd` inside `validate()` — and the binary identity was
already cleared via `self.info.clear()` in `package_id()`. Drop both
the `settings` declaration and the now-unreachable `validate()` method
(plus the `check_min_cppstd` import). The C++20/23 minimums are still
enforced by libfn's CMake side via `target_compile_features(...
cxx_std_{20,23})` on `include_pfn`/`include_fn`, so a too-old consumer
compiler fails at CMake configure rather than at `conan install` — a
few seconds later but with an equally clear error.
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-7
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🤖 Augment PR SummarySummary: This PR tightens packaging verification and CI supply-chain hygiene by pinning GitHub Actions, expanding build matrices, and parameterizing CI images. Changes:
Technical Notes: Conan’s early C++ standard validation is removed for the header-only package; the enforced language version now comes from the CMake configuration. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
Member
Author
|
augment review |
| constexpr T a = fn(T(unexpect, Error::file_not_found)); | ||
| static_assert(a.error() == Error::file_not_found); | ||
|
|
||
| #if !(defined(__clang__) && defined(__GLIBCXX__) && defined(PFN_TEST_VALIDATION)) |
There was a problem hiding this comment.
tests/pfn/expected.cpp:4187 — This new #if !(defined(__clang__) && defined(__GLIBCXX__) && defined(PFN_TEST_VALIDATION)) skip looks like a compiler/stdlib-specific workaround; consider adding a brief comment (or issue link) explaining the underlying failure so it’s clear why these constexpr assertions are omitted on that configuration.
Severity: low
Other Locations
tests/pfn/expected.cpp:4481tests/pfn/expected.cpp:4549tests/pfn/expected.cpp:4838
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Assisted-by: Claude:claude-opus-4-7
ENV declarations moved past intervening RUNs that don't depend on them, so local rebuilds with different ENV values reuse cached layers. ci-docs also exposes DEBIAN_VERSION and JAVA_VERSION as build args — it was the only image with hard-coded Debian release because the JDK package name ties it to a specific Debian. Assisted-by: Claude:claude-opus-4-7
Member
Author
|
augment review |
| matrix: | ||
| include: | ||
| - name: cxx23 | ||
| options: '-o "&:disable_cxx23=False" -s "compiler.cppstd=23"' |
There was a problem hiding this comment.
matrix.options includes embedded shell quotes (e.g. -s "compiler.cppstd=23"), but when injected via ${{ matrix.options }} those quotes won’t be re-parsed by bash, so Conan can receive literal-quote arguments and fail to parse settings/options.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-7
Assisted-by: Claude:claude-opus-4-7
Member
Author
|
augment review |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
buildworkflow (Clang version 21 and 22 only), with workaround intests/pfn/expected.cppfor validation mode onlypackage-test-*workflows to use matrix rather than stepszizmorandactionlintfor improved workflow securitycatch2_3dependency fromnixpre-commitDocker image frombookwormtotrixieCONTRIBUTING.md