search

nix-community / buildbot-nix

A nixos module to make buildbot a proper Nix-CI [maintainer=@Mic92]
64 stars 18 forks source link

"Register gcroot" doesn't work well with the pull-request flow #151

Open antifuchs opened 1 month ago

antifuchs commented 1 month ago

I noticed the following interaction that causes attrs to not get gcroot-ed when you would expect them to be:

  1. Open a pull request bumping any check attribute's dependencies
  2. buildbot-nix runs and builds the attr. It doesn't register a gcroot because the build runs on the pull request's branch, which isn't the repo's default branch
  3. Merge the pull request into the default branch, while the check attr remains unchanged.
  4. buildot-nix runs, but notices that the attr is cached; it skips the nix build of the attr, and doesn't gcroot the out_path for the attr (because the skipped builds don't have the gcroot-adding step).

This interaction has resulted in my check attrs getting new revisions several times, while the corresponding gcroot still points to versions multiple days out of date.

I think one way to potentially fix this would be to have the gcroot step run on the skipped builder also: It is slightly risky (the attr might be "unchanged" according to the scheduler but not be present in the store due to a mistimed gc run)... but this is already happening, I think.

antifuchs commented 1 month ago

I did fix this in my personal fork of buildbot-nix by not scheduling the "skipped" builder for already-cached attributes. That costs a few seconds of (parallel) build time but it does result in correctly gcrooted artifacts, so that's worth it to me. Not sure if that is a valid solution for everyone, though.

Mic92 commented 1 month ago

The only thing we need to account for is the case, when the package is present in the remote cache but not locally, because nix-eval-jobs will set the isCached flag in any case. And for people with a remote cache, they rather not want things to be downloaded in that case.

antifuchs commented 1 month ago

Hm, yeah, that could be unpleasant. I'm also noticing that n-e-j is setting isCached on derivations that are not present in the local store but also aren't cached remotely, if the derivation had been built previously... so I'm hoping that gcroots will help at least work around that issue for now (:

MagicRB commented 2 weeks ago

okay so the behavior we want here is:

  1. PR is opened
  2. PR is approved
  3. buildbot-nix runs the pipeline (doesn't register roots)
  4. PR is merged
  5. buildbot-nix registers gcroots

or

  1. PR is opened
  2. PR is approved
  3. buildbot-nix runs the pipeline (does register roots)
  4. PR is merged