Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make fetch* source derivation names (optionally) more descriptive and homogenize them across fetchers #49862

Open
wants to merge 3 commits into
base: staging
Choose a base branch
from

Conversation

oxij
Copy link
Member

@oxij oxij commented Nov 7, 2018

Description of changes

This patch adds lib.repoRevToName function that generalizes away most of the code used for derivation name generation by fetch* (fetchzip, fetchFromGitHub, etc) functions.

It's first argument controls how the resulting name will look (see below).

Since lib has no equivalent of Nixpkgs' config, this patch adds config.nameSourcesPrettily option to Nixpkgs and then re-exposes lib.repoRevToName config.nameSourcesPrettily expression as pkgs.repoRevToNameMaybe which is then used in fetch* derivations.

The result is that different values of config.nameSourcesPrettily now control how the src derivations produced by fetch* functions are to be named, e.g.:

  • nameSourcesPrettily = false (the default):

    $ nix-instantiate -A fuse.src
    /nix/store/<hash>-source.drv
    
  • nameSourcesPrettily = true:

    $ nix-instantiate -A fuse.src
    /nix/store/<hash>-libfuse-2.9.9-source.drv
    
  • nameSourcesPrettily = "full":

    $ nix-instantiate -A fuse.src
    /nix/store/<hash>-libfuse-2.9.9-github-source.drv
    

See documentation of config.nameSourcesPrettily for more info.

In effect, this patch also homogenizes derivation names produced by 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).

@edolstra
Copy link
Member

edolstra commented Nov 7, 2018

No, we expressly switched to source last year to prevent the situation where every fetcher produced different store paths. This meant that (for instance) switching from fetchGit to fetchTarball would trigger a rebuild. See NixOS/nix#904.

@edolstra edolstra closed this Nov 7, 2018
@GrahamcOfBorg
Copy link

Failure on aarch64-linux (full log)

Attempted: gnuradio-rds

Partial log (click to expand)

shrinking /nix/store/z2nrx0lk13f3hd9fb7dfx2193r8fw15v-python2.7-PyQt-x11-gpl-4.12/lib/python2.7/site-packages/PyQt4/QtCore.so
shrinking /nix/store/z2nrx0lk13f3hd9fb7dfx2193r8fw15v-python2.7-PyQt-x11-gpl-4.12/lib/python2.7/site-packages/dbus/mainloop/qt.so
strip is /nix/store/6dpnd5aniypn8124mmy8f88s4mq2zl07-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/z2nrx0lk13f3hd9fb7dfx2193r8fw15v-python2.7-PyQt-x11-gpl-4.12/lib  /nix/store/z2nrx0lk13f3hd9fb7dfx2193r8fw15v-python2.7-PyQt-x11-gpl-4.12/bin
patching script interpreter paths in /nix/store/z2nrx0lk13f3hd9fb7dfx2193r8fw15v-python2.7-PyQt-x11-gpl-4.12
/nix/store/z2nrx0lk13f3hd9fb7dfx2193r8fw15v-python2.7-PyQt-x11-gpl-4.12/bin/.pyuic4-wrapped: interpreter directive changed from "/bin/sh" to "/nix/store/n1kfdl37qpzh3xn6klbym1ay6xpxvmw1-bash-4.4-p23/bin/sh"
checking for references to /build in /nix/store/z2nrx0lk13f3hd9fb7dfx2193r8fw15v-python2.7-PyQt-x11-gpl-4.12...
cannot build derivation '/nix/store/capjs47ha289ppvybp2q7jpwk5z9c102-gnuradio-3.7.13.4.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/71vxi4bllz854dj1p538gbvwagk31qjz-gnuradio-rds-1.0.0.drv': 2 dependencies couldn't be built
error: build of '/nix/store/71vxi4bllz854dj1p538gbvwagk31qjz-gnuradio-rds-1.0.0.drv' failed

@GrahamcOfBorg
Copy link

Timed out, unknown build status on x86_64-linux (full log)

Attempted: gnuradio-rds

Partial log (click to expand)

cannot build derivation '/nix/store/qknjgpiqg4rcm3wqdjr744navbbqs927-python2.7-wxPython-3.0.2.0.drv': 5 dependencies couldn't be built
cannot build derivation '/nix/store/0yd2m8hysdf5855j0i6f31ira41gqlmk-qt-4.8.7.drv': 5 dependencies couldn't be built
cannot build derivation '/nix/store/lkvmzj02jlflzwgzzxnaj99df8ajhila-hook.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/bwjvjw92p0mifbi7384859qjgp06lqc2-libpulseaudio-12.2.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/vxn58fji490a8mkgwqbbf06bf5f6z6q3-python2.7-PyQt-x11-gpl-4.12.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/xxhj6vhzhv82c6filpn02r4ifj60ivyf-SDL-1.2.15.drv': 3 dependencies couldn't be built
cannot build derivation '/nix/store/ly0y1m1q9ijhpn3mb0aymwh1ryiq2szd-qwt-6.1.3.drv': 2 dependencies couldn't be built
cannot build derivation '/nix/store/ajsgz3mikjqzfb5iwfyr4bdf5yyq6br1-gnuradio-3.7.13.4.drv': 11 dependencies couldn't be built
cannot build derivation '/nix/store/7n6jk065khcmgl30dcdbk6hf7kd1l0wk-gnuradio-rds-1.0.0.drv': 2 dependencies couldn't be built
error: build of '/nix/store/7n6jk065khcmgl30dcdbk6hf7kd1l0wk-gnuradio-rds-1.0.0.drv' failed

@oxij
Copy link
Member Author

oxij commented Nov 7, 2018 via email

@oxij oxij mentioned this pull request Nov 8, 2018
1 task
@antislava
Copy link

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 fetchTarball (with url = "${arg.url}/archive/${rev}.tar.gz") and fetchGit (with url = <path-to-the-mirror-clone>), respectively. Normally, such a switch doesn't cause rebuilds and the out paths are identical (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...). Such an invariance is obviously a very important property to have (even if I also found this very discussion while looking for a way to append a meaningful name to the out path, rather out of curiosity than necessity really.) 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)?

@oxij
Copy link
Member Author

oxij commented Dec 7, 2018 via email

@antislava
Copy link

@oxij I meant exactly this thing you mentioned above:

In practice switching from fetchgit to anything else triggers a rebuild anyway because, e.g. tarballs are usually produced by git-archive

In my (limited) experience switching from a github repo and fetchFromGitHub to its git clone --mirror and builtins.fetchGit did produce the same store paths and didn't cause rebuilds, i.e. the desired behaviour which I didn't take for granted. I guess you have more experience, in particular when this is not the case (my scope was mainly haskell projects so far).

On the second point, I thought that e.g. fetchTarball and nix-prefetch-url commands (which I call from make in my workflow to produce json files to be imported by nix, to avoid having explicit hashes in my nix files/logic) did not have the optional name argument, which would then be added to the store paths to make them more informative. 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?).

@oxij
Copy link
Member Author

oxij commented Dec 8, 2018 via email

@antislava
Copy link

antislava commented Dec 10, 2018

@oxij

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...

Thank you for the information - good to know about these cases.

BTW, I checked that both builtins.fetchurl (<nix/fetchurl.nix>) and builtins.fetchTarball accept the optional name argument (appended to the store path). Given that, I personally prefer the current solution #49862 (comment) (avoiding rebuilds on switching the fetcher by default but providing enough flexibility if needed) as it is.

@oxij
Copy link
Member Author

oxij commented Jul 17, 2023

Reopening...

@oxij oxij reopened this Jul 17, 2023
@oxij oxij requested a review from infinisil as a code owner July 17, 2023 11:28
@github-actions github-actions bot added 6.topic: kernel The Linux kernel 6.topic: lib The Nixpkgs function library labels Jul 17, 2023
@oxij oxij force-pushed the tree/repo-to-name branch from 97dd9ed to 088b934 Compare July 17, 2023 11:29
@infinisil
Copy link
Member

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

@oxij
Copy link
Member Author

oxij commented Jun 2, 2024

Alright, so I rebased this, switched the base to staging here, while also splitting off the noop part based on master into a separate PR #316668.

Since the first two commits are a noop, the last commit here now neatly shows the actual state fetch* names in Nixpkgs.

So, in short, the reality is that those names are a mess ATM.

oxij added 3 commits June 5, 2024 11:46
…ctions

This patch adds `lib.repoRevToName` function that generalizes away most of the
code used for derivation name generation by `fetch*` functions (`fetchzip`,
`fetchFromGitHub`, etc, except those which are delayed until latter commits
for mass-rebuild reasons).

It's first argument controls how the resulting name will look (see below).

Since `lib` has no equivalent of Nixpkgs' `config`, this patch adds
`config.nameSourcesPrettily` option to Nixpkgs and then re-exposes
`lib.repoRevToName config.nameSourcesPrettily` expression as
`pkgs.repoRevToNameMaybe` which is then used in `fetch*` derivations.

The result is that different values of `config.nameSourcesPrettily` now
control how the `src` derivations produced by `fetch*` functions are to be
named, e.g.:

- `nameSourcesPrettily = false` (the default):

  ```
  $ nix-instantiate -A fuse.src
  /nix/store/<hash>-source.drv
  ```

- `nameSourcesPrettily = true`:

  ```
  $ nix-instantiate -A fuse.src
  /nix/store/<hash>-libfuse-2.9.9-source.drv
  ```

- `nameSourcesPrettily = "full"`:

  ```
  $ nix-instantiate -A fuse.src
  /nix/store/<hash>-libfuse-2.9.9-github-source.drv
  ```

See documentation of `config.nameSourcesPrettily` for more info.
…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 both the default `config.nameSourcesPrettily` setting and with
`config.nameSourcesPrettily = true`.
@oxij oxij force-pushed the tree/repo-to-name branch from b03db0a to 8749da1 Compare June 5, 2024 11:48
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: fetch 6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants