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

Document "Import From Derivation" #7332

Merged
merged 15 commits into from
Sep 26, 2023

Conversation

fricklerhandwerk
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk commented Nov 22, 2022

The term "Import From Derivation" is historic and does not represent the concept well.

This change does not address the adequacy of term, and only explains the concept. Reviewers: Please assess the correctness and completeness of the description and how easy it is to understand.

Renaming can be done in a follow-up PR.

This work is sponsored by Antithesis

Related: #8450 (but won't fix here)

@edolstra
Copy link
Member

IMHO a glossary is not the right place for this kind of documentation. A glossary is a dictionary-like list of short descriptions, which can of course link to more extensive docs elsewhere. "Importing from a derivation" should probably be its own section in the Nix language chapter.

@Ericson2314
Copy link
Member

Ericson2314 commented Nov 22, 2022

Also, I think it is high time that we stop calling it "import from derivation"

  1. We're not importing a derivation. You can actually do that, and if the drv is created at eval time (i.e. no RFC 92 stuff) it is not IFD! We are instead importing a derivation output or build result, and it is that that requires building.
  2. It is not just import. builtins.readFile and other "input ingesting"-related primops are equally involved.

I would therefore call the section "reading build results", and just mention within the "import from derivation" term as something people do say that is misleading.

@Ericson2314
Copy link
Member

Also I still don't like "realise": it is an English word without meaning, pure nonsense jargon. I think "built" is fine; even if we didn't do the building someone else did.

Passive language can help here

  • "We need to build something" OK perhaps it sounds like we need to do the building ourselves.
  • "We need something built" Great, it is clear that the task can be delegated to remote builder or retrieved from a cache.
  • "We need to get a build result" Great, it clear what we need, and how we get it is left open-ended as it should be.

@fricklerhandwerk
Copy link
Contributor Author

fricklerhandwerk commented Nov 23, 2022

it is high time that we stop calling it "import from derivation"

@Ericson2314 @edolstra I agree, but renaming the concept makes this change much harder: we already have a configuration option allow-import-from-derivation which would need to be adapted, too. All I wanted to do is document the idea. I can do a follow-up to use a better name and a renaming strategy, which we can discuss in isolation.

I closed all the comments regarding the term itself.

We're not importing a derivation.

I avoided making explicit note of the historicity of the term, and focused on explaining the concept behind it instead. My request to reviewers is to check that the conceptual description is correct.

It is not just import. builtins.readFile and other "input ingesting"-related primops are equally involved.

This part already accounts for that fact.

I still don't like "realise": it is an English word without meaning, pure nonsense jargon.

I closed all the comments related to #7318 and #7320. Let us discuss those in isolation, please.

src/libexpr/eval.hh Outdated Show resolved Hide resolved
doc/manual/src/language/import-from-derivation.md Outdated Show resolved Hide resolved
doc/manual/src/language/import-from-derivation.md Outdated Show resolved Hide resolved
doc/manual/src/language/import-from-derivation.md Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-40/23480/1

@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-19-nix-team-meeting-minutes-18/24121/1

@fricklerhandwerk fricklerhandwerk marked this pull request as draft January 19, 2023 14:37
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/how-to-builtins-readdir-a-derivationss-subdir-when-the-subdirs-path-contains-a-symlink-to-another-derivations-directory/24987/5

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-06-05-documentation-team-meeting-notes-52/28937/1

@fricklerhandwerk fricklerhandwerk marked this pull request as ready for review July 13, 2023 21:09
@fricklerhandwerk
Copy link
Contributor Author

I know we discussed renaming IFD (proposal: Read From Realisation), but let's first agree on the documentation.

@fricklerhandwerk
Copy link
Contributor Author

This should be ready to merge.

@Ericson2314 Ericson2314 changed the title document "Import From Derivation" Document "Import From Derivation" Sep 5, 2023
output path is not really a thing, since it's really just the store
path to a derivation output.
@fricklerhandwerk
Copy link
Contributor Author

Addressed all comments. Let's fix forward from here.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Very nice! Here's some small things I noticed

doc/manual/src/language/import-from-derivation.md Outdated Show resolved Hide resolved
@fricklerhandwerk fricklerhandwerk enabled auto-merge (squash) September 26, 2023 01:02
@fricklerhandwerk fricklerhandwerk merged commit b17f200 into NixOS:master Sep 26, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Document allow-import-from-derivation in detail
7 participants