From d1f57c5dae43468d331a7fdb4c5a5e44eff28f1c Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 30 Jun 2025 13:56:04 -0700 Subject: [PATCH 1/4] external-derivation-builder: write the json doc into builder's stdin --- src/libstore/include/nix/store/globals.hh | 2 +- .../unix/build/external-derivation-builder.cc | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/libstore/include/nix/store/globals.hh b/src/libstore/include/nix/store/globals.hh index fcfc2e94a..041300bed 100644 --- a/src/libstore/include/nix/store/globals.hh +++ b/src/libstore/include/nix/store/globals.hh @@ -1248,7 +1248,7 @@ public: R"( Helper programs that execute derivations. - The program is passed a JSON document that describes the build environment as the final argument. + The program is passed a JSON document that describes the build environment on standard input. The JSON document looks like this: { diff --git a/src/libstore/unix/build/external-derivation-builder.cc b/src/libstore/unix/build/external-derivation-builder.cc index 1906ddd70..9fe0eb19f 100644 --- a/src/libstore/unix/build/external-derivation-builder.cc +++ b/src/libstore/unix/build/external-derivation-builder.cc @@ -4,6 +4,11 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl { Settings::ExternalBuilder externalBuilder; + /** + * Pipe for talking to the spawned builder. + */ + Pipe toBuilder; + ExternalDerivationBuilder( Store & store, std::unique_ptr miscMethods, @@ -83,23 +88,22 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl json.emplace("realStoreDir", getLocalStore(store).config->realStoreDir.get()); json.emplace("system", drv.platform); - // FIXME: maybe write this JSON into the builder's stdin instead....? - auto jsonFile = topTmpDir + "/build.json"; - writeFile(jsonFile, json.dump()); + toBuilder.create(); pid = startProcess([&]() { openSlave(); try { commonChildInit(); + if (dup2(toBuilder.readSide.get(), STDIN_FILENO) == -1) + throw SysError("duping to-builder read side to builder's stdin"); + Strings args = {externalBuilder.program}; if (!externalBuilder.args.empty()) { args.insert(args.end(), externalBuilder.args.begin(), externalBuilder.args.end()); } - args.insert(args.end(), jsonFile); - debug("executing external builder: %s", concatStringsSep(" ", args)); execv(externalBuilder.program.c_str(), stringsToCharPtrs(args).data()); @@ -109,6 +113,9 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl _exit(1); } }); + + writeFull(toBuilder.writeSide.get(), json.dump()); + toBuilder.close(); } }; From efa239875b772544e6650aee57452d108d29acbe Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 14 Jul 2025 07:32:11 -0700 Subject: [PATCH 2/4] Add an `external-builders` experimental feature --- src/libstore/include/nix/store/globals.hh | 20 ++++++++++++++++++- .../unix/build/external-derivation-builder.cc | 1 + src/libutil/experimental-features.cc | 8 ++++++++ .../include/nix/util/experimental-features.hh | 1 + 4 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/libstore/include/nix/store/globals.hh b/src/libstore/include/nix/store/globals.hh index 041300bed..2dfd187c1 100644 --- a/src/libstore/include/nix/store/globals.hh +++ b/src/libstore/include/nix/store/globals.hh @@ -1309,7 +1309,25 @@ public: "tmpDirInSandbox": "/build", "topTmpDir": "/private/tmp/nix-build-hello-2.12.2.drv-0" } - )" + )", + {}, // aliases + true, // document default + // NOTE(cole-h): even though we can make the experimental feature required here, the errors + // are not as good (it just becomes a warning if you try to use this setting without the + // experimental feature) + // + // With this commented out: + // + // error: experimental Nix feature 'external-builders' is disabled; add '--extra-experimental-features external-builders' to enable it + // + // With this uncommented: + // + // warning: Ignoring setting 'external-builders' because experimental feature 'external-builders' is not enabled + // error: Cannot build '/nix/store/vwsp4qd8a62jqa36p26d15hin4xnj949-opentofu-1.10.2.drv'. + // Reason: required system or feature not available + // Required system: 'aarch64-linux' with features {} + // Current system: 'aarch64-darwin' with features {apple-virt, benchmark, big-parallel, nixos-test} + // Xp::ExternalBuilders }; }; diff --git a/src/libstore/unix/build/external-derivation-builder.cc b/src/libstore/unix/build/external-derivation-builder.cc index 9fe0eb19f..20919187c 100644 --- a/src/libstore/unix/build/external-derivation-builder.cc +++ b/src/libstore/unix/build/external-derivation-builder.cc @@ -17,6 +17,7 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl : DerivationBuilderImpl(store, std::move(miscMethods), std::move(params)) , externalBuilder(std::move(externalBuilder)) { + experimentalFeatureSettings.require(Xp::ExternalBuilders); } static std::unique_ptr newIfSupported( diff --git a/src/libutil/experimental-features.cc b/src/libutil/experimental-features.cc index 04e8705e5..075b90ec5 100644 --- a/src/libutil/experimental-features.cc +++ b/src/libutil/experimental-features.cc @@ -288,6 +288,14 @@ constexpr std::array xpFeatureDetails )", .trackingUrl = "https://github.com/NixOS/nix/milestone/55", }, + { + .tag = Xp::ExternalBuilders, + .name = "external-builders", + .description = R"( + Enables support for external builders / sandbox providers. + )", + .trackingUrl = "", + }, { .tag = Xp::BLAKE3Hashes, .name = "blake3-hashes", diff --git a/src/libutil/include/nix/util/experimental-features.hh b/src/libutil/include/nix/util/experimental-features.hh index d7bc56f27..5a01d960c 100644 --- a/src/libutil/include/nix/util/experimental-features.hh +++ b/src/libutil/include/nix/util/experimental-features.hh @@ -35,6 +35,7 @@ enum struct ExperimentalFeature MountedSSHStore, VerifiedFetches, PipeOperators, + ExternalBuilders, BLAKE3Hashes, }; From 5b27325bc23472862ece37cd5883ebb65f206959 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 14 Jul 2025 11:00:13 -0700 Subject: [PATCH 3/4] Revert "external-derivation-builder: write the json doc into builder's stdin" This reverts commit d1f57c5dae43468d331a7fdb4c5a5e44eff28f1c. --- src/libstore/include/nix/store/globals.hh | 2 +- .../unix/build/external-derivation-builder.cc | 17 +++++------------ 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/libstore/include/nix/store/globals.hh b/src/libstore/include/nix/store/globals.hh index 2dfd187c1..fdc0c0827 100644 --- a/src/libstore/include/nix/store/globals.hh +++ b/src/libstore/include/nix/store/globals.hh @@ -1248,7 +1248,7 @@ public: R"( Helper programs that execute derivations. - The program is passed a JSON document that describes the build environment on standard input. + The program is passed a JSON document that describes the build environment as the final argument. The JSON document looks like this: { diff --git a/src/libstore/unix/build/external-derivation-builder.cc b/src/libstore/unix/build/external-derivation-builder.cc index 20919187c..e71cd7119 100644 --- a/src/libstore/unix/build/external-derivation-builder.cc +++ b/src/libstore/unix/build/external-derivation-builder.cc @@ -4,11 +4,6 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl { Settings::ExternalBuilder externalBuilder; - /** - * Pipe for talking to the spawned builder. - */ - Pipe toBuilder; - ExternalDerivationBuilder( Store & store, std::unique_ptr miscMethods, @@ -89,22 +84,23 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl json.emplace("realStoreDir", getLocalStore(store).config->realStoreDir.get()); json.emplace("system", drv.platform); - toBuilder.create(); + // FIXME: maybe write this JSON into the builder's stdin instead....? + auto jsonFile = topTmpDir + "/build.json"; + writeFile(jsonFile, json.dump()); pid = startProcess([&]() { openSlave(); try { commonChildInit(); - if (dup2(toBuilder.readSide.get(), STDIN_FILENO) == -1) - throw SysError("duping to-builder read side to builder's stdin"); - Strings args = {externalBuilder.program}; if (!externalBuilder.args.empty()) { args.insert(args.end(), externalBuilder.args.begin(), externalBuilder.args.end()); } + args.insert(args.end(), jsonFile); + debug("executing external builder: %s", concatStringsSep(" ", args)); execv(externalBuilder.program.c_str(), stringsToCharPtrs(args).data()); @@ -114,9 +110,6 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl _exit(1); } }); - - writeFull(toBuilder.writeSide.get(), json.dump()); - toBuilder.close(); } }; From de158c335c97b4728856311d6cdacb2eaac920dd Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 14 Jul 2025 11:01:46 -0700 Subject: [PATCH 4/4] fixup: document why we're not writing through stdin right now --- src/libstore/unix/build/external-derivation-builder.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libstore/unix/build/external-derivation-builder.cc b/src/libstore/unix/build/external-derivation-builder.cc index e71cd7119..508ad45a3 100644 --- a/src/libstore/unix/build/external-derivation-builder.cc +++ b/src/libstore/unix/build/external-derivation-builder.cc @@ -84,8 +84,10 @@ struct ExternalDerivationBuilder : DerivationBuilderImpl json.emplace("realStoreDir", getLocalStore(store).config->realStoreDir.get()); json.emplace("system", drv.platform); - // FIXME: maybe write this JSON into the builder's stdin instead....? - auto jsonFile = topTmpDir + "/build.json"; + // TODO(cole-h): writing this to stdin is too much effort right now, if we want to revisit + // that, see this comment by Eelco about how to make it not suck: + // https://github.com/DeterminateSystems/nix-src/pull/141#discussion_r2205493257 + auto jsonFile = std::filesystem::path{topTmpDir} / "build.json"; writeFile(jsonFile, json.dump()); pid = startProcess([&]() {