Skip to content

Comments

Decode string context straight to using StorePaths #6237

Merged
edolstra merged 2 commits intoNixOS:masterfrom
obsidiansystems:store-path-string-context
Mar 22, 2022
Merged

Decode string context straight to using StorePaths #6237
edolstra merged 2 commits intoNixOS:masterfrom
obsidiansystems:store-path-string-context

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 12, 2022 •
edited
Loading

I gather decoding happens on demand, so I hope don't think this should
have any perf implications one way or the other.

@Ericson2314 Ericson2314 mentioned this pull request Mar 14, 2022
Merged
thufschmitt
thufschmitt reviewed Mar 18, 2022
View reviewed changes
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

The last commit looks good to me. Apart from the potential conflicts, is there any reason for this to depend on #6236 ?

@Ericson2314 Ericson2314 mentioned this pull request Mar 18, 2022
Closed
@Ericson2314
Copy link
Member Author

Ericson2314 commented Mar 18, 2022

@thufschmitt It is probably not necessary, but I was a bit nervous about making a Store more accessible to the eval cache, because I didn't want to risk some any bit of store state introducing impurities.

@Ericson2314 Ericson2314 force-pushed the store-path-string-context branch from 59fa30e to d60a76d Compare March 18, 2022 15:24
@Ericson2314
Decode string context straight to using StorePaths
4d6a380 
I gather decoding happens on demand, so I hope don't think this should
have any perf implications one way or the other.
@Ericson2314 Ericson2314 force-pushed the store-path-string-context branch from d60a76d to 4d6a380 Compare March 18, 2022 15:38
@Ericson2314
Copy link
Member Author

Ericson2314 commented Mar 18, 2022 •
edited
Loading

Squashed out the second commit so there is is no StoreDirConfig config in the history. (I will make it part of that PR.)

@dpulls
Copy link

dpulls bot commented Mar 21, 2022

🎉 All dependencies have been resolved !

@Ericson2314
Copy link
Member Author

Ericson2314 commented Mar 21, 2022 •
edited
Loading

@edolstra how is this? The core change is the switch to StorePath.

The auxillary change is new names for strong contexts, but I think this is good:

  1. Matches SearchPath and SearchPathElem.

  2. Allows changes like the Path to StorePath to be done easily, "catch" all the std::pairs that need to be so changed via variable substitution. For RFC 92 (Dynamic derivations RFC 92 #4628) I will need to again modify NixStringContextElem, so this isn't even that hypothetical a benefit after this one specific instance of changing the definition.

@edolstra edolstra merged commit e4ff430 into NixOS:master Mar 22, 2022
@edolstra
Copy link
Member

edolstra commented Mar 22, 2022

Thanks!

@Ericson2314 Ericson2314 deleted the store-path-string-context branch March 22, 2022 15:34
@Ericson2314
Copy link
Member Author

Ericson2314 commented Mar 22, 2022

Thank you!

@Ericson2314 Ericson2314 mentioned this pull request Apr 19, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@thufschmitt thufschmitt thufschmitt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Ericson2314 @edolstra @thufschmitt