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

Low level <drvPath>^<outputName> installable syntax to match existing <highLevelInstallable>^<outputNames> syntax #4543

Merged
merged 30 commits into from
Dec 13, 2022

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Feb 12, 2021

#6426 added ^<output-names> to high level installables (flakes ones and attribute paths). This adds it to our low level installable --- a plain store path --- to match.

I think this is important because it greatly helps "complete" the expressive power of Nix's low-level "plumbing" commands, and doing that (and eventually stabilizing them) is a good cost / benefit win.

In addition:

One ingredient for #7261, but should be good even if we reject that issue.
Also important for RFC 92 (tracking issue #6316).

@Ericson2314 Ericson2314 force-pushed the indexed-store-path-outputs branch from 13ef605 to e020807 Compare February 13, 2021 06:10
@matthewbauer
Copy link
Member

I think CmdProfileInstall also needs to change to reflect this:

https://github.com/NixOS/nix/blob/master/src/nix/profile.cc#L267-L270

so only outputs listed in BuildableFromDrv "outputs" are installed.

@thufschmitt
Copy link
Member

I'm not sure what this brings implementation-wise compared to #4494 (except obviously for the doc and test which are obviously missing in the latter). In particular it seems to me that having an extra Installable type just for that is a lot of boilerplate for not much benefit. Or do I miss something?

@Ericson2314
Copy link
Member Author

@regnat I'm afraid I didn't even realize you had a PR this did this!

@Ericson2314
Copy link
Member Author

@regnat I think the simpler and pure (no DB access) toBuildables is notable. (To be clear, yours made the problem no worse, just inherited the complexity) I am very skeptical of all the complexity we have today (along with --derivation) and so hope this can be the beginning of getting rid of it.

@Ericson2314 Ericson2314 force-pushed the indexed-store-path-outputs branch 3 times, most recently from 093a8ee to 8e2206e Compare February 19, 2021 15:39
@Ericson2314 Ericson2314 force-pushed the indexed-store-path-outputs branch 2 times, most recently from 03729eb to 15d9d45 Compare February 28, 2021 00:22
@Ericson2314 Ericson2314 force-pushed the indexed-store-path-outputs branch from 15d9d45 to 8d36d96 Compare March 6, 2021 06:23
@Ericson2314 Ericson2314 mentioned this pull request Mar 10, 2021
4 tasks
@dpulls
Copy link

dpulls bot commented Apr 5, 2021

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 force-pushed the indexed-store-path-outputs branch from cece11c to 57e90af Compare April 6, 2021 05:14
Being conservative and only doing a single output name for now.
@Ericson2314 Ericson2314 force-pushed the indexed-store-path-outputs branch from 57e90af to 8499f32 Compare April 6, 2021 14:25
@fogti
Copy link
Contributor

fogti commented Apr 6, 2021

This doesn't solve the shell 'escape needed' problems. I'm unsure if ! is a wise choice. It shows unwanted behavoir e.g. in bash:

$ nix profile install nixpkgs!lol
bash: !lol: event not found

(e.g. similiar to #4686, but with ! instead of #)

@Ericson2314
Copy link
Member Author

Yeah we didn't settle on a surface syntax yet.

@fogti
Copy link
Contributor

fogti commented Oct 1, 2021

Currently, this syntax is (already) used with CA-drvs, which leads to problems with nix-collect-garbage, because it terminates when it finds any store which uses that syntax. Maybe this should be fixed in a smaller PR earlier if possible, because this one seems to take another while until merged (and I'm not sure if this PR does really fix that annoyance).

@thufschmitt
Copy link
Member

Currently, this syntax is (already) used with CA-drvs, which leads to problems with nix-collect-garbage, because it terminates when it finds any store which uses that syntax

Unless I missed something, these have only been created because of a now-closed bug (#5176), and it only concerns lockfiles. Is there more of these?

@Ericson2314
Copy link
Member Author

Yeah this has nothing to do with that one, which @regnat fixed. This is just making an internal syntax exposed to the user.

In this case, we in fact rely on ! being disallowed in store paths, so there is no ambiguity.

@Ericson2314
Copy link
Member Author

I should also mention that nix-store -r /nix/store/3v8v9alqxfhif646mrvrafv2hmw60ypx-cabal2nix-reflex.drv!out is already valid syntax, hah.

@tomberek
Copy link
Contributor

To clarify, this is existing syntax that happens to not function with installables, but would otherwise be allowed. Would this leak into things like profiles or machine-readable output?

@Ericson2314
Copy link
Member Author

This is preexing syntax for some legacy commands (e.g. nix-build) and used internally (though I modernized it). Basically, see DerivedPath and StorePathWithOutputs in the code base, I have made the former replace the latter where possible.

This specific change won't leak anywhere, this is just changing the CLI syntax for installables. In way, the leaking behind the scenes is already done! --- thanks to those DerivedPath refactors underneath the hood.

This, or something like this, is part of RFC 92, so we should figure out what the experimental feature is called and get this merged. I had been waiting to hopefully add some more tests firsts for the status quo first, but if that is controversial we should just go ahead with this and the other parts of RFC 92.

@Ericson2314 Ericson2314 force-pushed the indexed-store-path-outputs branch from e7e1fe4 to e5c42bb Compare March 10, 2022 17:10
@Ericson2314 Ericson2314 changed the title New low level "indexed" drv file installable syntax: <drvPath>^<outputName> Low level <drvPath>^<outputName> installable syntax to match existing <highLevelInstallable>^<outputNames> syntax Nov 25, 2022
@fricklerhandwerk
Copy link
Contributor

Discussed in Nix team meeting 2022-11-28:

  • @fricklerhandwerk: what are installables anyway?
    • @Ericson2314: when you do nix build foo bar baz, each of the arguments could be
      • store derivation path
      • store path
        • which could be not built but substitutable
      • path to Nix expression
      • flake reference
    • @Ericson2314: we can already do nix build nixpkgs#foo^man (i.e., flake references), but not for store paths such as nix build /nix/store/<hash>-foo^man.
    • Are there any objections that we want to have this or the implementation strategy?
      • agreement on both

-> to review

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2022-11-28-nix-team-meeting-minutes-12/23730/1

req(storePath.isDerivation()
? (DerivedPath) DerivedPath::Built {
.drvPath = std::move(storePath),
.outputs = {},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note it previously read the derivation and returned all its output names explicitly here, but that didn't work e.g. when the drv file itself needs to be downloaded and so we cannot yet read it.

doc/manual/src/release-notes/rl-next.md Outdated Show resolved Hide resolved
src/nix/nix.md Outdated Show resolved Hide resolved
src/nix/nix.md Outdated Show resolved Hide resolved
src/nix/nix.md Outdated Show resolved Hide resolved
src/nix/nix.md Outdated Show resolved Hide resolved
Ericson2314 and others added 3 commits December 12, 2022 17:37
Thanks!

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
@Ericson2314
Copy link
Member Author

OK @edolstra, I think all review from today is now addressed!

@edolstra edolstra merged commit e2a4e7a into NixOS:master Dec 13, 2022
@Ericson2314 Ericson2314 deleted the indexed-store-path-outputs branch December 13, 2022 16:23
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2022-12-12-nix-team-meeting-minutes-16/24119/1

@ghost
Copy link

ghost commented Nov 17, 2023

Hey I know this is the bikesheddiest of bikesheds, but I just realized we could have used / instead of ^:

nix build /nix/store/09wshq4g5mc2xjx24wmxlw018ly5mxgl-bash-5.2-p15.drv/man

... since Nix strictly forbids a top-level store path which isn't a derivation from having the suffix .drv. Since derivations are files, not directories, the string above can never be a valid filesystem path.

Who cares, you ask? Maybe it's just me, but the 6 key on a QWERTY keyboard is farther from the home row position than any other key, meaning that 6 and ^ are mistyped more often than any other character.

Like I said, bikesheds. Glad I got that out of my system. We now return you to your regularly-scheduled software development...

@Ericson2314
Copy link
Member Author

@amjoseph-nixpkgs I think some command allow you specify a store path with trailing path relative to it? It could get a git confusing in that case. On a conceptual level I want /nix/store/09wshq4g5mc2xjx24wmxlw018ly5mxgl-bash-5.2-p15.drv^man/share/man to work too, but I realize that can also be confusing and maybe it's better to stick to some sort of ${...} notation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants