Skip to content

Comments

Make more string values work as installables#7601

Merged
roberth merged 5 commits intoNixOS:masterfrom
obsidiansystems:string-installables
May 15, 2023
Merged

Make more string values work as installables#7601
roberth merged 5 commits intoNixOS:masterfrom
obsidiansystems:string-installables

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jan 14, 2023 •
edited
Loading

Motivation

As discussed in #7417, it would be good to make more string values work as installables. That is to say, if an installable refers to a value, and the value is a string, it used to not work at all, since #7484, it works somewhat, and this PR make it work some more.

The new cases that are added for BuiltPath contexts:

  • Fixed input- or content-addressed derivation:

    nix-repl> hello.out.outPath
"/nix/store/jppfl2bp1zhx8sgs2mgifmsx6dv16mv2-hello-2.12"

nix-repl> :p builtins.getContext hello.out.outPath
{ "/nix/store/c7jrxqjhdda93lhbkanqfs07x2bzazbm-hello-2.12.drv" = { outputs = [ "out" ]; }; }

The string matches the specified single output of that derivation, so it should also be valid.

  • Floating content-addressed derivation:

    nix-repl> (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath
"/1a08j26xqc0zm8agps8anxpjji410yvsx4pcgyn4bfan1ddkx2g0"

nix-repl> :p builtins.getContext (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath
{ "/nix/store/qc645pyf9wl37c6qvqzaqkwsm1gp48al-hello-2.12.drv" = { outputs = [ "out" ]; }; }

    The string is not a path but a placeholder, however it also matches the context, and because it is a CA derivation we have no better option. This should also be valid.

Context

See #7417, where these ideas were discussed.

The history is broken out to be legible. One can review commit-by-commit to see all the support infra put in place before it is finally used in the last commit.

We may also want to think about richer attrset based values (also discussed in that issue and #6507), but this change "completes" our string-based building blocks, from which the others can be desugared into or at least described/document/taught in terms of.

Depends on #7710

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

@roberth
Copy link
Member

roberth commented Jan 23, 2023 •
edited
Loading

Seems like this would solve #7654 partially? Maybe allow the whole string to be printed in the repl and CLI?

tomberek
tomberek reviewed Jan 27, 2023
View reviewed changes
Ericson2314
Ericson2314 commented Jan 27, 2023
View reviewed changes
tomberek
tomberek reviewed Jan 27, 2023
View reviewed changes
tomberek
tomberek reviewed Jan 27, 2023
View reviewed changes
Ericson2314
Ericson2314 commented Jan 27, 2023
View reviewed changes
Ericson2314
Ericson2314 commented Jan 27, 2023
View reviewed changes
Ericson2314
Ericson2314 commented Jan 27, 2023
View reviewed changes
Ericson2314
Ericson2314 commented Jan 27, 2023
View reviewed changes
@Ericson2314 Ericson2314 marked this pull request as draft January 27, 2023 18:05
@fricklerhandwerk fricklerhandwerk added the store Issues and pull requests concerning the Nix store label Mar 3, 2023
@github-actions github-actions bot added with-tests Issues related to testing. PRs with tests have some priority and removed store Issues and pull requests concerning the Nix store labels Apr 20, 2023
@Ericson2314 Ericson2314 force-pushed the string-installables branch from f57d7f4 to aeb330a Compare April 20, 2023 04:06
@github-actions github-actions bot added new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger labels Apr 20, 2023
@Ericson2314 Ericson2314 force-pushed the string-installables branch from aeb330a to 62ccb73 Compare April 21, 2023 05:11
@dpulls
Copy link

dpulls bot commented Apr 21, 2023

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 force-pushed the string-installables branch from 62ccb73 to d64ad07 Compare April 21, 2023 06:18
@github-actions github-actions bot removed repl The Read Eval Print Loop, "nix repl" command and debugger new-cli Relating to the "nix" command labels Apr 21, 2023
@Ericson2314 Ericson2314 force-pushed the string-installables branch from d64ad07 to 7303910 Compare May 11, 2023 00:57
@Ericson2314 Ericson2314 marked this pull request as ready for review May 11, 2023 00:58
@Ericson2314 Ericson2314 force-pushed the string-installables branch from 7303910 to 153952e Compare May 11, 2023 01:32
@Ericson2314 Ericson2314 added the RFC Related to an accepted RFC label May 11, 2023
@Ericson2314 Ericson2314 mentioned this pull request May 11, 2023
Open
4 tasks
roberth
roberth suggested changes May 11, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Needs a bit of cleanup.

@Ericson2314 Ericson2314 force-pushed the string-installables branch 2 times, most recently from 401d471 to ee521ab Compare May 12, 2023 13:03
tomberek
tomberek reviewed May 12, 2023
View reviewed changes
@Ericson2314 Ericson2314 force-pushed the string-installables branch 2 times, most recently from 8aae7bf to 4614303 Compare May 12, 2023 14:19
@Ericson2314 Ericson2314 mentioned this pull request May 12, 2023
Merged
4 tasks
roberth
roberth reviewed May 15, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Almost there 🚀

Ericson2314 and others added 5 commits May 15, 2023 09:03
@Ericson2314
Remove unneeded argument for mkOutputString
9550c38
@Ericson2314
Split mkOutputString in two
0a9afce 
This well help us with some unit testing
@Ericson2314
Expose mkOutputString as method of EvalState
8e1a990
@Ericson2314
Create EvalState::coerceToDerivedPath
5a23b80 
This gives us some round trips to test.

`EvalState::coerceToDerivedPathUnchecked` is a factored out helper just
for unit testing.
@Ericson2314 @roberth
Make more string values work as installables
d2162e7 
As discussed in NixOS#7417, it would be good to make more string values work
as installables. That is to say, if an installable refers to a value,
and the value is a string, it used to not work at all, since NixOS#7484, it
works somewhat, and this PR make it work some more.

The new cases that are added for `BuiltPath` contexts:

- Fixed input- or content-addressed derivation:

  ```
  nix-repl> hello.out.outPath
  "/nix/store/jppfl2bp1zhx8sgs2mgifmsx6dv16mv2-hello-2.12"

  nix-repl> :p builtins.getContext hello.out.outPath
  { "/nix/store/c7jrxqjhdda93lhbkanqfs07x2bzazbm-hello-2.12.drv" = { outputs = [ "out" ]; }; }

  The string matches the specified single output of that derivation, so
  it should also be valid.

- Floating content-addressed derivation:

  ```
  nix-repl> (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath
  "/1a08j26xqc0zm8agps8anxpjji410yvsx4pcgyn4bfan1ddkx2g0"

  nix-repl> :p builtins.getContext (hello.overrideAttrs (_: { __contentAddressed = true; })).out.outPath
  { "/nix/store/qc645pyf9wl37c6qvqzaqkwsm1gp48al-hello-2.12.drv" = { outputs = [ "out" ]; }; }
  ```

  The string is not a path but a placeholder, however it also matches
  the context, and because it is a CA derivation we have no better
  option. This should also be valid.

We may also want to think about richer attrset based values (also
discussed in that issue and NixOS#6507), but this change "completes" our
string-based building blocks, from which the others can be desugared
into or at least described/document/taught in terms of.

Progress towards NixOS#7417

Co-authored-by: Robert Hensing <[email protected]>
@Ericson2314 Ericson2314 force-pushed the string-installables branch from d7a5e35 to d2162e7 Compare May 15, 2023 13:06
@roberth roberth enabled auto-merge May 15, 2023 13:33
@roberth roberth merged commit 0c49c1a into NixOS:master May 15, 2023
@nixos-discourse
Copy link

nixos-discourse commented May 15, 2023

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

https://discourse.nixos.org/t/2023-05-12-nix-team-meeting-minutes-54/28197/1

@Ericson2314 Ericson2314 deleted the string-installables branch May 15, 2023 13:57
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Jun 27, 2023
@Ericson2314
Get rid of the --derivation flag
e35385e 
Since PR NixOS#7601, we've had enough flexibility in our types of
installables that it is no longer needed.

Progress towards NixOS#7261
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Jun 27, 2023
@Ericson2314
Get rid of the --derivation flag
4f712fe 
Since PR NixOS#7601, we've had enough flexibility in our types of
installables that it is no longer needed.

Progress towards NixOS#7261
@Ericson2314 Ericson2314 mentioned this pull request Jun 27, 2023
Draft
8 tasks
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Jun 27, 2023
@Ericson2314
Get rid of the --derivation flag
2f3e9e9 
Since PR NixOS#7601, we've had enough flexibility in our types of
installables that it is no longer needed.

Progress towards NixOS#7261
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Jul 31, 2023
@Ericson2314
Get rid of the --derivation flag
8c18ed7 
Since PR NixOS#7601, we've had enough flexibility in our types of
installables that it is no longer needed.

Progress towards NixOS#7261
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Oct 17, 2023
@Ericson2314
Get rid of the --derivation flag
05f4679 
Since PR NixOS#7601, we've had enough flexibility in our types of
installables that it is no longer needed.

Progress towards NixOS#7261
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Oct 23, 2023
@Ericson2314
Get rid of the --derivation flag
248993d 
Since PR NixOS#7601, we've had enough flexibility in our types of
installables that it is no longer needed.

Progress towards NixOS#7261
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Oct 23, 2023
@Ericson2314
Get rid of the --derivation flag
ff207e6 
Since PR NixOS#7601, we've had enough flexibility in our types of
installables that it is no longer needed.

Progress towards NixOS#7261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomberek tomberek tomberek left review comments

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

+1 more reviewer

@roberth roberth roberth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tomberek tomberek

@roberth roberth

Labels

RFC Related to an accepted RFC with-tests Issues related to testing. PRs with tests have some priority

Projects

Nix team
Archived in project

Milestone

dynamic-derivations stabilisation

Development

Successfully merging this pull request may close these issues.

5 participants

@Ericson2314 @roberth @nixos-discourse @tomberek @fricklerhandwerk