scripts/update: Set nix-update-script by default #325074
Conversation
|
Why? |
|
I just want to preface my comment with this: I'm opening this PR not to have it immediately merged, but just to get a discussion going.
Basically, much of the same rationale that would apply for #178468 or any other non-trivial change to default values. |
|
What does |
| @@ -120,7 +120,7 @@ let | |||
| if pathContent == null then | |||
| builtins.throw "Attribute path `${path}` does not exist." | |||
| else | |||
| packagesWithPath prefix (path: pkg: builtins.hasAttr "updateScript" pkg) | |||
| packagesWithPath prefix (path: pkg: !(builtins.hasAttr "updateScript" pkg && pkg.updateScript == null)) | |||
There was a problem hiding this comment.
not blocking, but wonder why we didn't just use pkg ? updateScript instead of the hasAttr bit
ofborg
bot
added
10.rebuild-darwin: 0
It's this, but upstreamed to nixpkgs: https://github.com/Mic92/nix-update/ |
Pandapip1
force-pushed
the
nix-update-script-by-default
branch
from
89024d3
to
cd3fa01
Compare
|
It is similar to the debate between whitelists vs blacklists. Generally, I prefer the former since there can be issues that require human evaluation. But I acknowledge that it takes much more work.
On the top of my head:
|
This would be a case where a custom update script is needed anyway.
Broken how? It would at least build, as the bot tries to build it before sending a PR.
Same as above, we'd want a custom script if some versions need to be handled differently. |
A pull request is oriented to a merge, not to a discussion. But OK.
Famous last words A failure creeping an already massive package database?
Why does it feel redundant?
You are trying to fix a failure of documentation by burying this poorly documented feature in all packages? As an example, Just do a small research about splicing and explain like I am five why
Your comparison could not be more unconvincing. Globally setting strictDeps as true deals directly with the core task of Nixpkgs, namely, building software. It requires a huge refactor, since many softwares packaged by Nixpkgs are not built with cross-compilation in mind - including some Nixpkgs dark corners, ironically -, and setting strictDeps shows many of those cockroaches (to quote @\Aleksanaa). But, setting an out-of-tree updater script? Why?
Hum, nice. Injecting a third party project in all expressions and packages, what could go wrong?
Oh, the auto update fails, and that's it. Never mind. |
Ryanbot does not test it over Darwin. Example: #324211 |
|
I was just asking in Matrix earlier because it was completely unclear to me whether you’re meant to explicitly set |
As far as I remember, ryanbot can work automagically on the easiest packages. However, ryanbot also honors an updateScript explicitly set. It is useful for some harder cases like multi-source packages.
Updater scripts should never be a hard requirement. |
|
Hence “should”. |
Sorry, meant that as a sub-point of the multiple sources. |
|
Would you guys prefer that I make a bot that automatically tries to add a |
|
Would the bot be able to tell if the update script has any chance of working? The nice thing about a default is that it doesn’t imply that. |
Addressing #325074 (comment)
This is a fair point. The question is whether it's worth spending that extra bandwidth in order to keep packages more up-to-date.
For linux, builds are built (and if tests are available, tests are run). For darwin, broken PRs are a valid concern, but that's true for all PRs that touch darwin and is something that should probably be solved.
@r-ryantm uses https://repology.org/ to detect if there's a new version that's been included to a different downstream package repository, and naively tries to update the version and the hash.
This is also a valid concern. Addressing #325074 (comment)
You misunderstand what a failure looks like.
The same line, repeated hundreds if not thousands of times, is redundant. Yes, it's not much storage-wise, but my concern is more about the repeated work, not the reduced code quality.
...Yes, I guess?
I don't get what point you are trying to make here, or how it relates to the broader discussion here.
This directly deals with that same core task by ensuring that the software that is built is more up-to-date.
See my previous comments. There are more reasons than just saving memory (and that's not even a valid reason, as due to the way git works, those older versions will also be saved.
Auto-updates fail. Nothing bad happens, and once someone forks it, life continues as normal.
The worst that can happen is that auto-updates fail, or maybe there's an RCE that allows for the PR contents to be arbitrarily changed. Since manual merges / reviews are necessary by a commiter or a package maintainer, this is also fine.
Auto-updates fail or the PR contents can be arbitrarily changed.
There are other possible effects too. Addressing #325074 (comment)
Are you sure this is not because nixpkgs/lib/systems/inspect.nix Line 94 in 552844e If darwin is just straight up never tested, is there a technological reason why? Addressing #325074 (comment)
Yes. |
|
drafting to prevent accidental merge |
|
What does r-ryantm do if BTW @AndersonTorres I already suggested moving But it should also be noted that r-ryantm, which is also an out-of-tree bot that we have no control over, has provided more contributions to nixpkgs than any human contributor. Switching to a better-maintained script (nixpkgs-update -> nix-update) seems like it only has upsides WRT the bot. |
Indeed it is not a good idea. It is just vendoring with extra steps. Further, nix-update is not limited to Nixpkgs.
I agree 100%.
nix-update repo also says it is better suited for interactive usage, not for massive updates. |
|
As far as I see, https://github.com/nix-community/nixpkgs-update/blob/main/src/Update.hs does not indicate any intelligent updating for various language builders, it just tries to update src. |
|
If that was the case, this PR would not bring any benefit. But as far as I can tell, they are independent projects,see the code example linked above. |
Not everyone here lives inside a backbone.
Yes, and we all memorized the error codes emitted by the tool, so that we know if the original repo changed location or was deleted, if the tarball was deleted because upstream does not like version control, if the tool itself is bugged or does not support that specific protocol... Ah, about unsupported protocols, as far as I remember nix-update does not support oldschool tarballs.
And we at Nixpkgs should be utterly proud of our wonderful reputation of having a subpar documentation.
About splicingSplicing is a huge piece of dark magic that less than five people truly understand. And look, splicing is Nix code, kept inside Nixpkgs. If we have basically no control for an in-house tool that pervades all Nixpkgs, why should I blindly believe in a tool that is completely external to it that is not even written in the same language? Just to save less than 35M in a 380M repo, and with the special prize of slowing down Nixpkgs evaluation even more? (The part about old code being backed up is not the point - it is about the negligible economy of mere 50 characters per file)
In a repo with five branches of nv-codec-headers, two or three versions of Nix, eleven GCCs, seven JDKs, seven Clangs... Yes, up-to-date is everything a package manager is concerned.
All auto-updates fail if nix-update disappears.
Not having up-to-date software - a thing you yourself called "the same core task" as cross-compilation support - is not a bad thing? Not being able to build Android apps outside a smartphone is "nothing bad"?
The worst that can happen is that we became dependent on a piece of code that we can't control.
A bug called "not having enough money to run this on an M1 machine" :( |
Paradoxically, a dumb tool is better suited to a massive repetitive task. Further, the same nixpkgs-update honors
Then the documentation of nix-update should be updated:
|
|
You've made your point clear. I'll work on a treewide addition of updateScripts instead. |
|
Just to make sure: But the way it was going to be implemented here is not good. Injecting a default updater in all packages is not good. About another mildly-related incidentFurther, we had a serious problem before when someone tried to be smart (and full of sophistry) and forced a functionality over everyone: it broke eval because it assumed each and every package was "well-written". Let's learn a bit with our mistakes. |
|
@AndersonTorres, I’d really appreciate it if you could try to be a bit kinder and more charitable towards contributors. Accusing people of sophistry for not implementing something the way you thought is best on unrelated PRs gets us nowhere as far as productive mutual discussion goes. |
|
To be clear, sophistry is not always committed on bad faith, and I am not accusing people of having bad faith here and/or using sophistry on this conversation. |
|
I wouldn't go so far as to call @AndersonTorres's comments sophistry. They were firm and sometimes poorly phrased, but nonetheless in good faith and always contained legitimate concerns and/or misunderstandings. |
|
Just to clarify: I was referring to the now‐collapsed section of @AndersonTorres’s earlier comment, which described another contributor on a previous PR as having been “full of sophistry”; I certainly have no intentions of accusing anyone of sophistry myself. (I’ve never personally seen the word used in a way that doesn’t imply bad faith on the part of the presumed sophist, but I recognize that the language barrier may have made the use of the term come off as more hostile to me than intended.) |
Description of changes
Will cause
maintainers/scrips/update.nixto usenix-update-script { }by default if nopassthru.updateScriptis set, unlesspassthru.updateScriptis set tonull.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.