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

Add Store::buildPathsWithResults() #6221

Merged
merged 2 commits into from
Mar 9, 2022
Merged

Conversation

edolstra
Copy link
Member

@edolstra edolstra commented Mar 8, 2022

This function is like buildPaths(), except that it returns a vector of BuildResults containing the exact statuses and output paths of each derivation / substitution. This is convenient for functions like Installable::build(), because they then don't need to do another series of calls to get the outputs of CA derivations. It's also a precondition to impure derivations, where we can't query the output of those derivations since they're not stored in the Nix database.

Note that PathSubstitutionGoal can now also return a BuildStatus.

This function is like buildPaths(), except that it returns a vector of
BuildResults containing the exact statuses and output paths of each
derivation / substitution. This is convenient for functions like
Installable::build(), because they then don't need to do another
series of calls to get the outputs of CA derivations. It's also a
precondition to impure derivations, where we *can't* query the output
of those derivations since they're not stored in the Nix database.

Note that PathSubstitutionGoal can now also return a BuildStatus.
Comment on lines +179 to +181
void copyDrvsFromEvalStore(
const std::vector<DerivedPath> & paths,
std::shared_ptr<Store> evalStore);
Copy link
Member

Choose a reason for hiding this comment

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

I've been meaning to bring this up, can we call it "DrvStore" or similar in libstore? "Eval" is not a concept libstore should be aware of.

Also, can we do

Suggested change
void copyDrvsFromEvalStore(
const std::vector<DerivedPath> & paths,
std::shared_ptr<Store> evalStore);
void copyDrvsFromEvalStore(
const std::vector<DerivedPath> & paths,
Store & evalStore);

I am hoping to cut down on the extraneous use of shared pointers (for ergonomics, more than efficiency).

Copy link
Member Author

Choose a reason for hiding this comment

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

The only problem with that is that if you need a shared_ptr, it's hard to get it from a &.

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 8, 2022

@edolstra C.F. #4588 which was trying to get us closer to this this.

@thufschmitt was worry about the serialization helpers trapping us WRT protocol versions, but after starting #1165 I decided it would be better to completely separate the legacy and new protocols so protocol version could be passed to the serializers, ameliorating that issue.

If that sounds good, I'll do it after this lands.

@edolstra
Copy link
Member Author

edolstra commented Mar 8, 2022

Alternatively we could serialize BuildResult as JSON, like Realisation.

@Ericson2314
Copy link
Member

OK made the protocol versioning PR #6223

Alternatively we could serialize BuildResult as JSON, like Realisation.

Sure, but I don't think it matters that much. JSON is nice enough, sure, but it doesn't save us from all versioning considerations, just adding new fields to objects. I am completely ambivalent.

@edolstra edolstra merged commit 1c1a707 into master Mar 9, 2022
@edolstra edolstra deleted the build-paths-with-results branch March 9, 2022 13:37
@Ericson2314
Copy link
Member

Thanks for making that change, excited for this!

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-27/18433/1

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.

4 participants