None of our jobs is expected to run for 6 hours, the GitHub limit. These
limits are generous and take into accounts that some jobs need to wait
for others.
If jobs exceed these times, most likely something else is wrong and
needs investigation.
This reverts commit f2648b263b.
While the idea to never use swap was fine, in practice this meant that
when nix ran OOM, some other process was killed instead. This lead to
the job not being possible to be cancelled anymore and thus needing to
timeout, before subsequent jobs could be scheduled. This can take up to
6 hours for GitHub Actions by default.
Re-enabling the swap file to catch this case more gracefully. It's still
the goal to never actually *use* the swap file during Eval and just a
safeguard.
Keeping the changed chunkSize and not reverting it - this makes it
slightly less likely to hit the swap file when running with Lix.
Recent performance tests show that (a) swapping heavily slows down the
Eval job, while (b) lowering the chunkSize does not have an effect on
run-time. It does on memory usage, though - thus we can get rid of
swapping entirely by reducing chunkSize respectively.
This will give us a better idea about:
- Which jobs use the most API calls and can possibly be made more
efficient.
- Which rate limits apply exactly to which tokens.
This can only happen if the target branch's eval run failed and we're
trying to fix it. In this case, Eval should not fail, even though it
might not be able to do a comparison and add the correct rebuild labels.
But if we failed here, we wouldn't be able to merge the fix once we have
required status checks.
This allows *not* depending on those two jobs with the required status
checks in the next commit, which wouldn't really make sense. If labeling
or pinging maintainers fails for obscure reasons or because the GitHub
API is down, a PR might still pass all other tests and be
merge-eligible.
The overall idea is to use names short enough to fit into the status
checks list without shortening. This change mostly happened in the
commits before, here we just follow the same pattern for the remaining
workflows.
We previously moved this out of the main eval workflow to avoid running
it on push and/or undrafting the PR. The latter has been removed in the
meantime and the former can be checked with a simple condition. Thus we
move it back in, to make it part of the 4 main workflows, which will be
required before merge eventually.
Those two workflows bundle all the main jobs in two event-specific
wrapper workflows. This enables us to do two things later on:
- Synchronize the merge commits between most of the jobs run in a PR.
- Create a single "required" job to be targeted by GitHub's "required
status checks to pass" feature.
This didn't work as intended. When a workflow is run with
`workflow_call`, it will have `github.workflow` set to the *parent*
workflow. So the `caller` input that we passed, resulted in this
concurrency key:
```
Eval-Eval-...
```
But that's bad, because the labels and reviewers workflows will cancel
each other!
What we actually want is this:
- Label and Reviewers workflow should have different groups.
- Reviewers called via Eval and called directly via undraft should have
*different* groups.
We can't use the default condition we use everywhere else, because
`github.workflow` is the same for Label and Reviewers. Thus, we hardcode
the workflow's name as well. This essentially means we have this as a
key:
```
<name-of-running-workflow>-<name-of-triggering-workflow>-<name-of-event>-<name-of-head-branch>
```
This should do what we want.
Since workflows can be made reusable workflows later on, we add those
hardcoded names to *all* concurrency groups. This avoids copy&paste
errors later on.
This can happen when two PRs run at the same time, which come from
different forks, but have the same head branch name.
github.head_ref is suggested by GitHub's docs, but.. that's not really
useful for cases with forks.
This moves the actual labeling from the eval workflow to the labels
workflow. At this stage, this only has a disadvantage: Adding the
topic-labels to the pull request will now only happen after eval has
finished, instead of instantly.
We will only benefit from this later, when we manage approval related
events. With this change, we will have the comparison results and thus
the package maintainer info available.
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-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.
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.
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.
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.
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.
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.
While OfBorg is still adding these, it takes a much longer time to do so
compared to the eval action. Since we're adding rebuild labels, I think
it'd be nice to just do it within the eval action.
All workflows where it's remotely useful now trigger on a pull_request
event when the workflow file itself is changed. This gives us basic
sanity testing of changes to workflow files itself and reduces the need
for manual tests in forks.
Previously, the attrs step consisted of:
- 7s queue time
- 1m 15s run time
Only 25s of this were spent preparing the attr paths. A bit more than a
minute was just spent for queuing, checking out the repo, downloading
nix, downloading dependencies, uploading the artifacts - and then
downloading them again in the next step. All of that can be avoided if
we collect the attrs as part of the outpaths job.
By running the attrs step as part of each outpaths step the attrpaths
will be collected 4x, but:
- We save a minute for each eval run to complete.
- We save a full job, giving us more free runners and *possibly* less
queue times for other jobs in the repo.
- We reduce complexity in the workflow file.
