NixOS
Feature Request: Warn when mass rebuild PR has wrong base
This is a small feature request I have that would be helpful for PRs.
Basically, when either 'rebuild-darwin: 501+' or 'rebuild-linux: 501+' is found, check what the base branch is. If it's based off of 'master', post a warning message from @GrahamcOfBorg explaining that it should be based off of 'staging' instead (except in rare circumstances).
I think this is a really good idea, but am afraid of adding more comments to pull requests :(
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
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…
What if instead of a comment it failed the PR's status?
if master & 501+ => fail: please move to staging
cc @vcunat
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?
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?
I think the usage percentage might matter... But as a starting point let's try it as pure numbers.
Indeed -- 501+ -> staging has always sort of been loose policy. Failing the PR would make it less soft policy and more hard policy.
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?
Well, when there are hundreds of small autoconverted packages, the inconvenience of going through staging is larger than rebuilding the twenty you actually need now…
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.
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.
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?
I don't think that level of detail in the rules is a feasible thing right now.
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.
@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.
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.
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.
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.
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.
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.
ofborg counts based on changed named attributes, not based on total changed derivations,h so it should count an npm-based project as one.
Maybe you can offer some insight in to what you would find useful, and possibly better package-rebuild buckets?
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.
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.
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.
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.
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.
Another thing that can change the balance is a notable bugfix (a security issue, a data integrity risk)…