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

Introduce nix derivation instantiate #11506

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

Conversation

layus
Copy link
Member

@layus layus commented Sep 15, 2024

This is similar to nix-instantiate with support for installables (and flakes).

Motivation

The ability to obtain a derivation with the new cli has never been really implemented. #3908 (comment) is the best work-around, but it does not add a gc root for the generated paths.

This brings a dedicated entry-point with the right semantics instead of the workaround above.

Context

Fixes #3908

To some extent, this also addresses #7138 because outputs can easily be rooted. This brings that capability to store derivations.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added documentation new-cli Relating to the "nix" command labels Sep 15, 2024
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Looks overall like a clean subset of nix-instantiate.
However some integration test would be nice and should be easy to add (with and without JSON).

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

I think instantiate should not be placed under the derivation namespace. As I see it, derivation commands should operate on derivations and should not have to know about expressions/flakes/installables. This command however operates on installables to produce a derivation.

See also #7868 and #7903, cc @Ericson2314

@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Sep 16, 2024
@layus
Copy link
Member Author

layus commented Sep 16, 2024

@fgaz All right, but where then ?

@layus
Copy link
Member Author

layus commented Sep 16, 2024

The best place I can think of right now is nix build --derivation. But one may prefer nix instantiate. This is a trivial change, but how to get an agreement ? Should I organize a poll ?

@fgaz
Copy link
Member

fgaz commented Sep 16, 2024

The best place I can think of right now is nix build --derivation. But one may prefer nix instantiate.

