Skip to content

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

Merged
edolstra merged 7 commits intoNixOS:masterfrom
obsidiansystems:remove-storetype-delegate-regStore
Sep 17, 2020
Merged

Remove storetype delegate reg store -- contains #3736#3829
edolstra merged 7 commits intoNixOS:masterfrom
obsidiansystems:remove-storetype-delegate-regStore

Conversation

@meditans
Copy link
Copy Markdown
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.

if (stdio) {
if (getStoreType() == tDaemon) {
if (openUncachedStore().dynamic_pointer_cast<UDSRemoteStore>()) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@Ericson2314
Copy link
Copy Markdown
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
Copy Markdown
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.

4 participants