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.

Sign up for GitHub

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

Ensure we can construct remote store configs in isolation #11108

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Ensure we can construct remote store configs in isolation #11108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion maintainers/flake-module.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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$''
Copy link
Contributor

Choose a reason for hiding this comment

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

why not apply formatting?

Copy link
Member Author

@Ericson2314 Ericson2314 Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

there is already a clang-format; I find this take a bit odd.
Format and move on my take.

''^src/libstore/content-address\.cc$''
''^src/libstore/content-address\.hh$''
''^src/libstore/daemon\.cc$''
Expand Down Expand Up @@ -215,7 +216,6 @@
''^src/libstore/serve-protocol\.hh$''
''^src/libstore/sqlite\.cc$''
''^src/libstore/sqlite\.hh$''
''^src/libstore/ssh-store-config\.hh$''
''^src/libstore/ssh-store\.cc$''
''^src/libstore/ssh\.cc$''
''^src/libstore/ssh\.hh$''
Expand Down
2 changes: 1 addition & 1 deletion src/libstore/ssh-store-config.cc → src/libstore/common-ssh-store-config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#include <regex>

#include "ssh-store-config.hh"
#include "common-ssh-store-config.hh"
Copy link
Contributor

Choose a reason for hiding this comment

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

ssh directory ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

#include "ssh.hh"

