In a pull_request event, vars are available, but secrets are not. Thus
the steps will run, even though there is no secret to request those
tokens - they will fail.
The intent was, to skip them entirely in that case.
After we removed the dismissed-review workflow, it can happen that
multiple non-minimized reviews exist after a force push. To avoid
cluttering the PR's history while it's actively worked on, we minimize
all of them instead of only the latest.
Any change to shell.nix or ci/ currently needs to rebuild treefmt on 4
systems from scratch. We can avoid that by using the existing cachix
cache.
Adding the cache to lib-tests won't do much on it's own, yet, but it
will with the next commit.
We can avoid running old jobs to completion, when pushing changes to a
pull request. This is done via concurrency groups. We set them on the
workflow level, with the following keys in the group name:
- `github.workflow` to only cancel / block the same workflow.
- `github.event_name` to avoid blocking between pull_request and
pull_request_target.
- `github.head_ref` which is unique for a PR, but the same when changing
it. This will cause PRs to cancel in progress jobs. Unset on pushes to
master & co.
- `github.run_id` as fallback for push events. In this case, the run_id
is unique for every push, thus *no* cancelling happens on the dev
branches.
GitHub comments have a length limit, so we can't just dump everything.
The 10k limit is arbitrary, but the assumption is that reviewing the
range-diff is not the sensible thing to do once it becomes a certain
size - reviewing the regular diff and treating the commit as "new" is
easier to do in that case. Thus, truncating should work out fine,
especially when the full range-diff is still available in the runner
log.
This could still end up in with an error, if a PR has multiple commits,
which all hit the limit. Let's get there first, before we try to fix
that hypothetical case, too.
Instead of failing the job, the workflow will now post review comments
as "Request Changes". This makes the feedback more readily visible and
avoids having to merge despite a failing CI job. It is also a
pre-requisite to enable required status checks / required workflows in
the future.
Committers are asked to confirm the differences by explicitly dismissing
the generated review. After dismissal, the related review comment will
automatically be marked as "resolved".
The comments only report warnings and errors. Reviews are automatically
dismissed when they have been addressed by the author and no problems
remain. If problems remain, existing, still pending, review comments
will be updated. If the same problems had already been dismissed
earlier, no new review comment will be created either.
github-script provides a better way to access the workflow's context
than bash variables + interpolation. Especially when considering future
changes, where you'll always be tempted to just use interpolation
directly in bash code.
Granting the "issues: write" permission allows creating the "port to
stable" label, if it doesn't exist, yet. This avoids failing the
workflow when testing in a fork without that label.
There is no reason not to build the NixOS manual on stable branches as
well.
Also removing the restricted-eval, because it turned out not to be
necessary for the nixpkgs-manual either.
We don't need to run full eval when undrafting a PR. We already have an
eval result, so we can use that to do the maintainer pings.
We need to wait for eval to finish first, but we're already half there
because of how we're waiting for the artifact to appear. Since the
ready_for_review case is triggered in a different workflow run, we'll
need to fetch the ID of the relevant Eval workflow first, though.
This allows us to trigger only the reviewers job when undrafting a PR in
the next step. Split for ease of review. The code is copied 1:1 to
reviewers.yml.
Splitting the job up into two is required to then move the reviews job
to a separate, re-usable workflow in the next step.
To avoid the eval workflow from taking more time to finish, after having
it optimized carefully recently, the reviews job starts in parallel to
the compare job, even though it requires the comparison results to
finish. We can then use the time to already set up nix, build
requestReviews and fetch our app-token.
This might seem overly complex, but given that we need to do the same
dance in the next commit when we run the re-usable workflow separately,
it's actually just easier to review that way, not more.
By using the pinned nixpkgs we have for CI, we can lift the restriction
of building the nixpkgs manual only in PRs targeting master.
At the same time, this uses the pinned nixpkgs for the doc/ folder's dev
shell. This allows entering that shell while working on a staging-based
branch and write documentation.
Why should staging be un(der)documented, after all?
Note: The package that is available in nixpkgs as pkgs.nixpkgs-manual
will still be built with the current nixpkgs checkout, not the pinned
version. This is the same that hydra builds.
Using a `tree:0` filter instead of `blob:none` reduces the checkout time
from over 3 minutes to about 45 seconds. The required trees/blobs will
then be fetched on-demand.
This on-demand fetching creates additional output for `git range-diff`
on stderr, so we hide that. This only happens the first time it's run,
so we don't need to adjust the other calls - which will still return any
real errors, should they happen.
Since DEs like KDE Plasma 6, GNOME and COSMIC are not designed to be X11-exclusive, putting them under `services.xserver` is misleading. In particular, GNOME defaults to Wayland these days and X11 support is going to be dropped in near future.
Let’s follow Plasma and move GNOME NixOS options out of `xserver` attribute.
This patch does not include any changes to X11 support itself.
Signed-off-by: John Titor <50095635+JohnRTitor@users.noreply.github.com>
A while ago, I added those "owner == NixOS" conditions, because I
couldn't figure out why my fork kept failing those jobs, even though I
had set up the apps correctly.
Turns out, that when a label doesn't exist, those actions try to
implicitly create it. But to do that, they actually need write
permissions on the *issues* endpoint, the pull-requests endpoint is not
enough. Even though the docs state otherwise.
Thus, adding those permissions. This will also lead to new labels being
created when they are added via code (for example in
.github/labels.yml), even when they had not been created, yet. Labels
created this way will initially be grayish color and without description
- but we can always add those later, there is no point in failing
pipelines for everyone in that case.
Since process doesn't need to run on push events anymore, we can just as
well remove it entirely. The little bit of combine and comparison can be
done in the tag job, even with elevated privileges. That's because those
parts can be done entirely from the target commit, which is trusted.
This saves startup, installing nix, downloading tools and artifacts for
one job. It saves about 1 minute per run, start to finish.
This moves the diff of outpaths into the outpaths job, mainly as a
preparation to allow future improvements. For example, this will allow
running the purity release checks only on changed outpaths instead of
the whole eval.
This also removes the inefficiency introduced in the last commit about
uploading the intermediate paths twice. Now, only the diff is passed on.
Also, technically, the diff is now run in parallel across 4 jobs. This
should be *slightly* faster than before, where outpaths from all systems
were combined first and then diffed. It's probably only a few seconds,
though.
This is an intermediate step towards more efficiency. At this stage, the
outpaths job pulls the result from the matching outpaths job on the
target branch and uploads both results together. The process job then
downloads both results at once and does the comparison as usual.
This is slightly more inefficient, because the intermediate results are
essentially stored as artifacts twice. But that inefficiency will go
away in the next step, this refactor is split to make it slightly more
reviewable and testable.
On the other side, this allows us to save the process job on push events
entirely, which is a win, because most of it is setup and nix download
anyway.
We don't really need to run the combine and comparison steps from the
untrusted merge commit. By switching to the trusted target commit, we
can avoid adding another worktree - and lay the foundation to later do
those steps in the tag job, which has access to secrets.
Everything is a result, especially when nix-build uses "result" as its
default output. This becomes confusing, when re-wiring the different
parts later.
Thus, consistently name those things after some of their properties and
avoid the term result.
We have added nixpkgs-vet as a regular package to nixpkgs a while ago,
so we can now use it from pinned nixpkgs. This avoids pulling a
platform-specific binary version from upstream.
This change also allows to run the tool easily locally, the same way as
other tools:
nix-build ci -A nixpkgs-vet
This will do a full check of the repo with the exception of
nixpkgs-vet's "ratchet" checks: Those depend on having two branches to
compare, but the default is to only look at the head branch. Those
ratchet checks will still be run in CI, though.
By consistently checking out nixpkgs into the same location in every
workflow, it's easier to reason about the different workflows at once.
We also use crystal-clear names to make clear, which checkouts are
considered trusted, because they only contain target-branch-code and
which checkouts are untrusted, because they contain code from the head
branch. By naming the checkout directories trusted/untrusted, it's
obvious at the call-site.
One example of where we likely did the wrong thing is the nixpkgs-vet
workflow: Fetching the toolVersion from the untrusted checkout opens the
door for an injection into the download URL, thus code could be
downloaded from anywhere. This is not a problem, because this workflow
does not run with elevated privileges, but it's a scary oversight
nonetheless.
Using core.setOutput is much nicer than having to parse the json
"result" on the outside. This also avoids some very odd errors, when the
result can, for unknown reasons, *not* be parsed as JSON later on.
Also avoiding a bit of duplication between the "if mergeable" branches.
In PRs with multiple commits and merge conflicts the logic "targetSha ==
immediate parent of mergedSha" doesn't hold anymore. The head and base
commits of the PR's branch have some commits inbetween them, instead.
Before this change, we'd get a "fatal: invalid reference" on the
"worktree add". Now, not anymore, because we fetch the right commit
directly.
Previously, `eval.full` organized the results for the supported systems
in a specific layout, i.e. with a folder with one subfolder per system.
Then, `eval.combine` relied on that.
When using `eval.singleSystem` and `eval.combine` directly, the caller
was responsible to recreate the same layout. This is annoying and
error-prone to do, when downloading artifacts from CI to recreate some
steps locally.
With this change, all the artifacts can be downloaded and extracted into
the same folder - because the result from `eval.singleSystem` already
contains the <system-name>/ subfolder.
`env` blocks are a bit like `let` blocks in Nix. They define a few
things, which are then used in the `run` block. The workflows are
considerably easier to read, if those definitions come first, making it
crystal clear where they belong and requiring less visual jumping.
This makes a difference for the case of a merge conflict: In that case,
the magic `.../merge` branch actually points to the *last test merge
commit* that was successful, which might not contain the latest head
commit in any way. Running the tests on that commit is heavily
misleading. By using the get-merge-commit action, we run on the PR's
head commit in this case, which is much better.
We don't need a separate workflow anymore, because we don't need to skip
dependent jobs on failures anymore. The biggest failure mode was
"conflict" previously, but we resolved that on the last commit. The
remaining failure modes are so rare, that it's OK to just fail the jobs
in this case instead of marking them as "skipped". Especially, because
the resolve-merge-commit job would have previously failed anyway.
By moving this to an action we avoid running separate jobs each time we
need the merge commit. This also makes the check list in PRs much
cleaner.
When a PR is having conflicts with the base branch, we used to skip most
jobs depending on the target branch. With this change, we still run
those jobs, but without actually merging the PR temporarily. That means
we compare the head of the PR with the merge-base of the PR's branch and
the target branch - i.e. the point where the PR branched off.
This is not 100% accurate, but that's not important, because after
resolving the merge conflicts, those workflows will run again anyway. It
allows to give early feedback, though, instead of just skipping all the
jobs.
The reason this was a separate shell script was, that this would be
included in multiple workflows separately. But a while ago this had been
changed to a re-usable workflow, so we can just as well inline the
script.
This also allows us to use actions/github-script, which makes for a much
more readable script than the bash script before.
After creating the backport successfully, we previously created the
"has: port to stable" label with the Nixpkgs CI App's token. This would
trigger another labeled event for the backport workflow. This only
appears as "skipped", so doesn't use any resources, but it clutters the
GitHub Actions output with useless skipped workflows.
Using `github.token` does not trigger any other workflows so avoids that
problem.
