Merge pull request #106 from ddunbar/buildsystem-env
[BuildSystem] Add support for a base environment.
diff --git a/docs/buildsystem.rst b/docs/buildsystem.rst
index c292e76..38b3a03 100644
--- a/docs/buildsystem.rst
+++ b/docs/buildsystem.rst
@@ -390,9 +390,13 @@
* - env
- A mapping of keys and values defining the environment to pass to the
- launched process. If provided, **only** the entries in this mapping will
- be exposed in the process environment. If not provided, the process will
- inherit the environment from the client.
+ launched process. See also `inherit-env`.
+
+ * - inherit-env
+ - A boolean flag controlling whether this command should inherit the base
+ environment provided when executing the build system (either from the
+ command line, or via the internal C APIs), or whether it should only
+ include the entries explicitly provided in the `env` mapping above.
* - allow-missing-inputs
- A boolean value, indicating whether the commands should be allowed to run
diff --git a/include/llbuild/BuildSystem/BuildExecutionQueue.h b/include/llbuild/BuildSystem/BuildExecutionQueue.h
index b969137..475d326 100644
--- a/include/llbuild/BuildSystem/BuildExecutionQueue.h
+++ b/include/llbuild/BuildSystem/BuildExecutionQueue.h
@@ -108,8 +108,11 @@
///
/// \param commandLine The command line to execute.
///
- /// \param environment The environment to launch with. If empty, the process
- /// environment will be inherited.
+ /// \param environment The environment to launch with.
+ ///
+ /// \param inheritEnvironment If true, the supplied environment will be
+ /// overlayed on top base environment supplied when creating the queue. If
+ /// false, only the supplied environment will be passed to the subprocess.
///
/// \returns True on success.
//
@@ -118,7 +121,8 @@
virtual bool
executeProcess(QueueJobContext* context,
ArrayRef<StringRef> commandLine,
- ArrayRef<std::pair<StringRef, StringRef>> environment) = 0;
+ ArrayRef<std::pair<StringRef, StringRef>> environment,
+ bool inheritEnvironment = true) = 0;
/// @}
@@ -223,8 +227,8 @@
/// become invalid as soon as the client returns from this API call.
///
/// \param result - Whether the process suceeded, failed or was cancelled.
- /// \param exitStatus - The raw exit status of the process, or -1 if an error was
- /// encountered.
+ /// \param exitStatus - The raw exit status of the process, or -1 if an error
+ /// was encountered.
//
// FIXME: Need to include additional information on the status here, e.g., the
// signal status, and the process output (if buffering).
@@ -236,7 +240,8 @@
/// Create an execution queue that schedules jobs to individual lanes with a
/// capped limit on the number of concurrent lanes.
BuildExecutionQueue *createLaneBasedExecutionQueue(
- BuildExecutionQueueDelegate& delegate, int numLanes);
+ BuildExecutionQueueDelegate& delegate, int numLanes,
+ const char* const* environment);
}
}
diff --git a/include/llbuild/BuildSystem/BuildSystemFrontend.h b/include/llbuild/BuildSystem/BuildSystemFrontend.h
index 8c90b23..b70fdfa 100644
--- a/include/llbuild/BuildSystem/BuildSystemFrontend.h
+++ b/include/llbuild/BuildSystem/BuildSystemFrontend.h
@@ -295,6 +295,14 @@
/// The path of the build trace output file to use, if any.
std::string traceFilePath = "";
+ /// The base environment to use when executing subprocesses.
+ ///
+ /// The format is expected to match that of `::main()`, i.e. a null-terminated
+ /// array of pointers to null terminated C strings.
+ ///
+ /// If empty, the environment of the calling process will be used.
+ const char* const* environment = nullptr;
+
/// The positional arguments.
std::vector<std::string> positionalArgs;
diff --git a/lib/BuildSystem/BuildSystem.cpp b/lib/BuildSystem/BuildSystem.cpp
index a470c0b..3aaa600 100644
--- a/lib/BuildSystem/BuildSystem.cpp
+++ b/lib/BuildSystem/BuildSystem.cpp
@@ -959,7 +959,10 @@
/// The style of dependencies used.
DepsStyle depsStyle = DepsStyle::Unused;
-
+
+ /// Whether to inherit the base environment.
+ bool inheritEnv = true;
+
virtual uint64_t getSignature() override {
// FIXME: Use a more appropriate hashing infrastructure.
using llvm::hash_combine;
@@ -975,6 +978,7 @@
code = hash_combine(code, path);
}
code = hash_combine(code, int(depsStyle));
+ code = hash_combine(code, int(inheritEnv));
return size_t(code);
}
@@ -1148,6 +1152,13 @@
return false;
}
return true;
+ } else if (name == "inherit-env") {
+ if (value != "true" && value != "false") {
+ ctx.error("invalid value: '" + value + "' for attribute '" +
+ name + "'");
+ return false;
+ }
+ inheritEnv = value == "true";
} else {
return ExternalCommand::configureAttribute(ctx, name, value);
}
@@ -1201,7 +1212,7 @@
// Execute the command.
if (!bsci.getExecutionQueue().executeProcess(
context, std::vector<StringRef>(args.begin(), args.end()),
- environment)) {
+ environment, /*inheritEnvironment=*/inheritEnv)) {
// If the command failed, there is no need to gather dependencies.
return false;
}
diff --git a/lib/BuildSystem/BuildSystemFrontend.cpp b/lib/BuildSystem/BuildSystemFrontend.cpp
index 137e556..1078a19 100644
--- a/lib/BuildSystem/BuildSystemFrontend.cpp
+++ b/lib/BuildSystem/BuildSystemFrontend.cpp
@@ -313,7 +313,8 @@
if (impl->invocation.useSerialBuild) {
return std::unique_ptr<BuildExecutionQueue>(
- createLaneBasedExecutionQueue(impl->executionQueueDelegate, 1));
+ createLaneBasedExecutionQueue(impl->executionQueueDelegate, 1,
+ impl->invocation.environment));
}
// Get the number of CPUs to use.
@@ -327,7 +328,8 @@
}
return std::unique_ptr<BuildExecutionQueue>(
- createLaneBasedExecutionQueue(impl->executionQueueDelegate, numLanes));
+ createLaneBasedExecutionQueue(impl->executionQueueDelegate, numLanes,
+ impl->invocation.environment));
}
bool BuildSystemFrontendDelegate::isCancelled() {
diff --git a/lib/BuildSystem/LaneBasedExecutionQueue.cpp b/lib/BuildSystem/LaneBasedExecutionQueue.cpp
index 8977150..d19103c 100644
--- a/lib/BuildSystem/LaneBasedExecutionQueue.cpp
+++ b/lib/BuildSystem/LaneBasedExecutionQueue.cpp
@@ -17,6 +17,7 @@
#include "llbuild/Basic/PlatformUtility.h"
#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/Twine.h"
@@ -43,11 +44,13 @@
using namespace llbuild;
using namespace llbuild::buildsystem;
-#if !defined(_WIN32)
-extern "C" {
- extern char **environ;
+namespace std {
+ template<> struct hash<llvm::StringRef> {
+ size_t operator()(const StringRef& value) const {
+ return size_t(hash_value(value));
+ }
+ };
}
-#endif
namespace {
@@ -82,6 +85,9 @@
std::mutex queueCompleteMutex;
bool queueComplete { false };
+ /// The base environment.
+ const char* const* environment;
+
void executeLane(unsigned laneNumber) {
// Set the thread name, if available.
#if defined(__APPLE__)
@@ -153,8 +159,10 @@
public:
LaneBasedExecutionQueue(BuildExecutionQueueDelegate& delegate,
- unsigned numLanes)
- : BuildExecutionQueue(delegate), numLanes(numLanes)
+ unsigned numLanes,
+ const char* const* environment)
+ : BuildExecutionQueue(delegate), numLanes(numLanes),
+ environment(environment)
{
for (unsigned i = 0; i != numLanes; ++i) {
lanes.push_back(std::unique_ptr<std::thread>(
@@ -208,7 +216,8 @@
executeProcess(QueueJobContext* opaqueContext,
ArrayRef<StringRef> commandLine,
ArrayRef<std::pair<StringRef,
- StringRef>> environment) override {
+ StringRef>> environment,
+ bool inheritEnvironment) override {
{
std::unique_lock<std::mutex> lock(readyJobsMutex);
// Do not execute new processes anymore after cancellation.
@@ -309,7 +318,7 @@
posix_spawn_file_actions_adddup2(&fileActions, 2, 2);
}
- // Form the complete C-string command line.
+ // Form the complete C string command line.
std::vector<std::string> argsStorage(
commandLine.begin(), commandLine.end());
std::vector<const char*> args(argsStorage.size() + 1);
@@ -320,24 +329,49 @@
// Form the complete environment.
std::vector<std::string> envStorage;
- for (const auto& entry: environment) {
- SmallString<256> assignment;
- assignment += entry.first;
- assignment += '=';
- assignment += entry.second;
- assignment += '\0';
- envStorage.emplace_back(assignment.str());
- }
- std::vector<const char*> env(environment.size() + 1);
- char* const* envp = nullptr;
+ std::vector<const char*> env;
+ const char* const* envp = nullptr;
+
+ // If no additional environment is supplied, use the base environment.
if (environment.empty()) {
- envp = environ;
+ envp = this->environment;
} else {
- for (size_t i = 0; i != envStorage.size(); ++i) {
- env[i] = envStorage[i].c_str();
+ // Inherit the base environment, if desired.
+ if (inheritEnvironment) {
+ std::unordered_set<StringRef> overriddenKeys{};
+ // Compute the set of strings which are overridden in the process
+ // environment.
+ for (const auto& entry: environment) {
+ overriddenKeys.insert(entry.first);
+ }
+
+ // Form the complete environment by adding the base key value pairs
+ // which are not overridden, then adding all of the overridden ones.
+ for (const char* const* p = this->environment; *p != nullptr; ++p) {
+ // Find the key.
+ auto key = StringRef(*p).split('=').first;
+ if (!overriddenKeys.count(key)) {
+ env.emplace_back(*p);
+ }
+ }
}
- env[envStorage.size()] = nullptr;
- envp = const_cast<char**>(env.data());
+
+ // Add the requested environment.
+ for (const auto& entry: environment) {
+ SmallString<256> assignment;
+ assignment += entry.first;
+ assignment += '=';
+ assignment += entry.second;
+ assignment += '\0';
+ envStorage.emplace_back(assignment.str());
+ }
+ // We must do this in a second pass, once the entries are guaranteed not
+ // to move.
+ for (const auto& entry: envStorage) {
+ env.emplace_back(entry.c_str());
+ }
+ env.emplace_back(nullptr);
+ envp = env.data();
}
// Resolve the executable path, if necessary.
@@ -360,7 +394,7 @@
if (posix_spawn(&pid, args[0], /*file_actions=*/&fileActions,
/*attrp=*/&attributes, const_cast<char**>(args.data()),
- envp) != 0) {
+ const_cast<char* const*>(envp)) != 0) {
getDelegate().commandProcessHadError(
context.job.getForCommand(), handle,
Twine("unable to spawn process (") + strerror(errno) + ")");
@@ -439,8 +473,18 @@
}
+#if !defined(_WIN32)
+extern "C" {
+ extern char **environ;
+}
+#endif
+
BuildExecutionQueue*
llbuild::buildsystem::createLaneBasedExecutionQueue(
- BuildExecutionQueueDelegate& delegate, int numLanes) {
- return new LaneBasedExecutionQueue(delegate, numLanes);
+ BuildExecutionQueueDelegate& delegate, int numLanes,
+ const char* const* environment) {
+ if (!environment) {
+ environment = const_cast<const char* const*>(environ);
+ }
+ return new LaneBasedExecutionQueue(delegate, numLanes, environment);
}
diff --git a/products/libllbuild/BuildSystem-C-API.cpp b/products/libllbuild/BuildSystem-C-API.cpp
index ce7d411..4931759 100644
--- a/products/libllbuild/BuildSystem-C-API.cpp
+++ b/products/libllbuild/BuildSystem-C-API.cpp
@@ -328,7 +328,9 @@
invocation.buildFilePath =
cAPIInvocation.buildFilePath ? cAPIInvocation.buildFilePath : "";
invocation.dbPath = cAPIInvocation.dbPath ? cAPIInvocation.dbPath : "";
- invocation.traceFilePath = cAPIInvocation.traceFilePath ? cAPIInvocation.traceFilePath : "";
+ invocation.traceFilePath = (
+ cAPIInvocation.traceFilePath ? cAPIInvocation.traceFilePath : "");
+ invocation.environment = cAPIInvocation.environment;
invocation.useSerialBuild = cAPIInvocation.useSerialBuild;
invocation.showVerboseStatus = cAPIInvocation.showVerboseStatus;
diff --git a/products/libllbuild/public-api/llbuild/buildsystem.h b/products/libllbuild/public-api/llbuild/buildsystem.h
index 61b16b2..b35d76e 100644
--- a/products/libllbuild/public-api/llbuild/buildsystem.h
+++ b/products/libllbuild/public-api/llbuild/buildsystem.h
@@ -124,7 +124,18 @@
/// The path of the build trace output file to use, if any.
const char* traceFilePath;
-
+
+ /// The base environment to use when executing subprocesses.
+ ///
+ /// The format is expected to match that of `::main()`, i.e. a null-terminated
+ /// array of pointers to null terminated C strings.
+ ///
+ /// If empty, the environment of the calling process will be used.
+ ///
+ /// The environment is *not* copied by the build invocation, and must remain
+ /// alive for the lifetime of any build using this invocation.
+ const char* const* environment;
+
/// Whether to show verbose output.
//
// FIXME: This doesn't belong here, move once the status is fully delegated.
@@ -247,7 +258,8 @@
///
/// The system guarantees that all such calls will be paired with a
/// corresponding \see commandFinished() call.
- bool (*should_command_start)(void* context, llb_buildsystem_command_t* command);
+ bool (*should_command_start)(void* context,
+ llb_buildsystem_command_t* command);
/// Called when a command has been started.
///
@@ -323,6 +335,7 @@
/// It is an unchecked error for the client to request multiple builds
/// concurrently.
///
+/// \param key The key to build.
/// \returns True on success, or false if the build was aborted (for example, if
/// a cycle was discovered).
LLBUILD_EXPORT bool
@@ -403,8 +416,6 @@
///
/// The contents *MUST* be returned in a new buffer allocated with \see
/// malloc().
- //
- // FIXME: We need to use a better data type than a uint64_t here.
void (*get_signature)(void* context, llb_buildsystem_command_t* command,
llb_data_t* data_out);
diff --git a/products/libllbuild/public-api/llbuild/llbuild.h b/products/libllbuild/public-api/llbuild/llbuild.h
index 6ebee03..41103fe 100644
--- a/products/libllbuild/public-api/llbuild/llbuild.h
+++ b/products/libllbuild/public-api/llbuild/llbuild.h
@@ -31,6 +31,18 @@
#define LLBUILD_EXPORT extern
#endif
+/// A monotonically increasing indicator of the llbuild API version.
+///
+/// The llbuild API is *not* stable. This value allows clients to conditionally
+/// compile for multiple versions of the API.
+///
+/// Version History:
+///
+/// 1: Added `environment` parameter to llb_buildsystem_invocation_t.
+///
+/// 0: Pre-history
+#define LLBUILD_API_VERSION 1
+
/// Get the full version of the llbuild library.
LLBUILD_EXPORT const char* llb_get_full_version_string(void);
diff --git a/tests/BuildSystem/Build/basic.llbuild b/tests/BuildSystem/Build/basic.llbuild
index 03ab159..6110430 100644
--- a/tests/BuildSystem/Build/basic.llbuild
+++ b/tests/BuildSystem/Build/basic.llbuild
@@ -9,8 +9,11 @@
# RUN: diff "%t.build/input A" %t.build/output
#
# CHECK: /usr/bin/env
+# CHECK: ENV_KEY=ENV_VALUE_1
# CHECK-NOT: PATH=
-# CHECK: ENV_KEY=ENV_VALUE
+# CHECK-NEXT: /usr/bin/env
+# CHECK: ENV_KEY=ENV_VALUE_2
+# CHECK: PATH=random-overridden-path
# CHECK: cp 'input A' output
# Check the engine trace.
@@ -21,7 +24,7 @@
# CHECK-TRACE: "new-rule", "R1", "Tbasic"
# CHECK-TRACE: "new-rule", "R2", "Noutput"
# CHECK-TRACE: "new-rule", "R3", "Ccp-output"
-# CHECK-TRACE: "new-rule", "R4", "N<env>"
+# CHECK-TRACE: "new-rule", "R4", "N<env-2>"
# CHECK-TRACE: "build-ended"
# Check that a null build does nothing.
@@ -59,16 +62,26 @@
default: basic
commands:
- "<env>":
+ "<env-1>":
tool: shell
- outputs: ["<env>"]
+ outputs: ["<env-1>"]
args: ["/usr/bin/env"]
env:
- ENV_KEY: ENV_VALUE
-
+ ENV_KEY: ENV_VALUE_1
+ inherit-env: false
+
+ "<env-2>":
+ tool: shell
+ inputs: ["<env-1>"]
+ outputs: ["<env-2>"]
+ args: /usr/bin/env | /usr/bin/sort
+ env:
+ ENV_KEY: ENV_VALUE_2
+ PATH: random-overridden-path
+
cp-output:
tool: shell
- inputs: ["input A", "<env>"]
+ inputs: ["input A", "<env-1>", "<env-2>"]
outputs: ["output"]
# FIXME: Design a limited mechanism for substitution. Might be tool specific.
args: ["cp", "input A", "output"]
diff --git a/unittests/BuildSystem/LaneBasedExecutionQueueTest.cpp b/unittests/BuildSystem/LaneBasedExecutionQueueTest.cpp
index c2107a9..5b5c77b 100644
--- a/unittests/BuildSystem/LaneBasedExecutionQueueTest.cpp
+++ b/unittests/BuildSystem/LaneBasedExecutionQueueTest.cpp
@@ -37,10 +37,15 @@
virtual void commandJobStarted(Command* command) override {}
virtual void commandJobFinished(Command* command) override {}
- virtual void commandProcessStarted(Command* command, ProcessHandle handle) override {}
- virtual void commandProcessHadError(Command* command, ProcessHandle handle, const Twine& message) override {}
- virtual void commandProcessHadOutput(Command* command, ProcessHandle handle, StringRef data) override {}
- virtual void commandProcessFinished(Command* command, ProcessHandle handle, CommandResult result, int exitStatus) override {}
+ virtual void commandProcessStarted(Command* command,
+ ProcessHandle handle) override {}
+ virtual void commandProcessHadError(Command* command, ProcessHandle handle,
+ const Twine& message) override {}
+ virtual void commandProcessHadOutput(Command* command, ProcessHandle handle,
+ StringRef data) override {}
+ virtual void commandProcessFinished(Command* command, ProcessHandle handle,
+ CommandResult result,
+ int exitStatus) override {}
};
TEST(LaneBasedExecutionQueueTest, basic) {
@@ -48,7 +53,8 @@
std::unique_ptr<FileSystem> fs = createLocalFileSystem();
TmpDir tempDir{"LaneBasedExecutionQueueTest"};
std::string outputFile = tempDir.str() + "/yes-output.txt";
- auto queue = std::unique_ptr<BuildExecutionQueue>(createLaneBasedExecutionQueue(delegate, 2));
+ auto queue = std::unique_ptr<BuildExecutionQueue>(
+ createLaneBasedExecutionQueue(delegate, 2, /*environment=*/nullptr));
auto fn = [&outputFile, &queue](QueueJobContext* context) {
queue->executeShellCommand(context, "yes >" + outputFile);
@@ -56,11 +62,13 @@
queue->addJob(QueueJob((Command*)0x1, fn));
- // Busy wait until `outputFile` appears which indicates that `yes` is running
+ // Busy wait until `outputFile` appears which indicates that `yes` is
+ // running.
time_t start = ::time(NULL);
while (fs->getFileInfo(outputFile).isMissing()) {
if (::time(NULL) > start + 5) {
- // We can't fail gracefully because the `LaneBasedExecutionQueue` will always wait for spawned processes to exit
+ // We can't fail gracefully because the `LaneBasedExecutionQueue` will
+ // always wait for spawned processes to exit
abort();
}
}
@@ -71,7 +79,8 @@
TEST(LaneBasedExecutionQueueTest, exhaustsQueueAfterCancellation) {
DummyDelegate delegate;
- auto queue = std::unique_ptr<BuildExecutionQueue>(createLaneBasedExecutionQueue(delegate, 1));
+ auto queue = std::unique_ptr<BuildExecutionQueue>(
+ createLaneBasedExecutionQueue(delegate, 1, /*environment=*/nullptr));
bool buildStarted { false };
std::condition_variable buildStartedCondition;