namespace nix {
Expand Down
0 src/libstore/ssh-store-config.hh → src/libstore/common-ssh-store-config.hh
View file
Open in desktop
File renamed without changes.
13 changes: 11 additions & 2 deletions src/libstore/legacy-ssh-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#include "legacy-ssh-store.hh"
#include "ssh-store-config.hh"
#include "common-ssh-store-config.hh"
#include "archive.hh"
#include "pool.hh"
#include "remote-store.hh"
Expand All @@ -15,6 +15,15 @@

namespace nix {

LegacySSHStoreConfig::LegacySSHStoreConfig(
std::string_view scheme,
std::string_view authority,
const Params & params)
: StoreConfig(params)
, CommonSSHStoreConfig(scheme, authority, params)
{
}

std::string LegacySSHStoreConfig::doc()
{
return
Expand All @@ -35,7 +44,7 @@ LegacySSHStore::LegacySSHStore(
const Params & params)
: StoreConfig(params)
, CommonSSHStoreConfig(scheme, host, params)
, LegacySSHStoreConfig(params)
, LegacySSHStoreConfig(scheme, host, params)
, Store(params)
, connections(make_ref<Pool<Connection>>(
std::max(1, (int) maxConnections),
Expand Down
7 changes: 6 additions & 1 deletion src/libstore/legacy-ssh-store.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#pragma once
///@file

#include "ssh-store-config.hh"
#include "common-ssh-store-config.hh"
#include "store-api.hh"
#include "ssh.hh"
#include "callback.hh"
Expand All @@ -13,6 +13,11 @@ struct LegacySSHStoreConfig : virtual CommonSSHStoreConfig
{
using CommonSSHStoreConfig::CommonSSHStoreConfig;

LegacySSHStoreConfig(
std::string_view scheme,
std::string_view authority,
const Params & params);

const Setting<Strings> remoteProgram{this, {"nix-store"}, "remote-program",
"Path to the `nix-store` executable on the remote machine."};

Expand Down
5 changes: 3 additions & 2 deletions src/libstore/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ sources = files(
'builtins/fetchurl.cc',
'builtins/unpack-channel.cc',
'common-protocol.cc',
'common-ssh-store-config.cc',
'content-address.cc',
'daemon.cc',
'derivations.cc',
Expand Down Expand Up @@ -206,7 +207,6 @@ sources = files(
'serve-protocol-connection.cc',
'serve-protocol.cc',
'sqlite.cc',
'ssh-store-config.cc',
'ssh-store.cc',
'ssh.cc',
'store-api.cc',
Expand All @@ -233,6 +233,7 @@ headers = [config_h] + files(
'builtins/buildenv.hh',
'common-protocol-impl.hh',
'common-protocol.hh',
'common-ssh-store-config.hh',
'content-address.hh',
'daemon.hh',
'derivations.hh',
Expand Down Expand Up @@ -272,11 +273,11 @@ headers = [config_h] + files(
'remote-store.hh',
's3-binary-cache-store.hh',
's3.hh',
'ssh-store.hh',
'serve-protocol-connection.hh',
'serve-protocol-impl.hh',
'serve-protocol.hh',
'sqlite.hh',
'ssh-store-config.hh',
'ssh.hh',
'store-api.hh',
'store-cast.hh',
Expand Down
35 changes: 16 additions & 19 deletions src/libstore/ssh-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#include "ssh-store-config.hh"
#include "store-api.hh"
#include "ssh-store.hh"
#include "local-fs-store.hh"
#include "remote-store.hh"
#include "remote-store-connection.hh"
#include "source-accessor.hh"
#include "archive.hh"
Expand All @@ -12,23 +10,22 @@

namespace nix {

struct SSHStoreConfig : virtual RemoteStoreConfig, virtual CommonSSHStoreConfig
SSHStoreConfig::SSHStoreConfig(
std::string_view scheme,
std::string_view authority,
const Params & params)
: StoreConfig(params)
, RemoteStoreConfig(params)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this validate it's ssh-ng ?

Copy link
Member Author

@Ericson2314 Ericson2314 Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh can't do that because its mounted-ssh-ng for subclass (which itself is bad, but we cannot yet fix)

, CommonSSHStoreConfig(scheme, authority, params)
{
using RemoteStoreConfig::RemoteStoreConfig;
using CommonSSHStoreConfig::CommonSSHStoreConfig;

const Setting<Strings> remoteProgram{this, {"nix-daemon"}, "remote-program",
"Path to the `nix-daemon` executable on the remote machine."};

const std::string name() override { return "Experimental SSH Store"; }
}

std::string doc() override
{
return
#include "ssh-store.md"
;
}
};
std::string SSHStoreConfig::doc()
{
return
#include "ssh-store.md"
;
}

class SSHStore : public virtual SSHStoreConfig, public virtual RemoteStore
{
Expand All @@ -41,7 +38,7 @@ class SSHStore : public virtual SSHStoreConfig, public virtual RemoteStore
: StoreConfig(params)
, RemoteStoreConfig(params)
, CommonSSHStoreConfig(scheme, host, params)
, SSHStoreConfig(params)
, SSHStoreConfig(scheme, host, params)
, Store(params)
, RemoteStore(params)
, master(createSSHMaster(
Expand Down
28 changes: 28 additions & 0 deletions src/libstore/ssh-store.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#pragma once
///@file

#include "common-ssh-store-config.hh"
#include "store-api.hh"
#include "remote-store.hh"

namespace nix {

struct SSHStoreConfig : virtual RemoteStoreConfig, virtual CommonSSHStoreConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

{
using CommonSSHStoreConfig::CommonSSHStoreConfig;
using RemoteStoreConfig::RemoteStoreConfig;

SSHStoreConfig(std::string_view scheme, std::string_view authority, const Params & params);

const Setting<Strings> remoteProgram{
this, {"nix-daemon"}, "remote-program", "Path to the `nix-daemon` executable on the remote machine."};

const std::string name() override
{
return "Experimental SSH Store";
}

std::string doc() override;
};

}
26 changes: 26 additions & 0 deletions tests/unit/libstore/legacy-ssh-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#include <gtest/gtest.h>

#include "legacy-ssh-store.hh"

namespace nix {

TEST(LegacySSHStore, constructConfig)
{
LegacySSHStoreConfig config{
"ssh",
"localhost",
StoreConfig::Params{
{
"remote-program",
// TODO #11106, no more split on space
"foo bar",
},
}};
EXPECT_EQ(
config.remoteProgram.get(),
(Strings{
"foo",
"bar",
}));
}
}
2 changes: 2 additions & 0 deletions tests/unit/libstore/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ sources = files(
'derivation.cc',
'derived-path.cc',
'downstream-placeholder.cc',
'legacy-ssh-store.cc',
'machines.cc',
'nar-info-disk-cache.cc',
'nar-info.cc',
Expand All @@ -67,6 +68,7 @@ sources = files(
'path.cc',
'references.cc',
'serve-protocol.cc',
'ssh-store.cc',
'store-reference.cc',
'worker-protocol.cc',
)
Expand Down
26 changes: 26 additions & 0 deletions tests/unit/libstore/ssh-store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#include <gtest/gtest.h>

#include "ssh-store.hh"

namespace nix {

TEST(SSHStore, constructConfig)
{
SSHStoreConfig config{
"ssh",
"localhost",
StoreConfig::Params{
{
"remote-program",
// TODO #11106, no more split on space
"foo bar",
},
}};
EXPECT_EQ(
config.remoteProgram.get(),
(Strings{
"foo",
"bar",
}));
}
}
Loading