Both look fine to me. Another option is nix eval --derivation (eval instantiates the derivation from the expression anyway, it just isn't able to print it because .drvPath is an implementation detail).

This is a trivial change, but how to get an agreement ? Should I organize a poll ?

I think for now we can just wait for a nix maintainer to give their opinion.

@layus layus changed the title Introduce nix derivation instantiate Introduce nix instantiate Sep 16, 2024
@edolstra
Copy link
Member

I think nix derivation instantiate is preferable since we don't want to pollute the top-level namespace with a utility command like this. Also, maybe it should be nix derivation create to avoid the "instantiate" terminology from the old CLI. (Or nix derivation add, but that one is already taken. It should really have been called nix derivation import.)

BTW, the reason that the new CLI doesn't have an equivalent for nix-instantiate is because ideally we would get rid of store derivations in the future. The Nix client should just send an in-memory representation of derivations to the daemon, without the need to materialize them on disk. So the fewer places where we expose .drvs in the new CLI, the better.

@roberth
Copy link
Member

roberth commented Sep 19, 2024

So the fewer places where we expose .drvs in the new CLI, the better.

We're already doing a good job at this, but in this new scenario, we'll still need the functionality of writing .drvs somewhere not in memory, for analysis by other tools, such as nix-diff. While all other commands could skip the instantiation into the actual store, this command could be used to do it anyway.
Alternatively or in addition, this functionality could be added to all commands by means of a flag, such as --instantiate and/or --instantiation-store.
Before any of that, we have to be sure whether instantiation means the act of computing the derivations (hashing them, etc), which is quite independent of evaluating the derivation arguments, especially when we have hash placeholder string thunks, or whether it is the act of writing them out in a somewhat user facing way.
Or do we define instantiation as computing the derivation of the outPath to further diminish the role of actual drv files? That is in contrast to nix eval which does not care about derivations and outputs any more than the language itself does.
(I assumed #6507; s/outPath/drvPath/ otherwise.)

@layus
Copy link
Member Author

layus commented Sep 25, 2024

Should it be named nix derivation add-to-store or nix derivation store ? These would be more explicit that it is going to write the derivation to the store, right ?
I understand the desire to move away from .drv files, but these are a core part of many workflows and there is still no way to properly write one int the store and get a gc-root on it.

Should I add the feature to the old nix-instantiate command instead ?

@roberth
Copy link
Member

roberth commented Sep 30, 2024

Hold on, shouldn't nix derivation commands be store-level only?
Not 100% sure we decided that. @Ericson2314 would know?

@edolstra
Copy link
Member

Hold on, shouldn't nix derivation commands be store-level only?

No, I think commands should work on as many installable types as possible. We definitely want people to be able to say nix derivation instantiate nixpkgs#hello (just like they can currently do nix derivation show nixpkgs#hello).

@tomberek tomberek added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Sep 30, 2024
@tomberek
Copy link
Contributor

tomberek commented Sep 30, 2024

Discussed in meeting:

  • rename to nix derivation instantiate

@layus layus changed the title Introduce nix instantiate Introduce nix derivation instantiate Sep 30, 2024
@layus
Copy link
Member Author

layus commented Sep 30, 2024

@tomberek Thanks for making a decision. I updated the PR to reflect the naming choice.
It seems there may have been a second point, but you did not fill it. Did you discuss anything else ?

@layus
Copy link
Member Author

layus commented Sep 30, 2024

Oh, just rebased, that was probably the second point.

@Ericson2314
Copy link
Member

nix derivation should be store-layer only. However, this could technically pass if the idea is that store-layer commands still get transparently "upgraded" in full Nix into talking "full" installables and not just "store layer installables". We have gone back-and-forth on that.

@Mic92
Copy link
Member

Mic92 commented Oct 1, 2024

@tomberek Thanks for making a decision. I updated the PR to reflect the naming choice. It seems there may have been a second point, but you did not fill it. Did you discuss anything else ?

No. There was none.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-09-30-nix-team-meeting-minutes-182/53814/1

@7c6f434c
Copy link
Member

7c6f434c commented Oct 3, 2024

We're already doing a good job at this, but in this new scenario, we'll still need the functionality of writing .drvs somewhere not in memory, for analysis by other tools, such as nix-diff.

Also, derivations provide some natural workflows for build-dependency GC-pinning.

src/nix/derivation-instantiate.md Outdated Show resolved Hide resolved
src/nix/derivation-instantiate.md Outdated Show resolved Hide resolved
return res;
}

// TODO deduplicate with other code also setting such out links.
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to deduplicate this, but that can also be done in a future PR.

Copy link
Member

@edolstra edolstra Oct 8, 2024

Choose a reason for hiding this comment

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

See 7f6d006 which factors out createOutLinks().

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-10-07-nix-team-meeting-minutes-184/54046/1

This is simmilar to `nix-instantiate` with support for installables (and flakes).
@layus
Copy link
Member Author

layus commented Oct 7, 2024

I have reworked output. It now uses a list instead of a set. Documentation and tests updated accordingly.

(also rebased, because I thought that may fix running tests in meson, but no. I still need to run meson --reconfigure to make meson test pick up changes to files in test/functional. Weird)

@edolstra
Copy link
Member

edolstra commented Oct 8, 2024

I still need to run meson --reconfigure to make meson test pick up changes to files in test/functional. Weird)

@Ericson2314 ^

@Ericson2314
Copy link
Member

@layus

This clashes with RFC 92 a bit, because it is unclear what it should do in the case of derivations that are themselves the build outputs of derivations.

We don’t want to do this now in light of those uncertainties, and would much prefer it if you could work on the plan outlined in #7261

We would also appreciate advice on how to make issues like this more prominant so contributors like you don’t end up spending a bunch of time implementing something that overlaps with an agreed-upon plan that’s just waiting to be implemented.

Sorry for the confusing discussion response from us on this, and the delayed disapproval.

@layus
Copy link
Member Author

layus commented Oct 10, 2024

[edit] I do not think this message was constructive or useful [/edit]

@Ericson2314
Copy link
Member

Ericson2314 commented Oct 11, 2024

In merely linking #7261 I also buried the lede: with the ^.. installable syntax proposed there

nix derivation instantiate foo

should be the same thing as

nix build foo^..

The latter is, we freely admit, not so discoverable, and reveals once again how the build verb is confusing and not really a good choice. However it does provide this functionality, and in the 92 case, additional things like

nix build foo^..^..

which are not expressible with this command.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-10-09-nix-team-meeting-minutes-185/54335/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

nixFlakes: can't figure out nix-instantiate equivalent
9 participants