The change to use `builtins.storePath` was good - for when the store
path *is* already part of the nix store. In all my tests so far, that
was already the case, because I was iterating on the solution and the
Eval results stayed the same.
But when this is run on a entirely new commit, these the values for
`afterDir` and `combinedDir` are *not* in the store, yet. As part of
running `eval.full` on a new commit they will be created. `eval.full` is
linked up, so that the values passed around there will actually be
derivations, which might not be realized, yet.
Checking whether the input is a path or not fixes this for both cases.
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.
Update the README.md to document what gets returned.
We might in the future split these up into other attrsets but I don't see a usecase for that at the moment.
Not all packages that are reported as changed will actually exist on the
platform that the maintainers are colleted on.
This is the case for some attributes that are only available on Darwin
or explicitly set to `null` on Linux. By filtering out packages without
maintainers, these are ignored - and we should potentially get a small
performance improvement as well.
This attribute was supposed to be set on derivations, to make the
release tools recurse into them. The remaining uses were all on regular
attrsets, though, so this is safe to remove.
The following changes were made:
- Using `lib.` instead of `builtins.`
- Using `mapAttrsToList` instead of `mapAttrs` + `attrValues`
- Joining two of the if conditions with the same return value
- Using `traceIf` instead of `if` / `else`
- Using `showAttrPath` instead of `concatStringsSep`
This condition doesn't make a difference anymore, ever since we removed
the tryEval code from this file and had already enabled unfree packages
earlier anyway.
By now, these files have been changed enough to not need the "vendored
from" notes anymore. These links would still be there when going through
the history of the file, but today GHA CI has not many similarities
anymore to what ofborg did, so these are not really helpful.
These files are tightly coupled with the code in ci/eval and not used
anywhere else. They are subject to the same backporting requirements as
the remaining CI code. They are better placed next here.
It makes no sense to check newly added attrpaths for maintainers on the
target branch - by definition these attrpaths won't exist, yet. We can
avoid falling back to `null` for these etc.
This should not be necessary anymore, because packages that fail to
evaluate should already be filtered out by the attrpath generation step
in main eval.
This change pings maintainers of actually removed packages, aka where
the package's expression is deleted.
This will not ping maintainers of packages that become invisible,
because a (transitive) dependency of them is marked as insecure or
broken.
This allows the labels workflow to support issue management in two ways:
- New package request can potentially created with a `4.workflow:
auto-close` label immediately and be closed automatically this way.
- Existing package requests can be bulk-closed by adding this label.
This has the advantage of posting the explanatory comment at the same
time, which is not possible with regular bulk operations.
This allows running a full comparison between two commits locally.
What was previously `eval.full` is now called `eval.all`. The new
`eval.full` takes a `baseline` argument for the comparison.
We had set a default of 5000 for local evaluation earlier for
`singleSystem`, it makes sense to also use that for `full`.
The README is also a bit outdated, because Nix 2.30 significantly
changed the memory requirements. Rewriting the README to also show the
ability to directly evaluate the current system only.
Not a problem for prepare/commits, but the labels comand will remove the
temp directory again, before it actually runs the command. Nothing good
will come out of that!
When a user deletes their account, they appear as a "ghost user". This
user is represented as `null` on API requests. If such a user had posted
a review before, this breaks a few places, which assume to be able to
access `user.login`.
The owners check is not reproducible, because it depends on the state of
the NixOS org on GitHub. Owners can rename their accounts or they can
leave the organisation and access to Nixpkgs can be removed from teams.
All of this breaks the owners check for reasons unrelated to the PR at
hand.
This PR makes the check for the owners file conditionally required: Only
when the ci/OWNERS file is actually modified a failed check will block
merging the PR. When that's not the case, the check will still fail
visibily in the checklist, but the failure can be ignored.
This is especially relevant for the Merge Queue, which should not be
entirely blocked whenever any of these events happen.
Also, it allows passing the checks in a fork when testing, where the
owners check will *always* fail, because the respective teams and
members are never part of the "user org" that a fork is.
When a contributor mistakenly sets the wrong target branch for a Pull
Request, this can lead to bad consequences for CI. Most prominent is the
mass ping of codeowners, that is already handled in
`ci/request-reviews/verify-base-branch.sh`. But there are other things
that go wrong:
- After eval, a mass ping of maintainers would still be possible, in
theory. Practically, this doesn't happen, because we have a limit of 10
reviewer requests at the same time.
- This will most often contain a change to `ci/pinned.json`, thus the
full Eval matrix of all Lix/Nix versions will be run, burning a lot of
resources.
- The PR will be labelled with almost all labels that are available.
We can improve on the current situation with some API calls to determine
the "best" merge-base for the current PR. We then consider this as the
"real base". If the current target is not the real base, we fail the
prepare step, which is early enough to prevent all other CI from
running.
This moves the no-channel-base check into the prepare script to exit
early and prevent all of CI to run against those branches. We also
provide better output by posting a "Changes Requested" review, using the
existing infrastructure from the old cherry-picks check.
The review will be dismissed automatically once the branch has been
corrected, because the commits check will run and do it.
This is the very first step to extending the commits job to do more than
just cherry-picks in the future: It could check reverts or merge
commits, but also the commit message format and more.
Of course, cherry-picks are still just checked on the stable branches as
before. For now, this allows us to run the part that dismisses automated
reviews automatically. This helps us when we do branch related checks in
the prepare step, which would also create such a review. To avoid
cluttering multiple reviews across a PR, we'll want all of these reviews
to be handled by the same code, thus this change.
This workflow runs the PR and Push workflow files on a `pull_request`
trigger. The intent is to test changes to the workflow files
immediately. Previously, these were run directly from the respective
workflow files.
The new approach allows us to move the logic to run this only when
workflow files changed from the pull_request trigger into a job. This
has the advantage that older jobs are cleaned up, when the PR changes
from a state of "workflow files changed" to "no workflow files changed".
This can happen when changing a PR's base from staging to master, in
which case changes from master would temporarily appear in the PR as
changes. When these include changes to workflow files, this would
trigger the PR workflow via `pull_request`. Once the base is changed,
the PR is closed and re-opened, so CI runs again - but since it's on the
same commit and the new run doesn't trigger `pull_request`, the results
of the previous run are still kept and displayed. These results may
include cancelled or failed jobs, which are impossible to recover from
without another force-push.
Checking this condition at run-time is only possible, because we move it
into a separate workflow, turning the `pr.yml` workflow into a re-usable
workflow. This will make sure to skip the whole workflow at once, when
no change was detected, which will prevent the "no PR failures" job from
appearing as skipped - which would imply "success" and make the PR
mergeable immediately. Instead the "no PR failures" job is not shown at
all for this trigger, which is generally what we want.
Do the same for `push.yml` for consistency.
> Anyway, I keep picturing all these little kids playing some game in this big field of rye and all. Thousands of little kids, and nobody’s around―nobody big, I mean―except me. And I’m standing on the edge of some crazy cliff. What I have to do, I have to catch everybody if they start to go over the cliff―I mean if they’re running and they don’t look where they’re going I have to come out from somewhere and catch them. That’s all I’d do all day. I’d just be the catcher in the rye and all. I know it’s crazy, but that’s the only thing I’d really like to be. I know it’s crazy.
The python-updates branch is not a "development" branch in the sense of
ci/README.md's classification. That's because it allows force pushes.
When rewrites are possible, cherry-picking from this branch should not
be allowed, because the commit references will potentially end up out of
sync.
These kind of branches are now termed "Work-in-Progress" branches. Up
until recently these branches didn't work well for Pull Requests
targeting them, because Eval wouldn't run on them with a push event and
thus, Eval in the PR couldn't succeed either. That's now fixed, PRs
towards *any* WIP branch should work correctly.
Instead of uploading the outpaths as artifact, this uploads them via
cachix. Most of all, this makes CI less brittle, because Eval in PRs
will still be able to succeed, even if no workflow run for the push
event could be found on the target branch. It will just take longer.
This also makes moving Eval into the Merge Queue easier to do: When
downloading artifacts from a different run, these would always have to
match on the right event, too. By pulling from cachix, the same workflow
can support target branches with merge queue and without merge queue at
the same time. The latter would still use the push event, while the
former could use the merge_group event.
Last but not least, this should fix Eval on PRs targeting `wip-`
branches and any other branches that the push event doesn't trigger on.
These would never find an Eval result from the target branch and could
never show rebuilds accurately. Now these PRs should work at a slightly
higher runtime cost.
This needs the multiline flags, which enables `^` and `$` to match line
start and line end, not start and end of the whole string.
Not sure how this got past testing when initially merged.
This ensures we don't accidentally use aliases in the nixpkgs shell or
other places that depend on the CI-pinned pkgs instance.
Nixpkgs generally — and CI specifically — do not use aliases, because we
want to ensure they are not load-bearing and can be removed safely.
See: https://github.com/NixOS/nixpkgs/blob/ce9979ec1c/pkgs/top-level/release-outpaths.nix#L28
Currently treefmt-nix is still defaulting `programs.nixfmt.package` to
the `nixfmt-rfc-style` alias. This makes sense, as they do not know for
certain which revision of nixpkgs is in use.
We do know, however, so we can explicitly use the non-alias name.
