Ensure we can construct remote store configs in isolation #11108
Conversation
github-actions
bot
added
the
store
Ericson2314
force-pushed
the
remote-store-constructors
branch
from
4b8204e to
dfa9add
Compare
| @@ -143,6 +143,7 @@ | |||
| ''^src/libstore/common-protocol-impl\.hh$'' | |||
| ''^src/libstore/common-protocol\.cc$'' | |||
| ''^src/libstore/common-protocol\.hh$'' | |||
| ''^src/libstore/common-ssh-store-config\.hh$'' | |||
There was a problem hiding this comment.
why not apply formatting?
There was a problem hiding this comment.
We don't yet have a plan for how we want to format existing files (and this is a renamed file) --- e.g. creating conflicts with PRs, cherry-picks between us and Lix, etc. makes for some real downsides.
There was a problem hiding this comment.
there is already a clang-format; I find this take a bit odd.
Format and move on my take.
| @@ -1,6 +1,6 @@ | |||
| #include <regex> | |||
|
|
|||
| #include "ssh-store-config.hh" | |||
| #include "common-ssh-store-config.hh" | |||
There was a problem hiding this comment.
I would wait for such things until we've fully switched to Meson -- the subdirectories we have today are not done in a consistent manner and mostly stuff is flat.
| std::string_view scheme, | ||
| std::string_view host, | ||
| const Params & params) | ||
| : StoreConfig(params) |
There was a problem hiding this comment.
should this validate it's ssh-ng ?
There was a problem hiding this comment.
Oh can't do that because its mounted-ssh-ng for subclass (which itself is bad, but we cannot yet fix)
Ericson2314
force-pushed
the
remote-store-constructors
branch
from
dfa9add to
395108e
Compare
Progress towards NixOS#10766 I thought that NixOS#10768 achieved, but when I went to use this stuff (in Hydra), turns out it did not. (Those `using FooConfig;` lines were not working --- they are so finicky!) This PR gets the job done, and adds some trivial unit tests to make sure I did what I intended. I had to add add a header to expose `SSHStoreConfig`, after which the preexisting `ssh-store-config.*` were very confusingly named files, so I renamed them to `common-ssh-store-config.hh` to match the type defined therein.
Ericson2314
force-pushed
the
remote-store-constructors
branch
from
395108e to
808082e
Compare
|
|
||
| namespace nix { | ||
|
|
||
| struct SSHStoreConfig : virtual RemoteStoreConfig, virtual CommonSSHStoreConfig |
There was a problem hiding this comment.
Any comments here you can add?
I'm not familiar with what makes this experimental vs. legacy?
Should the legacy have \deprecated or @deprecated (whatever is the pattern in doxygen)
There was a problem hiding this comment.
I just moved this code. the legacy ssh uses a different protocol, but this one uses the same protocol as the Nix daemon. It probably shouldn't say "experimental" as its not an experimental feature (it predates those), but I think that's best done in a separate PR.
Motivation
Progress towards #10766
Context
I thought that #10768 achieved, but when I went to use this stuff (in Hydra), turns out it did not. (Those
using FooConfig;lines were not working --- they are so finicky!) This PR gets the job done, and adds some trivial unit tests to make sure I did what I intended.I had to add add a header to expose
SSHStoreConfig, after which the preexistingssh-store-config.*were very confusingly named files, so I renamed them tocommon-ssh-store-config.hhto match the type defined therein.Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.