Skip to content

ci/eval: fix local comparison with baseline#441746

Merged
wolfgangwalther merged 1 commit intoNixOS:masterfrom
wolfgangwalther:ci-eval-baseline
Sep 10, 2025
Merged

ci/eval: fix local comparison with baseline#441746
wolfgangwalther merged 1 commit intoNixOS:masterfrom
wolfgangwalther:ci-eval-baseline

Conversation

@wolfgangwalther
Copy link
Copy Markdown
Contributor

@wolfgangwalther wolfgangwalther commented Sep 10, 2025

Due to how we pass in existing store paths via CLI arguments for the diff and combine scripts, Nix didn't register a dependency on the store paths properly. This meant that some of the derivations that were built, didn't have the right store paths made available in the sandbox - leading to all kinds of "not found" errors.

We worked around this in CI by resolving the symlinks to the nix store beforehand. We tried to work around this locally by storing the nix store path in BASELINE, but this didn't fully work. By explicitly registering these store paths as dependencies, this should work across the board - without any magic required by the caller.

Follow up to #440895.

The error I would be getting would be a "file not found" in the python script to compare stats.

Things done


Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions backport release-25.05 labels Sep 10, 2025
@wolfgangwalther wolfgangwalther marked this pull request as draft September 10, 2025 12:02
@wolfgangwalther
Copy link
Copy Markdown
Contributor Author

Seems like this still doesn't work. It worked when I ran it on the same commit, but now it doesn't when I compare two different commits.

@wolfgangwalther
Copy link
Copy Markdown
Contributor Author

OK, TIL about builtins.storePath. Rewrote the whole PR description (and the fix) - my previous approach was not even close.

This should now work correctly - and also remove the odd readlink workarounds we had in CI. Tested locally, let's see what CI says.

@wolfgangwalther wolfgangwalther marked this pull request as ready for review September 10, 2025 12:34
@nixpkgs-ci nixpkgs-ci bot added the 6.topic: policy discussion Discuss policies to work in and around Nixpkgs label Sep 10, 2025
Due to how we pass in existing store paths via CLI arguments for the
diff and combine scripts, Nix didn't register a dependency on the store
paths properly. This meant that some of the derivations that were built,
didn't have the right store paths made available in the sandbox -
leading to all kinds of "not found" errors.

We worked around this in CI by resolving the symlinks to the nix store
beforehand. We tried to work around this locally by storing the nix
store path in BASELINE, but this didn't fully work. By explicitly
registering these store paths as dependencies, this should work across
the board - without any magic required by the caller.
Copy link
Copy Markdown
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

looks simpler.

@wolfgangwalther
Copy link
Copy Markdown
Contributor Author

Not sure whether it works 100% correctly, yet. The test workflow has this in its summary:

⚠️ Skipping comparison: stats directory is missing in the target commit.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 10, 2025
Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

I'm sure you're already aware of the different behaviour of path concatenation, interpolation, etc...

But just for my own understanding, and to try and understand the motivation behind the new/old impl, I'll ask the stupid questions anyway:

@wolfgangwalther
Copy link
Copy Markdown
Contributor Author

I'm sure you're already aware of the different behaviour of path concatenation, interpolation, etc...

Let me assure you, that this is not the case :). This is all a big mystery to me!

@wolfgangwalther
Copy link
Copy Markdown
Contributor Author

Not sure whether it works 100% correctly, yet. The test workflow has this in its summary:

⚠️ Skipping comparison: stats directory is missing in the target commit.

Ah. that's a cross-over between target branch and this PR: The eval "compare" step runs as nix-build nixpkgs/trusted/ci .... That means it uses the eval.compare code from the target branch, where the storepath change is not included. That's why the change to combinedDir doesn't take effect and the script can't find the stats folder.

I confirmed this works correctly when repeating these steps that CI does locally. Will be good after merge.

Copy link
Copy Markdown
Contributor

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

Another nice improvement! Thanks again @wolfgangwalther, and thanks for entertaining my belligerent questioning 😅

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Sep 10, 2025
@wolfgangwalther wolfgangwalther added this pull request to the merge queue Sep 10, 2025
Merged via the queue into NixOS:master with commit 3d44f60 Sep 10, 2025
57 of 60 checks passed
@wolfgangwalther wolfgangwalther deleted the ci-eval-baseline branch September 10, 2025 17:28
@nixpkgs-ci
Copy link
Copy Markdown
Contributor

nixpkgs-ci bot commented Sep 10, 2025

Successfully created backport PR for release-25.05:

@github-actions github-actions bot added the 8.has: port to stable This PR already has a backport to the stable release. label Sep 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 6.topic: policy discussion Discuss policies to work in and around Nixpkgs 8.has: port to stable This PR already has a backport to the stable release. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants