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

Remove storetype delegate reg store -- contains #3736 #3829

Merged

Conversation

meditans
Copy link
Member

to each Store implementation. The generic regStore implementation will
only be for the ambiguous shorthands, like "" and "auto".

This also could get us close to simplifying the daemon command.

to each Store implementation. The generic regStore implementation will
only be for the ambiguous shorthands, like "" and "auto".

This also could get us close to simplifying the daemon command.
@@ -277,7 +278,9 @@ static int _main(int argc, char * * argv)
initPlugins();

if (stdio) {
if (getStoreType() == tDaemon) {
if (openUncachedStore().dynamic_pointer_cast<UDSRemoteStore>()) {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this cause the store to be opened twice in the non-UDSRemoteStore case? Maybe it's better to do store = openUncachedStore(); if (store.dynamic_pointer_cast ...) ... .

Copy link
Member

Choose a reason for hiding this comment

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

@edolstra Ah I remember now: I wanted to do that, but I wasn't sure how to destroy the opened store in the UDSRemoteStore before reopening it, since nix::ref is always non-null.

Copy link
Member

Choose a reason for hiding this comment

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

The connections are opened on demand, so this was never a problem, but the new way also uses the store that was opened/downcast so there is no duplicate work.

Removes duplicate websocket opening code, and also means we should be
able to to ssh-ssh-... daemon relays, not just uds-uds-... ones.
@roberth
Copy link
Member

roberth commented Sep 15, 2020

It seems like the diff still contains changes from #3736 (merged). Rebasing on master will make the review easier.

@Ericson2314
Copy link
Member

Thanks for catching @roberth! I can't edit the commit message but I did fix the diff.

@Ericson2314
Copy link
Member

@edolstra OK fixed up post-merge so this is ready again.

@edolstra edolstra merged commit 649d3aa into NixOS:master Sep 17, 2020
@Ericson2314 Ericson2314 deleted the remove-storetype-delegate-regStore branch September 17, 2020 15:45
@Ericson2314
Copy link
Member

Thanks!!

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.

None yet

4 participants