Merged
Conversation
#5826 fixed the cache paths but #5831 reverted it because the cache hit on main started failing test-elixir: the Rustler-built NIF lives at prqlc/bindings/elixir/priv/native/prql.so, outside _build, so the cache restored beam files without the .so, mix compile saw the project as up-to-date and skipped Rustler, and tests failed to load the NIF. Changes: - Restore the prqlc/bindings/elixir/{deps,_build} paths from #5826 - Add prqlc/bindings/elixir/priv to the build cache so the .so is preserved across runs - Bump cache-name to invalidate the stale 288KB entry from #5826 - Hash Cargo.lock and prqlc Rust sources in the build key so the .so is rebuilt whenever the Rust code that produces it changes - Drop restore-keys on the build cache; a partial-prefix restore would deliver a stale .so on Rust changes
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
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.
Problem
#5826 corrected the
actions/cachepaths intest-elixir.yaml(which had been wrong since the bindings reorg in #3683 — the cache silently never restored). Once those paths were correct, the next nightly onmain(run 24955057135) hit the saved cache and test-elixir failed:That run-failure is what motivated the revert in #5831.
Root cause
Rustler builds the NIF via cargo into the workspace
target/and then copies it toprqlc/bindings/elixir/priv/native/prql.so(see job 73060377548 loggingCopying /home/runner/work/prql/prql/target/release/libprql.so to priv/native/prql.so)._build/<env>/lib/prql/privis just a symlink back to thatpriv/— the actual.solives outside_build. The saved cache from #5826 was only ~288KB (beam files only, no NIF).The elixir module uses
use Rustler, otp_app: :prql(a runtime-loaded NIF). When the cache restored beam files, mix saw the project as already compiled and skipped Rustler entirely, so the.sowas never rebuilt andmix testfailed to load the NIF.Fix
prqlc/bindings/elixir/deps,prqlc/bindings/elixir/_build).prqlc/bindings/elixir/privto the build cache so the.sosurvives across runs.cache-nametocache-compiled-build-v2to invalidate the stale 288KB entry that was saved during ci: correct test-elixir cache paths #5826.Cargo.lockandprqlc/**/*.rs(plusCargo.toml) in the build cache key so cache invalidation tracks any change that affects the.so. Mix's beam staleness check doesn't see Rust source changes, so without this the cache would happily serve a stale.sowhenever Rust code changed butmix.lockdidn't.restore-keyson the build cache. With a comprehensive key, any miss means something the.sodepends on changed; a partial-prefix restore would deliver a stale.so. The deps cache keeps itscache-name-prefix restore-key — partial dep updates can reuse most deps.Trade-offs vs. just reverting
prqlc/invalidates the build cache → ~2 min Rust rebuild on those PRs. But that's the correctness floor — anything cheaper would risk stale.sotest passes.Verification
main) should show a cache hit and skip the Rust build.Per prql-bot comment on #5831 and @max-sixty's request.