Make fetch* source derivation names (optionally) more descriptive and homogenize them across fetchers#49862
Make fetch* source derivation names (optionally) more descriptive and homogenize them across fetchers#49862oxij wants to merge 1 commit intoNixOS:stagingfrom
fetch* source derivation names (optionally) more descriptive and homogenize them across fetchers#49862Conversation
234705b to
97dd9ed
Compare
|
No, we expressly switched to |
|
Failure on aarch64-linux (full log) Attempted: gnuradio-rds Partial log (click to expand)
|
|
Timed out, unknown build status on x86_64-linux (full log) Attempted: gnuradio-rds Partial log (click to expand)
|
|
In practice switching from `fetchgit` to anything else triggers a rebuild anyway because, e.g. tarballs are usually produced by `git-archive` (which `fetchgit` does not do) followed by `autogen.sh`/`bootstrap.sh`. And similarly for most other fetchers.
The only exceptions to this rule are different versions of the same thing, like `fetchgit` and `fetchgitLocal`, and fetchers based of `fetchzip`.
This patchset doesn't touch the first and keeping the hashes for the `fetchzip`-based is a trivial addition to this, if anybody actually cares.
Also note that on master _most_ fetchers don't use `-source` suffix, only the most common ones do.
I'd say closing this is premature.
|
|
In defense of the current naming scheme, I would like to add that I often switch between github and a other git mirrors, and then I also switch between |
|
As I said above, to get the equivalent of the current naming scheme in that regard all you need to do is to make `repoToName` of this patchset ignore its first argument. One mass rebuild later things will stabilize again.
as long as the hash produced by `nix-prefetch-git` is the same in both cases, which they always are in my experience - maybe I was lucky...
`fetchGit` and `fetchgit` do the same thing, pretty much, so that's not surprising. But `fetchgit` and `fetchFromGitHub` do different things, their results don't match on a lot of repositories.
Nevertheless, could it be possible to pass something like `name ? "source"` as an attribute of an argument set to allow explicit overrides, if necessary, and maybe add the corresponding option to the `nix-prefetch-git` function (now there is an `--out` option but I didn't quite understand what it did)?
I don't understand, please elaborate. AFAIK you can pass `name` to most `fetch*` already, the point here is that the default naming of those `fetch*` outputs is very inconvenient for when you want to glance at the source real quick.
|
|
@oxij I meant exactly this thing you mentioned above:
In my (limited) experience switching from a github repo and fetchFromGitHub to its On the second point, I thought that e.g. |
|
For instance, repositories with `.gitattributes` file usually produce different hashes when switching between `fetchgit` and `fetchFromGitHub`. Some random examples: firefox/tor-browser/palemoon, qTox, linux, nixpkgs itself...
`fetchTarball` and `nix-prefetch-url` commands *did not* have the optional `name` argument, which would then be added to the store paths to make them more *informative*
`nix-prefetch-url` has `--name` option, see the man page.
`fetchTarball`, judging from a glance at the sources, does not.
If I am wrong then I (personally, at least) do see this issue as closed. Otherwise, I wonder if such an optional argument could be added (and then it wouldn't have to cause any massive rebuilds, would it?).
This patchset causes a single massive rebuild. Prefetch scripts, as noted above, can either be fixed by reimplementing some of `repoToName` in bash or, IMO, preferably, just dropped in favor of building derivations without requiring those hashes via nix itself.
|
Thank you for the information - good to know about these cases. BTW, I checked that both |
|
Reopening... |
|
Rebased on top of #247977 and dropped the |
|
Ok, so, with #245388, #247977, #248528, and #248683 merged this became mostly trivial now. So, I remade this a bit to use Nixpkgs' I still think this default is unhelpful for anyone but Hydra (see the description for the new (@GrahamcOfBorg fails with unrelated error :( somebody broke the global reference to |
|
Switched the base to |
|
There's an RFC for this now 😅, see the above links. I'm afraid I'm a bit low on time to review this, but I'm hoping that this idea gets others helping out with the RFC |
|
Alright, so I rebased this, switched the base to Since the first two commits are a noop, the last commit here now neatly shows the actual state So, in short, the reality is that those names are a mess ATM. |
|
Thanks so much for the work on this. The RFC got closed due to lack of interest and the person driving the RFC is banned from our community. So this seems like a bureaucratic dead end. The way this PR is currently written makes the behavior opt-in. I really don't see why this has to go through an RFC. It sounds uncontroversial to merge. I still really want this. Seems like RFC process is not the right way to get this change to land. How do we proceed? |
|
Well, I updated this, but #316668 still needs to be merged first. |
|
Given that this doesn't change any default can we retarget this to master? Edit: oh no the last commit does produce a mass rebuild I think. Sorry I misread. |
|
@winterqt Done. |
|
This will need another rebase now that the dependent PR has been merged. Then we can merge this into staging and we should be all good to go! |
…enize `name`s in `fetchsvn` In effect, this homogenizes derivation names produced by all `fetch*` functions so that switching from, e.g., `fetchFromGitHub` to `fetchgit` would be a noop (assuming the content hash does not change, which is not always the case for `fetchFromGitHub` since GitHub uses `git archive` internally and `fetchgit` does not) with all `config.fetchedSourceNameDefault` setting values except for `"full"`.
|
Done. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/fix-your-fods-fods-and-security/71531/2 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/fix-your-fods-fods-and-security/71531/15 |
Description of changes
This patch adds
lib.repoRevToNamefunction that generalizes away most of the code used for derivation name generation byfetch*(fetchzip,fetchFromGitHub, etc) functions.It's first argument controls how the resulting name will look (see below).
Since
libhas no equivalent of Nixpkgs'config, this patch addsconfig.fetchedSourceNameDefaultoption to Nixpkgs and then re-exposeslib.repoRevToName config.fetchedSourceNameDefaultexpression aspkgs.repoRevToNameMaybewhich is then used infetch*derivations.The result is that different values of
config.fetchedSourceNameDefaultnow control how thesrcderivations produced byfetch*functions are to be named, e.g.:fetchedSourceNameDefault = "source"(the default):fetchedSourceNameDefault = "versioned":fetchedSourceNameDefault = "full":See documentation of
config.fetchedSourceNameDefaultfor more info.In effect, this patch also homogenizes derivation names produced by
fetch*functions so that switching from e.g.fetchFromGitHubtofetchgitwould be a noop (assuming the content hash does not change, which is not always the case forfetchFromGitHubsince GitHub usesgit archiveinternally andfetchgitdoes not).