No matter how careful you were, sometimes changes in nixpkgs simply break things. But regularly, I stumble upon accidents that could have been avoided, and if you look at enough of them you start recognizing patterns:
Many of them were “trivial” or innocent-looking, like package bumps or small refactorings.
Many of them were merged rather quickly, sometimes within hours.
Many of them were self-merges.
Many of them had little to no review except from maybe “diff LGTM”.
Some of them didn’t even properly evaluate, which means they hadn’t been tested at all before merging.
Some of these are more actionable than others. For a start, I’d like to have PRs open for at least 24h before merging, to give people a reasonable chance to have a look at it. I’m thinking of a simple “wait 24h” CI pass that will help with this.
Self-merges should be frowned upon (even more than today). I know that we are still understaffed and that some things just don’t get reviewed, but I expect a realistic effort to at least try and give people a chance to have a look at it. (Ping some people, give them a good week to respond for example.)
Some of these breaking PRs are what I call “reckless merging”. I doubt that we can enforce some realistic review standards technically (nixpkgs-review for example does make no sense for a lot of changes), but when things break it’s rather easy to deduce the lack of care afterwards. Admittedly, these don’t happen often, but they still happen often enough that I just spent an hour writing this post because I’m pissed. So let’s discuss this in public and make things better.
I have also seen unfortunate and possibly avoidable incidents where a change with single-digit rebuild count got approved by a committer, left open so that other can comment, then forgotten by everyone for a month or more.
From what I know about the habits of people actually doing the bulk of the merging work while not disregarding OfBorg eval, the 24 hour guideline needs a merge bot.
A bot without access could probably do «request my review when CI passes», is there one that can be just taken?
Can we easily find PRs that were merged before eval checks passed? Sounds a much more useful nagger than the permanently broken (into false-positive direction) direct-push-complainer.
I think without some kind of impact scoping, and given the actual balance in the project, broad demands for more reviews will create a use for something that formally passes as a review independently of the chance it is useful.
How about a ping bot similar to our stale bot that simply leaves a comment after a month to remind us of it? (Just a ping, without adding a label, and probably limited to once per PR to avoid being annoying)
The CI does way too little. Unless there are some NixOS tests, it doesn’t even try to build the package. So it will only really fail on syntax errors etc. which are rare. IIRC it doesn’t even notice a hash mismatch. So “more CI” would a valid response to the problem as well.
GitHub already has a “merge when CI passes” feature (but it requires you to leave the tab open). It’s better than nothing, but in the long run we definitely want a merge bot anyways.
Of borg will attempt to build the installable before the :. So awscli: xxx will build awscli and awscli.passthru.tests if it exists. You can add nixosTests to the tests passthru as well; but that’s up to the maintainer.
Maybe, but there is also a question of how not to reach that mark too often. Apparently the only targeted way that is visible to others but without a notification to all is re-requesting review? Might be nice to have a bot re-requesting review based on some condition.
Hmm, a good question whether building all the changed fixed-order derivations (not only the ones triggered by the rules @jonringer mentions) is a good idea.
As long as the previous hash still exists in the nix store, then nix won’t notice hash mismatches either.
In general, I would recommend adding a name field to fetchers which don’t have version influencing the file output. fetchurl usually gets a url constructed with version in it. But fetchers like fetchFromGitHub don’t, it’s just called “source”. So, it’s very easy to have the source pointing to the stale reference.
This Fall I noticed that GH have started dogfooding their new issues/projects beta for a public roadmap.
They aren’t scheduled yet, but it seems like the main shortcomings for building better issue triage on it are all at least on the issue-specific view (I imagine issues 126, 129, 130 and 133 being the most-helpful).
I don’t think it can meet any needs in the short term, but it’s probably good to be chewing on how it could help since it might be usable at some point in the next year.
I imagine the most-important thing it may add (if the filters and actions are sufficient) is the ability to designate ~official purpose-specific views and automatically move issues/PRs between them…
Views might be things like a triage queue, review-needed queue, merge-needed queue?
Pull things out of the triage queue when feedback is requested and return them when it’s given.
Only move things to the merge queue when CI passes.
I wonder if we could do something helpful here like having the built-in fetchers expose a helper method that can be called to provide a default name if one was not given to the fetcher in the first place. Then mkDerivation could just call that on its src (or srcs) to provide its name if the package source did not already provide a name.
Heck, instead of providing a specific helper method, we could just add lib.makeOverridable to all of the built-in fetchers (honestly we should do this anyway, I hate not being able to override fetchers when patching packages locally), and then add a meta or passthru attribute whose presence says “I’m a fetcher whose name defaults to ‘source’, please call override on me to update my name if you please”. mkDerivation could then check this and call src.override (old: if old?name then {} else { name = "${name}-source"; }). I suppose instead of a flag it could also just check src?name && lib.isFunction src.override or null && (lib.functionArgs src.override).name or false, i.e. “src has a name, is overridable, and its override function takes an optional name”.
This would rebuild the world of course, but if we can get rid of these generic source names everywhere that would be wonderful.
Your PR uses the repo and rev to synthesize a name. Upon reflection, that’s probably a safer approach, as it means the source name depends only on inputs to the source (and thus several derivations that share a source will share the same source derivation too).
No idea if this experience is translatable, but I have been generally following the opposite philosophy for open source projects I maintain (rust-analyzer, and IntelliJ Rust before that ). I explicitly approve self-merges for maintainers, and encourage merging contributor’s PR as long as CI is green, without necessary iterating during review. Rather than blocking a PR, I favor merging and sending follow ups (often reviewer sends a follow up). Itself, this is very much inspired by 42/C4 | ZeroMQ RFC.
This approach worked quite admirably in those contexts! The main pre-requisite I think is sufficiently good test suite.
Again, not saying that this is the direction it makes to go in for NixOS, but I do want to claim that
have PRs open for at least 24h before merging, to give people a reasonable chance to have a look at it.
isn’t necessary a terminal value in and off itself. In some circumstances, it is possible to ship stable software with relatively non-burdensome review process.
We can always talk about the standard loopholes afterwards. At the moment, nothing is technically enforced anyways (heck, nobody prevents you from pushing directly to master), and I’d like to keep it mostly that way.
Sometimes I merely wait an ofborg successful evaluation in order to self-merge it.
However, I test my own things locally before self-merge. Indeed one of my pull requests is waiting literally since the year before (almost two months, to be less dramatic) just because I don’t know what it needs to be done.