-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Upgrade downstreamPlaceholder
to a type with methods
#8353
Upgrade downstreamPlaceholder
to a type with methods
#8353
Conversation
b279a0f
to
d0d7b49
Compare
OK think I addressed everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ lgtm. My suggestions to use both file static and class static were incoherent, but you resolved that well.
I assume we will later have a use for this type, and not just the construct + render combination?
Also just to be sure, what's the stability status of these hashes? I would hope they are meant to be stable but a change (if we were to accidentally make one) would rarely be observed by users? Or am I way off? Wouldn't nix eval
print them? Is this documented somewhere? Do we have a test that checks the placeholder against a hardcoded example to make sure it doesn't change by accident?
Yes, once
Yes they will end up in derivations (in memory and drv files, so they shouldn't change). If they do change, trying to build old ca or dynamic-derivation-depending derivations could fail because they no longer get substituted. We could write some tests, certainly. Don't think there are interesting properties to test, but we could just put in a fixed case of each. (Similarly we should pin down the hash-modulo stuff at some point too.) |
This is good in general, but in particular ensures when we heavily refactor it in the next commit there is less likelihood for an unintentional change in behavior to sneak in.
This gets us ready for dynamic derivation dependencies (part of RFC 92).
d0d7b49
to
b9e5ce4
Compare
I added some tests. The first I added test prior to the refactor so we can see that behavior is preserved. |
Motivation
This gets us ready for dynamic derivation dependencies (part of RFC 92).
Context
Tracking issue for RFC 92 #6316
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*
Priorities
Add 👍 to pull requests you find important.