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

Decode string context straight to using StorePaths #6237

Merged
merged 2 commits into from
Mar 22, 2022

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 12, 2022

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

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
Copy link
Member Author

@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.

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

Ericson2314 commented Mar 18, 2022

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

@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

Thanks!

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants