Alright so average per system is ~6m40s with the actually correct check: https://github.com/winterqt/nixpkgs/actions/runs/15007089150

IMO the ~doubled eval time here is worth it for peace of mind that we’re catching issues upfront, but it does make it important to replace the existing eval job entirely so that we’re not trashing our concurrency by doubling the number of jobs and having half of them take twice as long.

The get-merge-commit workflow has increased massively in complexity. I feel like this took a wrong turn after the latest discovery, that different workflows for a single event are not guaranteed to run on the same merge commit (which still puzzles me).

Knowing this, we should just not run those in different workflows anymore. But I think with #406825 (comment), we might not need to anyway. I have thought about this a bit more, and it's quite simple in theory:

• We know that outpaths on master belong to a state that passes the purity test.
• If any path-to-nixpkgs dependency sneaks in, an outpath will change (this is what the purity test is based on).
• Thus, this will cause an added or changed attribute after comparison.
• It's therefore enough to check those added/changed attributes against a different nixpkgs path. There is no way they can turn bad without changing.

For the big majority of PRs, with only a handful of changed attributes, this will make the purity test take a few seconds maximum. Think about it, with the current approach we are comparing 70k attributes per PR, although only 1 (or none!) might have changed.

Once this is not a big check anymore, we can move it into the main eval workflow. The effect of running on undrafting can thus be ignored for now. Yes, this has consequences: We need to run this after the comparison, which is exactly what I had opposed earlier, because of start-to-finish time for nixpkgs-review. But that's not a problem, if it doesn't take long. It will take much longer on staging, when you cause many rebuilds - but also you're not going to use nixpkgs-review there anyway.

With how eval is currently set up, we'd need to:

• Run the matrix of 4x jobs for main eval.
• Run the process job to combine and compare.
• Run the release-checks job, but possibly on a matrix of 4x jobs as well, otherwise it would take really long on staging.

Running an extra 4x mini jobs, which only re-eval a single package in the extreme case on master seems like waste, though.

Thus, I am currently working on my idea mentioned in #406825 (comment), namely switching up the order of "combine" and "compare". The new workflow would look like this:

• Run the matrix of 4x jobs for main eval.
• In those 4x jobs, download the intermediate results from the master branch for the current system.
• In those 4x jobs, do the diff for added, removed, changed and upload it.
• In process / tag do the combine and the remaining pieces of comparison.

This has a few benefits, because we'll be able to eventually get rid of a whole job (process), but also we could add the purity release-check into those 4x jobs for eval as well - the numbers for which attributes to run the check would already be known.

The get-merge-commit workflow has increased massively in complexity. I feel like this took a wrong turn after the latest discovery, that different workflows for a single event are not guaranteed to run on the same merge commit (which still puzzles me).

Right, but I think this is a desirable change for consistency, mainly avoiding weird results in the kind of conditions we just observed. I’d be willing to split it into a separate PR. WDYT?

Regarding your idea: I like it in theory, but I’ll need to think about it a bit more.

The get-merge-commit workflow has increased massively in complexity. I feel like this took a wrong turn after the latest discovery, that different workflows for a single event are not guaranteed to run on the same merge commit (which still puzzles me).

Right, but I think this is a desirable change for consistency, mainly avoiding weird results in the kind of conditions we just observed. I’d be willing to split it into a separate PR. WDYT?

I'm hoping to go into a different direction instead, but that's currently only an idea and not fully sketched out, yet:

• Currently, we skip all jobs when there is a merge conflict. This is unfortunate, because we're missing opportunity to provide feedback. We could just return a different targetSha in this case - the merge-base of the head and target branches. This would also allow to run all the comparisons and give 99% correct feedback. The missing 1% is not a concern, because when the merge conflicts are resolved, CI runs again anyway.
• With this, the get-merge-commit part doesn't need to be in a separate job / workflow anymore. It can become just a regular step, because there is no "skipped" case that we need to consider anymore (this is the main reason why this is a separate job). This saves quite a few jobs and also cleans up the output in the check list.
• I just merged workflows/eval: run when base branch changed #372475, which means that the workflows don't react to edited events anymore. IIUC, this means that we don't need the special error cases in get-merge-commit anymore: The "PR is not open anymore" would only happen for edits anyway. This means that we might be able to go back to the GitHub special branch refs/pull/xxx/merge in the checkout action, which would reduce complexity massively.

If we go this way, then there is no other get-merge-commit workflow / job to wait on, so the artifact idea won't work. However, I think that's not really a problem - we just need to make sure that different workflows don't depend on each other like we tried here.

Regarding your idea: I like it in theory, but I’ll need to think about it a bit more.

You can see the new outpaths jobs in #410852. They already produce a diff of attrpaths, which is what you could use to decide which paths to do the purity check against.

Does that work if the problem is that the existence of attribute paths in some way depends on the value of pkgs.path? (If the actual release checks don’t catch this then I suppose it’s fine, though. But IIRC they do?)

Does that work if the problem is that the existence of attribute paths in some way depends on the value of pkgs.path? (If the actual release checks don’t catch this then I suppose it’s fine, though. But IIRC they do?)

Interesting. Correct - this would not be caught. Attrpath generation takes ~ 20-30s per system. Thus, doing this with both paths for all systems in sequence would take max 4 mins. I'd say we could run this as a separate job, independent of everything else. This would make it easy to only run this in the merge queue later, which should be enough for what is probably very much an edge case.

Attrpath generation takes ~ 20-30s per system. Thus, doing this with both paths for all systems in sequence would take max 4 mins. I'd say we could run this as a separate job, independent of everything else.

We can do even better: attrpath generation is using a single core only. Thus, we could run both attrpath generations at the same time in the main eval job. Then we do the purity check for attrpaths already and can error out early in case of a mismatch.

Then we do main eval, then the diff against master, and then we run eval again on the changed attrpaths for purity.

For PRs with 0 rebuilds, the overhead will be 0. And the overhead for release checks scales with the number of changed packages, which is nice.

At one point I was tracking with the back-and-forth on this PR, but I've lost the plot. Where are we with this effort, @winterqt and @wolfgangwalther?

At one point I was tracking with the back-and-forth on this PR, but I've lost the plot. Where are we with this effort?

I think all the pre-requisites have been merged to do #406825 (comment) for evalPurity now.

This will still leave us with the badFiles and conflictingPaths checks. Those seem to be static analysis and thus not really related to eval. I think they might actually fit well into nixpkgs-vet. I was thinking about adding them to ci/nixpkgs-vet.nix first, with the goal of somebody upstreaming them?

Good news: I'm (one of) the maintainer of nixpkgs-vet. I'd be delighted to see it grow checks for those sorts of defects. I agree it's the right place to add them.

I'll have time to work on that in a few weeks.