Feature Request: Warn when mass rebuild PR has wrong base about ofborg HOT 37 OPEN

I think it is OK to add more comments for the situations that should be rare and special.

At the same times something about comment coalescing for the common case would be nice, yes…

from ofborg.

What if instead of a comment it failed the PR's status?

if master & 501+ => fail: please move to staging

cc @vcunat

from ofborg.

@grahamc For what it's worth, I'd love to get to the point where we can ditch staging altogether and have the -unstable channels be actually reliable ways to stay up-to-date without rebuilding too much.Maybe eventually we can have ofborg block a PR if the number of packages is too high, but have a command to do a build and populate the cache and then it can be merged.

from ofborg.

I support failure on stdenv change in master. A hand-picked set of extra critical libraries is also reasonable.

I think 501+ label is very useful to compare expectations and reality. I.e. «of course 500 packages changed, it's a change in Python» vs. «wait what why, what did I miss?»

I agree that going the TravisCI-for-Nixpkgs way where «you won't believe me, but the Travis failure is actually relevant» happens, is not attractive.

from ofborg.

Re: PRs vs. direct push — maybe I should make it explicit that I argue from the assumption that we do not want to do a borg policy hard turn between now and the beginning of «maybe no direct push to master» talk.

from ofborg.

from ofborg.

I think this is a really good idea, but am afraid of adding more comments to pull requests :(

from ofborg.

I hope it doesn't happen too much! I guess there might be other ways to warn with GitHub APIs? Some other options:

• regular comment (original)
• PR review comment
• additional status

from ofborg.

Not sure this is a good idea… Does this mean that a rust-unstable update must go to staging once there are enough cargo packages?

from ofborg.

I suppose yes,

Does this mean that a ≪compiler≫ update must go to staging once there are enough dependent packages?

if the goal is to avoid serious inconvenience for contributors, and there are hundreds of packages depending upon ≪compiler≫ then yes... right?

from ofborg.

I think the usage percentage might matter... But as a starting point let's try it as pure numbers.

from ofborg.

Indeed -- 501+ -> staging has always sort of been loose policy. Failing the PR would make it less soft policy and more hard policy.

from ofborg.

from ofborg.

Yeah, I don't know what to say other than rules are imperfect.

from ofborg.

It would be very inconvenient to be the person who depends on those 501+ rust packages and someone pushes a 501+ rust rebuild to master. At the end of the day is what is the 501+ rule for and why do we have staging, and how do those intentions translate in to the goals of the check.

from ofborg.

I mean, a separate conversation is 501+ even the line we want to draw? It was just sort of picked out of a hat when I started tagging.

from ofborg.

I think 501 Python packages are less annoying to rebuild than a single Chromium.

Also, I would expect that unstable rustPackages or lispPackages or pythonPackages have a different typical usage — do we have a person who depends on 500 rust packages?

from ofborg.

I don't think that level of detail in the rules is a feasible thing right now.

from ofborg.

If knowing 500 things rebuilds is not useful, why have the tag at all?

Also, for example the fail could be done only if stdenv is changed.

from ofborg.

I think it's a useful hint, but it's not definite. IMO we should just give it a shot to have 500+ cause a failure and just let trusted people bypass it for now. If it turns out to be obnoxious we can iterate.

from ofborg.

My only hesitation is I'm afraid of making PR checks fail for reasons that can be ignored. As it stands, there is no valid reason to merge a PR when its status is red.

from ofborg.

I agree with @grahamc, that we should avoid introducing failures that are "ok" to be ignored.

I think 500 is probably a bit of a low threshold to disallow a merge. Given that master progresses pretty quickly it's probably impossible for a normal staging merge not to cross that for example. Not sure what a reasonable number is but failing on a stdenv rebuild definitively sounds reasonable to me.

from ofborg.

As another input, updating a big npm-based project’s dependencies is probably also hit that 500 derivation policy. 200+ fixed input derivations (the sources) plus 200+ package derivations plus one node_modules derivation for every non-leaf dependency.

from ofborg.

ofborg counts based on changed named attributes, not based on total changed derivations,h so it should count an npm-based project as one.

from ofborg.

Version bumps of the default kernel are hitting 501+ IIRC.

from ofborg.

Maybe you can offer some insight in to what you would find useful, and possibly better package-rebuild buckets?

from ofborg.

Maybe one more level for Linux, say 500–2000? (The <10 level might get ditched, too.) EDIT: it might be a gray area where some cases are more suitable for master and some for staging.

I'm wary of shifts, as I don't know what we would do with already labeled PRs, though it might not be too difficult to just run eval on all of them again.

from ofborg.

Sounds good.

• Would it be proper to fail a rebuild-2000+ PR to master, saying it should be redirected to staging?
• Would it be proper to fail a rebuild-stdenv PR to master, saying it should be redirected to staging?

I can do the shift in such a way that the old 500+ tag will get removed upon re-eval. Thus, on PRs we're not sure on, we can just trigger a re-eval.

from ofborg.

It seems difficult to make these hard rules, as sometimes changes have a separate jobset so binaries are ready at that moment. I think they usually aren't merged via PRs, though.

I'm still not really sure how to do such workflows, as such branches still often interfere with effectivity of staging (in terms of minimizing total rebuilds), but it seemed typically only a minor issue so far.

from ofborg.

Makes sense, in this case I think the proper solution is probably not to fail the build at all, and fall back to a soft recommendation of using staging via a comment or something.

from ofborg.

Another thing that can change the balance is a notable bugfix (a security issue, a data integrity risk)…

from ofborg.

I would like to see another tag for 500 - 5000 and the number of rebuilds posted somewhere. In my opinion we should forbid merging to master at 5000+. Having that big changes on master slows down staging-next (and all other jobs), and basically means an extra day of delay because staging-next will need to rebuild that set of changes again before it is merged into master.

At 500 - 5000 it should be discouraged. I think @r-ryantm already changes branch to staging at 500+.

Edit: @LnL7 pointed out that, when clicking on the eval check, you're presented with the whole list of items to rebuild.

from ofborg.

I think @r-ryantm already changes branch to staging at 500+.

I manually change @r-ryantm's PRs when I see the "500+" tag. I don't always get to it though.

from ofborg.

If OfBorg removed it after it switched branches, this would make it very easy to search for the PRs that need to switch to staging.

from ofborg.

from ofborg.

If we had separate statuses one could be like ofborg-optional vs ofborg-mandatory.

from ofborg.

Note that this last week there have been 2 mass-rebuilds on master.

from ofborg.