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

Create StoreReference and use it in Machine #9839

Merged
merged 4 commits into from
May 22, 2024
Merged

Create StoreReference and use it in Machine #9839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d6c231
Slightly change formatting style
Ericson2314 May 21, 2024
c036d75
Factor out abstract syntax for Store URIs
Ericson2314 Jan 23, 2024
b59a7a1
Add `StoreReference::render`
Ericson2314 Jan 23, 2024
b3ebcc5
Use the new `StoreReference` in `Machine`
Ericson2314 Jan 23, 2024
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
4 changes: 3 additions & 1 deletion .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ SpaceAfterCStyleCast: true
SpaceAfterTemplateKeyword: false
AccessModifierOffset: -4
AlignAfterOpenBracket: AlwaysBreak
AlignEscapedNewlines: DontAlign
AlignEscapedNewlines: Left
ColumnLimit: 120
BreakStringLiterals: false
BitFieldColonSpacing: None
Expand All @@ -30,3 +30,5 @@ BreakBeforeBinaryOperators: NonAssignment
AlwaysBreakBeforeMultilineStrings: true
IndentPPDirectives: AfterHash
PPIndentWidth: 2
BinPackArguments: false
BinPackParameters: false
66 changes: 65 additions & 1 deletion maintainers/flake-module.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
excludes = [
# We don't want to format test data
# ''tests/(?!nixos/).*\.nix''
''^tests/.*''
''^tests/functional/.*$''
''^tests/unit/[^/]*/data/.*$''

# Don't format vendored code
''^src/toml11/.*''
Expand Down Expand Up @@ -426,6 +427,69 @@
''^src/nix/upgrade-nix\.cc$''
''^src/nix/verify\.cc$''
''^src/nix/why-depends\.cc$''

''^tests/nixos/ca-fd-leak/sender\.c''
''^tests/nixos/ca-fd-leak/smuggler\.c''
''^tests/unit/libexpr-support/tests/libexpr\.hh''
''^tests/unit/libexpr-support/tests/value/context\.cc''
''^tests/unit/libexpr-support/tests/value/context\.hh''
''^tests/unit/libexpr/derived-path\.cc''
''^tests/unit/libexpr/error_traces\.cc''
''^tests/unit/libexpr/eval\.cc''
''^tests/unit/libexpr/flake/flakeref\.cc''
''^tests/unit/libexpr/flake/url-name\.cc''
''^tests/unit/libexpr/json\.cc''
''^tests/unit/libexpr/main\.cc''
''^tests/unit/libexpr/primops\.cc''
''^tests/unit/libexpr/search-path\.cc''
''^tests/unit/libexpr/trivial\.cc''
''^tests/unit/libexpr/value/context\.cc''
''^tests/unit/libexpr/value/print\.cc''
''^tests/unit/libfetchers/public-key\.cc''
''^tests/unit/libstore-support/tests/derived-path\.cc''
''^tests/unit/libstore-support/tests/derived-path\.hh''
''^tests/unit/libstore-support/tests/libstore\.hh''
''^tests/unit/libstore-support/tests/nix_api_store\.hh''
''^tests/unit/libstore-support/tests/outputs-spec\.cc''
''^tests/unit/libstore-support/tests/outputs-spec\.hh''
''^tests/unit/libstore-support/tests/path\.cc''
''^tests/unit/libstore-support/tests/path\.hh''
''^tests/unit/libstore-support/tests/protocol\.hh''
''^tests/unit/libstore/common-protocol\.cc''
''^tests/unit/libstore/content-address\.cc''
''^tests/unit/libstore/derivation\.cc''
''^tests/unit/libstore/derived-path\.cc''
''^tests/unit/libstore/downstream-placeholder\.cc''
''^tests/unit/libstore/machines\.cc''
''^tests/unit/libstore/nar-info-disk-cache\.cc''
''^tests/unit/libstore/nar-info\.cc''
''^tests/unit/libstore/outputs-spec\.cc''
''^tests/unit/libstore/path-info\.cc''
''^tests/unit/libstore/path\.cc''
''^tests/unit/libstore/serve-protocol\.cc''
''^tests/unit/libstore/worker-protocol\.cc''
''^tests/unit/libutil-support/tests/characterization\.hh''
''^tests/unit/libutil-support/tests/hash\.cc''
''^tests/unit/libutil-support/tests/hash\.hh''
''^tests/unit/libutil/args\.cc''
''^tests/unit/libutil/canon-path\.cc''
''^tests/unit/libutil/chunked-vector\.cc''
''^tests/unit/libutil/closure\.cc''
''^tests/unit/libutil/compression\.cc''
''^tests/unit/libutil/config\.cc''
''^tests/unit/libutil/file-content-address\.cc''
''^tests/unit/libutil/git\.cc''
''^tests/unit/libutil/hash\.cc''
''^tests/unit/libutil/hilite\.cc''
''^tests/unit/libutil/json-utils\.cc''
''^tests/unit/libutil/logging\.cc''
''^tests/unit/libutil/lru-cache\.cc''
''^tests/unit/libutil/pool\.cc''
''^tests/unit/libutil/references\.cc''
''^tests/unit/libutil/suggestions\.cc''
''^tests/unit/libutil/tests\.cc''
''^tests/unit/libutil/url\.cc''
''^tests/unit/libutil/xml-writer\.cc''
];
};

Expand Down
22 changes: 11 additions & 11 deletions src/build-remote/build-remote.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ static std::string currentLoad;

static AutoCloseFD openSlotLock(const Machine & m, uint64_t slot)
{
return openLockFile(fmt("%s/%s-%d", currentLoad, escapeUri(m.storeUri), slot), true);
return openLockFile(fmt("%s/%s-%d", currentLoad, escapeUri(m.storeUri.render()), slot), true);
Copy link
Member

Choose a reason for hiding this comment

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

I think we have a problem here. Considering that we also have params in the URI, which do not affect the identity of the store to be opened, is this actually suitable for use in the name of a lock?
It's a pre-existing issue, fwiw.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes definitely a preexisting issue. If anything, we're better able to deal with it now.

}

static bool allSupportedLocally(Store & store, const std::set<std::string>& requiredFeatures) {
Expand Down Expand Up @@ -99,7 +99,7 @@ static int main_build_remote(int argc, char * * argv)
}

std::optional<StorePath> drvPath;
std::string storeUri;
StoreReference storeUri;

while (true) {

Expand Down Expand Up @@ -135,7 +135,7 @@ static int main_build_remote(int argc, char * * argv)
Machine * bestMachine = nullptr;
uint64_t bestLoad = 0;
for (auto & m : machines) {
debug("considering building on remote machine '%s'", m.storeUri);
debug("considering building on remote machine '%s'", m.storeUri.render());

if (m.enabled &&
m.systemSupported(neededSystem) &&
Expand Down Expand Up @@ -233,7 +233,7 @@ static int main_build_remote(int argc, char * * argv)

try {

Activity act(*logger, lvlTalkative, actUnknown, fmt("connecting to '%s'", bestMachine->storeUri));
Activity act(*logger, lvlTalkative, actUnknown, fmt("connecting to '%s'", bestMachine->storeUri.render()));

sshStore = bestMachine->openStore();
sshStore->connect();
Expand All @@ -242,7 +242,7 @@ static int main_build_remote(int argc, char * * argv)
} catch (std::exception & e) {
auto msg = chomp(drainFD(5, false));
printError("cannot build on '%s': %s%s",
bestMachine->storeUri, e.what(),
bestMachine->storeUri.render(), e.what(),
msg.empty() ? "" : ": " + msg);
bestMachine->enabled = false;
continue;
Expand All @@ -257,15 +257,15 @@ static int main_build_remote(int argc, char * * argv)

assert(sshStore);

std::cerr << "# accept\n" << storeUri << "\n";
std::cerr << "# accept\n" << storeUri.render() << "\n";

auto inputs = readStrings<PathSet>(source);
auto wantedOutputs = readStrings<StringSet>(source);

AutoCloseFD uploadLock = openLockFile(currentLoad + "/" + escapeUri(storeUri) + ".upload-lock", true);
AutoCloseFD uploadLock = openLockFile(currentLoad + "/" + escapeUri(storeUri.render()) + ".upload-lock", true);

{
Activity act(*logger, lvlTalkative, actUnknown, fmt("waiting for the upload lock to '%s'", storeUri));
Activity act(*logger, lvlTalkative, actUnknown, fmt("waiting for the upload lock to '%s'", storeUri.render()));

auto old = signal(SIGALRM, handleAlarm);
alarm(15 * 60);
Expand All @@ -278,7 +278,7 @@ static int main_build_remote(int argc, char * * argv)
auto substitute = settings.buildersUseSubstitutes ? Substitute : NoSubstitute;

{
Activity act(*logger, lvlTalkative, actUnknown, fmt("copying dependencies to '%s'", storeUri));
Activity act(*logger, lvlTalkative, actUnknown, fmt("copying dependencies to '%s'", storeUri.render()));
copyPaths(*store, *sshStore, store->parseStorePathSet(inputs), NoRepair, NoCheckSigs, substitute);
}

Expand Down Expand Up @@ -316,7 +316,7 @@ static int main_build_remote(int argc, char * * argv)
optResult = sshStore->buildDerivation(*drvPath, (const BasicDerivation &) drv);
auto & result = *optResult;
if (!result.success())
throw Error("build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri, result.errorMsg);
throw Error("build of '%s' on '%s' failed: %s", store->printStorePath(*drvPath), storeUri.render(), result.errorMsg);
} else {
copyClosure(*store, *sshStore, StorePathSet {*drvPath}, NoRepair, NoCheckSigs, substitute);
auto res = sshStore->buildPathsWithResults({
Expand Down Expand Up @@ -359,7 +359,7 @@ static int main_build_remote(int argc, char * * argv)
}

if (!missingPaths.empty()) {
Activity act(*logger, lvlTalkative, actUnknown, fmt("copying outputs from '%s'", storeUri));
Activity act(*logger, lvlTalkative, actUnknown, fmt("copying outputs from '%s'", storeUri.render()));
if (auto localStore = store.dynamic_pointer_cast<LocalStore>())
for (auto & path : missingPaths)
localStore->locksHeld.insert(store->printStorePath(path)); /* FIXME: ugly */
Expand Down
5 changes: 4 additions & 1 deletion src/libcmd/network-proxy.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ static StringSet getExcludingNoProxyVariables()
static const StringSet excludeVariables{"no_proxy", "NO_PROXY"};
StringSet variables;
std::set_difference(
networkProxyVariables.begin(), networkProxyVariables.end(), excludeVariables.begin(), excludeVariables.end(),
networkProxyVariables.begin(),
networkProxyVariables.end(),
excludeVariables.begin(),
excludeVariables.end(),
std::inserter(variables, variables.begin()));
return variables;
}
Expand Down
35 changes: 22 additions & 13 deletions src/libstore/machines.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@

namespace nix {

Machine::Machine(decltype(storeUri) storeUri,
Machine::Machine(
const std::string & storeUri,
decltype(systemTypes) systemTypes,
decltype(sshKey) sshKey,
decltype(maxJobs) maxJobs,
decltype(speedFactor) speedFactor,
decltype(supportedFeatures) supportedFeatures,
decltype(mandatoryFeatures) mandatoryFeatures,
decltype(sshPublicHostKey) sshPublicHostKey) :
storeUri(
storeUri(StoreReference::parse(
// Backwards compatibility: if the URI is schemeless, is not a path,
// and is not one of the special store connection words, prepend
// ssh://.
Expand All @@ -28,7 +29,7 @@ Machine::Machine(decltype(storeUri) storeUri,
|| hasPrefix(storeUri, "local?")
|| hasPrefix(storeUri, "?")
? storeUri
: "ssh://" + storeUri),
: "ssh://" + storeUri)),
systemTypes(systemTypes),
sshKey(sshKey),
maxJobs(maxJobs),
Expand Down Expand Up @@ -63,23 +64,26 @@ bool Machine::mandatoryMet(const std::set<std::string> & features) const
});
}

ref<Store> Machine::openStore() const
StoreReference Machine::completeStoreReference() const
{
Store::Params storeParams;
if (hasPrefix(storeUri, "ssh://")) {
storeParams["max-connections"] = "1";
storeParams["log-fd"] = "4";
auto storeUri = this->storeUri;

auto * generic = std::get_if<StoreReference::Specified>(&storeUri.variant);

if (generic && generic->scheme == "ssh") {
storeUri.params["max-connections"] = "1";
storeUri.params["log-fd"] = "4";
}

if (hasPrefix(storeUri, "ssh://") || hasPrefix(storeUri, "ssh-ng://")) {
if (generic && (generic->scheme == "ssh" || generic->scheme == "ssh-ng")) {
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
if (sshKey != "")
storeParams["ssh-key"] = sshKey;
storeUri.params["ssh-key"] = sshKey;
if (sshPublicHostKey != "")
storeParams["base64-ssh-public-host-key"] = sshPublicHostKey;
storeUri.params["base64-ssh-public-host-key"] = sshPublicHostKey;
}

{
auto & fs = storeParams["system-features"];
auto & fs = storeUri.params["system-features"];
auto append = [&](auto feats) {
for (auto & f : feats) {
if (fs.size() > 0) fs += ' ';
Expand All @@ -90,7 +94,12 @@ ref<Store> Machine::openStore() const
append(mandatoryFeatures);
}

return nix::openStore(storeUri, storeParams);
return storeUri;
}

ref<Store> Machine::openStore() const
{
return nix::openStore(completeStoreReference());
}

static std::vector<std::string> expandBuilderLines(const std::string & builders)
Expand Down
21 changes: 19 additions & 2 deletions src/libstore/machines.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@
///@file

#include "types.hh"
#include "store-reference.hh"

namespace nix {

class Store;

struct Machine {

const std::string storeUri;
const StoreReference storeUri;
const std::set<std::string> systemTypes;
const std::string sshKey;
const unsigned int maxJobs;
Expand All @@ -36,7 +37,8 @@ struct Machine {
*/
bool mandatoryMet(const std::set<std::string> & features) const;

Machine(decltype(storeUri) storeUri,
Machine(
const std::string & storeUri,
decltype(systemTypes) systemTypes,
decltype(sshKey) sshKey,
decltype(maxJobs) maxJobs,
Expand All @@ -45,6 +47,21 @@ struct Machine {
decltype(mandatoryFeatures) mandatoryFeatures,
decltype(sshPublicHostKey) sshPublicHostKey);

/**
* Elaborate `storeUri` into a complete store reference,
* incorporating information from the other fields of the `Machine`
* as applicable.
*/
StoreReference completeStoreReference() const;

/**
* Open a `Store` for this machine.
*
* Just a simple function composition:
* ```c++
* nix::openStore(completeStoreReference())
* ```
*/
ref<Store> openStore() const;
};

Expand Down
Loading
Loading