[staging] fetchFromGitHub: name source with content intristic to the value #153386
Conversation
|
cc @infinisil who has lead RFCs trying to get around this cc @domenkozar who had concerns around flake usage cc @edolstra to make sure I'm not dumb :) |
|
I believe ofborg timed out. As this is essentially a stdenv rebuild |
|
@GrahamcOfBorg eval |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/merge-fast-and-break-things/16957/10 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/getting-rid-of-source-as-default-pname-of-fetchfromgithub/16966/1 |
|
Using |
|
Strong 👎. We switched to If we do switch, it has to be done in a consistent way across the Nix ecosystem, e.g. Nix's builtin fetchers should use the same naming convention. |
But the content never changed, I don't have to recompute already correct FODs. fetchFromGitHub already unpacks the contents, so there's no source archive which influences the sha. The sha changes in this PR are from incorrectly updated packages. If I was influencing the contents, it would be a whole lot more than ~2 dozen sha updates: |
This is only true of the fetchers which don't unpack the contents. When you fetch an archive, generally it already has project and version information which upstream determined. For fetchFromGitHub, it doesn't behave like the builtin fetchers anyway: For Content-addressability, the FODs compute to the same contents: I'm not super familiar to how CAS is being implemented, but I feel like this should be a handled use case. |
|
On a tangential note, fetching from multiple sources is becoming a much more rare occurrence: I understand there's also plans to incorporate ipfs and other means of referring to nix store objects, but we should also be aware that for every current usage of multiple sources, there's 40 other usages of fetchFromGitHub, each potentially having unknown realization problems until a system which doesn't have the stale references attempts to download the sources. |
|
cc @Ericson2314 for CAS input |
|
Drawback is that if you fetch the same tarball via another method, you'll get a different hash. Ideally derivation name wouldn't be part of the hash, but here we are today. I also understand the need to figure out whats in the derivation, so it's a tradeoff. |
| # List generated from home-assistant documentation: | ||
| # git clone https://github.com/home-assistant/home-assistant.io/ | ||
| # cd source/_integrations | ||
| # cd ${src.name}/_integrations |
There was a problem hiding this comment.
I'm going to try and factor out all of the package updates to separate PRs, eventually this should just be the fetcher change and sha fixes
There was a problem hiding this comment.
I guess I could mark this as draft until then
There was a problem hiding this comment.
This is editing a comment that isn't fetcher related at all.
There was a problem hiding this comment.
correct, but eventually I'll fix it when pruning the commits :)
No, we shouldn't do it even temporarily (since temporary things have a tendency to become permanent). Like I said before, I don't want to go back to every fetcher in the Nix ecosystem producing inconsistent names. And it shouldn't be the case that changing the name of a repo or switching from a tag to a revision triggers a rebuild. |
I agree that this isn't ideal. However, there's not a single instance (that I could find) of I guess I'm optimizing for the human experience, and trying to avoid stale FODs. While retaining "source" as the derivation name is optimising for the nix experience. I think it makes sense for the exception be: "To use alongside builtins.fetch*, please pass With the changes in this PR, the default is: " |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-to-reference-src-directory-in-nix/18956/4 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/why-is-the-r-ryantm-bot-using-refs-tags-in-the-version/20519/7 |
|
Over a year ago, @Ericson2314 suggested waiting up to a year and then revisiting this. Can we revisit this? I would love to lose both the footgun of forgetting to update a hash and the confusion of looking at a list of paths named |
|
bump, I still think this is a major QoL win |
|
Building towards element-desktop this is what I see. 🤡 |
| ]; | ||
|
|
||
| patchPhase = ''sed "s@ldconfig.*@@" -i source/Makefile''; | ||
| patchPhase = ''sed "s@ldconfig.*@@" -i ${src.name}/Makefile''; |
There was a problem hiding this comment.
The default name of the FOD produced by fetchurl is the basename of the URL, with file name extension included. In this case, it is currently "aeolus-0.10.4.tar.bz2", and the sensible value is "aeolus-0.10.4".
In my opinion, commands in phases should respect "$sourceRoot", so that overriding with src from a different fetcher is possible. I'll put the discussion under the RFC PR.
|
@oxij's recent work
... #49862 was born in 2018. It will go to school this year.
To be a bit more serious, though, yes, this appears to want to do exactly the same thing my PR does, motivation here is part of my motivation there, and I got exactly the same pushback on it.
Though, I did recently propose a solution to make everyone happy by adding a `nixpkgs.config` option so Hydra can have its `*-source` and developers can have all those sources named prettily in `/nix/store` and force rebuilds on revision change, exactly as here, yes.
|
Now an RFC
NixOS/rfcs#171
Pre-RFC text:
Motivation for this change
DRAFT until package failures around src name are factored out
Just giving the derivation a name of "source" by default causes
a lot of pain when someone may update the rev, but fail to
compute the new sha256 or invalidate the existing sha256. A stale
sha256 will still be "valid" in the eyes of nix, as only the way
in which the sources are fetched has changed, but the previous
.drv and sha256 relationship is still preserved.
This change forces the FODs to be recomputed as the related .drv
has changed.
This will not require all of the FOD sha256s to be recomputed, as
the contents will not have changed. However, it will be a mass
rebuild as it will create a new DAG for packages which didn't
already pass a "name" value to fetchFromGitHub.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes