Get rid of .drv special-casing for store path installable
#7600
Conversation
Ericson2314
requested review from
fricklerhandwerk,
edolstra and
thufschmitt
as code owners
| bo.path.isDerivation() | ||
| ? bo.path |
There was a problem hiding this comment.
This allows nix show-derivation /nix/store/asdfasdfasdf-foo.drv to work. It would be silly to require ^* for that!
If we do end up doing all of #7261, I am pretty sure the useDriver flag, and possibly this entire function, will disappear altogether.
Ericson2314
force-pushed
the
explicit-drv-ness
branch
from
c189137
to
7bbca2f
Compare
|
Arguably this creates an inconsistency between store derivations and expressions that evaluate to a derivation. E.g. operates on the default outputs of |
|
This highlights the importance of being explicit about store level derivations and the expression-level "derivations" (which I propose to call packages instead, #6507). |
Ericson2314
changed the title
.drv special-casing for store path installable
Ericson2314
force-pushed
the
explicit-drv-ness
branch
from
7bbca2f
to
9a5cbca
Compare
src/libcmd/installables.cc
Outdated
| if (storePath.isDerivation()) { | ||
| auto oldDerivedPath = DerivedPath::Built { | ||
| .drvPath = storePath, | ||
| .outputs = OutputsSpec::All { }, | ||
| }; | ||
| warn( | ||
| "The interpretation of store paths arguments ending in `.drv` recently changed. If this command is now failing try again with '%s'", | ||
| oldDerivedPath.to_string(*store)); | ||
| }; |
There was a problem hiding this comment.
@thufschmitt says mention --derivation too
|
A possible resolution for the
|
|
Discussed in the Nix team meeting 2023-01-20:
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-01-20-nix-team-meeting-minutes-25/25432/1 |
|
Discussed in the Nix team meeting 2023-01-23:
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-01-23-nix-team-meeting-minutes-26/25433/1 |
thufschmitt
added
the
idea approved
|
Discussed in the Nix team meeting 2022-02-27:
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-02-27-nix-team-meeting-minutes-36/25890/1 |
Ericson2314
force-pushed
the
explicit-drv-ness
branch
from
9a5cbca
to
a21dc45
Compare
Ericson2314
force-pushed
the
explicit-drv-ness
branch
2 times, most recently
from
6001b3d
to
b263aae
Compare
The release notes document the change in behavior, I don't include it here so there is no risk to it getting out of sync. > Motivation >> Plumbing CLI should be simple Store derivation installations are intended as "plumbing": very simple utilities for advanced users and scripts, and not what regular users interact with. (Similarly, regular Git users will use branch and tag names not explicit hashes for most things.) The plumbing CLI should prize simplicity over convenience; that is its raison d'etre. If the user provides a path, we should treat it the same way not caring what sort of path it is. >> Scripting This is especially important for the scripting use-case. when arbitrary paths are sent to e.g. `nix copy` and the script author wants consistent behavior regardless of what those store paths are. Otherwise the script author needs to be careful to filter out `.drv` ones, and then run `nix copy` again with those paths and `--derivation`. That is not good! >> Surprisingly low impact Only two lines in the tests need changing, showing that the impact of this is pretty light. Many command, like `nix log` will continue to work with just the derivation passed as before. This because we used to: - Special case the drv path and replace it with it's outputs (what this gets rid of). - Turn those output path *back* into the original drv path. Now we just skip that entire round trip! > Context Issue NixOS#7261 lays out a broader vision for getting rid of `--derivation`, and has this as one of its dependencies. But we can do this with or without that. `Installable::toDerivations` is changed to handle the case of a `DerivedPath::Opaque` ending in `.drv`, which is new: it simply doesn't need to do any extra work in that case. On this basis, commands like `nix {show-derivation,log} /nix/store/...-foo.drv` still work as before, as described above. When testing older daemons, the post-build-hook will be run against the old CLI, so we need the old version of the post-build-hook to support that use-case. Co-authored-by: Travis A. Everett <travis.a.everett@gmail.com> Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Ericson2314
force-pushed
the
explicit-drv-ness
branch
from
b263aae
to
ea0adfc
Compare
|
Hi, what is the right way to use e.g. That last command (with |
|
@cole-h you seem to be witnessing some sort of bug. Is the drv path actually invalid? Is there some sort of eval store or other such thing going on? |
|
Well, the drv path doesn't exist on my machine, and I can't seem to fetch it with No eval store or any other funk. The most "strange" my setup is, is that I enable the I only posted here in case I was just holding it wrong, but it might be more valuable to move this to an issue if that is not the case. |
|
I think the
|
|
@raphaelr fixing this would be a great first contribution. Feel free to ping me in a PR or on Matrix if you get stuck. We're trying to make onboarding easier, and both the fix and feedback on the process are highly appreciated. |
ghost
mentioned this pull request
We no longer treat a store path different based on whether it ends in
.drv.To quote the release notes:
Motivation
Plumbing CLI should be simple
Store derivation installations are intended as "plumbing": very simple utilities for advanced users and scripts, and not what regular users interact with. (Similarly, regular Git users will use branch and tag names not explicit hashes for most things.)
The plumbing CLI should prize simplicity over convenience; that is its raison d'etre. If the user provides a path, we should treat it the same way not caring what sort of path it is.
Scripting
This is especially important for the scripting use-case. when arbitrary paths are sent to e.g.
nix copyand the script author wants consistent behavior regardless of what those store paths are. Otherwise the script author needs to be careful to filter out.drvones, and then runnix copyagain with those paths and--derivation. That is not good!Surprisingly low impact
Only two lines in the tests need changing, showing that the impact of this is pretty light.
Many command, like
nix logwill continue to work with just the derivation passed as before. This because we used to:Now we just skip that entire round trip!
Context
Issue #7261 lays out a broader vision for getting rid of
--derivation, and has this as one of its dependencies. But we can do this with or without that.Installable::toDerivationsis changed to handle the case of aDerivedPath::Opaqueending in.drv, which is new: it simply doesn't need to do any extra work in that case. On this basis, commands likenix {show-derivation,log} /nix/store/...-foo.drvstill work as before, as described above.When testing older daemons, the post-build-hook will be run against the old CLI, so we need the old version of the post-build-hook to support that use-case.
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.shsrc/*/testsSee the change log for user-facing details.
Only two lines in the tests need changing, showing that the impact of this is pretty light.
Progress towards #7261