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.

Sign up for GitHub

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

Jump to bottom

Make more string values work as installables #7601

Merged
merged 5 commits into from
May 15, 2023
Merged

Make more string values work as installables #7601

merged 5 commits into from
May 15, 2023

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?

@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
@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
@dpulls
Copy link

dpulls bot commented Apr 21, 2023

🎉 All dependencies have been resolved !

@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 marked this pull request as ready for review May 11, 2023 00:58
@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 requested 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.

src/libcmd/installable-attr-path.cc Outdated Show resolved Hide resolved
src/libcmd/installable-attr-path.cc Show resolved Hide resolved
src/libexpr/eval.cc Outdated Show resolved Hide resolved
@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
src/libexpr/eval.cc Outdated Show resolved Hide resolved
@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 🚀

src/libcmd/installable-value.hh Outdated Show resolved Hide resolved
src/libcmd/installable-value.hh Outdated Show resolved Hide resolved
tests/build.sh Show resolved Hide resolved
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 <roberth@users.noreply.github.com>
@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

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

@roberth roberth roberth approved these changes

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

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

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
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ericson2314 @roberth @nixos-discourse @tomberek @fricklerhandwerk