Skip to content

Set inherit-env=false and merge processEnvironment into task environments #1001

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

Open
PhantomInTheWire wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Set inherit-env=false and merge processEnvironment into task environments #1001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b87b736
Set inherit-env=false and merge processEnvironment into task environm…
PhantomInTheWire Jan 2, 2026
b58ab38
fix: windows ci
PhantomInTheWire Jan 7, 2026
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
23 changes: 14 additions & 9 deletions Sources/SWBTaskExecution/BuildDescription.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,11 @@ package final class BuildDescriptionBuilder {
/// - Parameters:
/// - path: The path of a directory to store the build description to.
/// - bypassActualTasks: If enabled, replace tasks with fake ones (`/usr/bin/true`).
init(path: Path, signature: BuildDescriptionSignature, buildCommand: BuildCommand, taskAdditionalInputs: [Ref<any PlannedTask>: NodeList], mutatedNodes: Set<Ref<any PlannedNode>>, mutatingTasks: [Ref<any PlannedTask>: MutatingTaskInfo], bypassActualTasks: Bool, targetsBuildInParallel: Bool, emitFrontendCommandLines: Bool, moduleSessionFilePath: Path?, invalidationPaths: [Path], recursiveSearchPathResults: [RecursiveSearchPathResolver.CachedResult], copiedPathMap: [String: String], outputPathsPerTarget: [ConfiguredTarget?: [Path]], allOutputPaths: Set<Path>, rootPathsPerTarget: [ConfiguredTarget: [Path]], moduleCachePathsPerTarget: [ConfiguredTarget: [Path]], artifactInfoPerTarget: [ConfiguredTarget: ArtifactInfo], casValidationInfos: [BuildDescription.CASValidationInfo], staleFileRemovalIdentifierPerTarget: [ConfiguredTarget?: String], settingsPerTarget: [ConfiguredTarget: Settings], targetDependencies: [TargetDependencyRelationship], definingTargetsByModuleName: [String: OrderedSet<ConfiguredTarget>], workspace: Workspace) {

/// The process environment to merge into task environments.
private let processEnvironment: [String: String]

init(path: Path, signature: BuildDescriptionSignature, buildCommand: BuildCommand, taskAdditionalInputs: [Ref<any PlannedTask>: NodeList], mutatedNodes: Set<Ref<any PlannedNode>>, mutatingTasks: [Ref<any PlannedTask>: MutatingTaskInfo], bypassActualTasks: Bool, targetsBuildInParallel: Bool, emitFrontendCommandLines: Bool, moduleSessionFilePath: Path?, invalidationPaths: [Path], recursiveSearchPathResults: [RecursiveSearchPathResolver.CachedResult], copiedPathMap: [String: String], outputPathsPerTarget: [ConfiguredTarget?: [Path]], allOutputPaths: Set<Path>, rootPathsPerTarget: [ConfiguredTarget: [Path]], moduleCachePathsPerTarget: [ConfiguredTarget: [Path]], artifactInfoPerTarget: [ConfiguredTarget: ArtifactInfo], casValidationInfos: [BuildDescription.CASValidationInfo], staleFileRemovalIdentifierPerTarget: [ConfiguredTarget?: String], settingsPerTarget: [ConfiguredTarget: Settings], targetDependencies: [TargetDependencyRelationship], definingTargetsByModuleName: [String: OrderedSet<ConfiguredTarget>], workspace: Workspace, processEnvironment: [String: String]) {
self.path = path
self.signature = signature
self.taskAdditionalInputs = taskAdditionalInputs
Expand All @@ -586,6 +590,7 @@ package final class BuildDescriptionBuilder {
self.settingsPerTarget = settingsPerTarget
self.targetDependencies = targetDependencies
self.definingTargetsByModuleName = definingTargetsByModuleName
self.processEnvironment = processEnvironment
self.taskStore = TaskStore()
}

Expand Down Expand Up @@ -904,11 +909,11 @@ package final class BuildDescriptionBuilder {
$0["inputs"] = inputs.map { $0.identifier }
$0["outputs"] = outputs.map { $0.identifier }
$0["args"] = commandLine
// FIXME: inherit-env defaults to true; we should pass false and inject the process environment explicitly, so that it is overridable for tests
// $0["inherit-env"] = "true"
if let environment {
$0["env"] = environment.bindings
}
// Merge processEnvironment with task environment; task bindings take precedence.
let taskEnvDict = environment?.bindings.reduce(into: [String: String]()) { $0[$1.0] = $1.1 } ?? [:]
let mergedEnvDict = processEnvironment.merging(taskEnvDict, uniquingKeysWith: { _, taskValue in taskValue })
let mergedEnvBindings = mergedEnvDict.map { ($0.key, $0.value) }.sorted(by: { $0.0 < $1.0 })
$0["env"] = mergedEnvBindings
if allowMissingInputs {
$0["allow-missing-inputs"] = true
}
Expand All @@ -935,7 +940,7 @@ package final class BuildDescriptionBuilder {

// We always compute the signature ourselves instead of letting llbuild use its default logic.
// However, we currently do use roughly the same information to compute it.
let signature = Self.computeShellToolSignature(args: task.type.commandLineForSignature(for: task.execTask) ?? commandLine, environment: environment, dependencyData: deps, isUnsafeToInterrupt: isUnsafeToInterrupt, additionalSignatureData: task.additionalSignatureData)
let signature = Self.computeShellToolSignature(args: task.type.commandLineForSignature(for: task.execTask) ?? commandLine, environment: EnvironmentBindings(mergedEnvBindings), dependencyData: deps, isUnsafeToInterrupt: isUnsafeToInterrupt, additionalSignatureData: task.additionalSignatureData)

$0["signature"] = signature
}
Expand Down Expand Up @@ -1027,7 +1032,7 @@ extension BuildDescription {
// FIXME: Bypass actual tasks should go away, eventually.
//
// FIXME: This layering isn't working well, we are plumbing a bunch of stuff through here just because we don't want to talk to TaskConstruction.
static package func construct(workspace: Workspace, tasks: [any PlannedTask], path: Path, signature: BuildDescriptionSignature, buildCommand: BuildCommand, diagnostics: [ConfiguredTarget?: [Diagnostic]] = [:], indexingInfo: [(forTarget: ConfiguredTarget?, path: Path, indexingInfo: any SourceFileIndexingInfo)] = [], fs: any FSProxy = localFS, bypassActualTasks: Bool = false, targetsBuildInParallel: Bool = true, emitFrontendCommandLines: Bool = false, moduleSessionFilePath: Path? = nil, invalidationPaths: [Path] = [], recursiveSearchPathResults: [RecursiveSearchPathResolver.CachedResult] = [], copiedPathMap: [String: String] = [:], rootPathsPerTarget: [ConfiguredTarget:[Path]] = [:], moduleCachePathsPerTarget: [ConfiguredTarget: [Path]] = [:], artifactInfoPerTarget: [ConfiguredTarget: ArtifactInfo] = [:], casValidationInfos: [BuildDescription.CASValidationInfo] = [], staleFileRemovalIdentifierPerTarget: [ConfiguredTarget?: String] = [:], settingsPerTarget: [ConfiguredTarget: Settings] = [:], delegate: any BuildDescriptionConstructionDelegate, targetDependencies: [TargetDependencyRelationship] = [], definingTargetsByModuleName: [String: OrderedSet<ConfiguredTarget>], userPreferences: UserPreferences) async throws -> BuildDescription? {
static package func construct(workspace: Workspace, tasks: [any PlannedTask], path: Path, signature: BuildDescriptionSignature, buildCommand: BuildCommand, diagnostics: [ConfiguredTarget?: [Diagnostic]] = [:], indexingInfo: [(forTarget: ConfiguredTarget?, path: Path, indexingInfo: any SourceFileIndexingInfo)] = [], fs: any FSProxy = localFS, bypassActualTasks: Bool = false, targetsBuildInParallel: Bool = true, emitFrontendCommandLines: Bool = false, moduleSessionFilePath: Path? = nil, invalidationPaths: [Path] = [], recursiveSearchPathResults: [RecursiveSearchPathResolver.CachedResult] = [], copiedPathMap: [String: String] = [:], rootPathsPerTarget: [ConfiguredTarget:[Path]] = [:], moduleCachePathsPerTarget: [ConfiguredTarget: [Path]] = [:], artifactInfoPerTarget: [ConfiguredTarget: ArtifactInfo] = [:], casValidationInfos: [BuildDescription.CASValidationInfo] = [], staleFileRemovalIdentifierPerTarget: [ConfiguredTarget?: String] = [:], settingsPerTarget: [ConfiguredTarget: Settings] = [:], delegate: any BuildDescriptionConstructionDelegate, targetDependencies: [TargetDependencyRelationship] = [], definingTargetsByModuleName: [String: OrderedSet<ConfiguredTarget>], userPreferences: UserPreferences, processEnvironment: [String: String] = [:]) async throws -> BuildDescription? {
var diagnostics = diagnostics

// We operate on the sorted tasks here to ensure that the list of task additional inputs is deterministic.
Expand Down Expand Up @@ -1290,7 +1295,7 @@ extension BuildDescription {
}

// Create the builder.
let builder = BuildDescriptionBuilder(path: path, signature: signature, buildCommand: buildCommand, taskAdditionalInputs: taskAdditionalInputs, mutatedNodes: Set(mutableNodes.keys), mutatingTasks: mutatingTasks, bypassActualTasks: bypassActualTasks, targetsBuildInParallel: targetsBuildInParallel, emitFrontendCommandLines: emitFrontendCommandLines, moduleSessionFilePath: moduleSessionFilePath, invalidationPaths: invalidationPaths, recursiveSearchPathResults: recursiveSearchPathResults, copiedPathMap: copiedPathMap, outputPathsPerTarget: outputPathsPerTarget, allOutputPaths: Set(producers.keys.map { $0.instance.path }), rootPathsPerTarget: rootPathsPerTarget, moduleCachePathsPerTarget: moduleCachePathsPerTarget, artifactInfoPerTarget: artifactInfoPerTarget, casValidationInfos: casValidationInfos, staleFileRemovalIdentifierPerTarget: staleFileRemovalIdentifierPerTarget, settingsPerTarget: settingsPerTarget, targetDependencies: targetDependencies, definingTargetsByModuleName: definingTargetsByModuleName, workspace: workspace)
let builder = BuildDescriptionBuilder(path: path, signature: signature, buildCommand: buildCommand, taskAdditionalInputs: taskAdditionalInputs, mutatedNodes: Set(mutableNodes.keys), mutatingTasks: mutatingTasks, bypassActualTasks: bypassActualTasks, targetsBuildInParallel: targetsBuildInParallel, emitFrontendCommandLines: emitFrontendCommandLines, moduleSessionFilePath: moduleSessionFilePath, invalidationPaths: invalidationPaths, recursiveSearchPathResults: recursiveSearchPathResults, copiedPathMap: copiedPathMap, outputPathsPerTarget: outputPathsPerTarget, allOutputPaths: Set(producers.keys.map { $0.instance.path }), rootPathsPerTarget: rootPathsPerTarget, moduleCachePathsPerTarget: moduleCachePathsPerTarget, artifactInfoPerTarget: artifactInfoPerTarget, casValidationInfos: casValidationInfos, staleFileRemovalIdentifierPerTarget: staleFileRemovalIdentifierPerTarget, settingsPerTarget: settingsPerTarget, targetDependencies: targetDependencies, definingTargetsByModuleName: definingTargetsByModuleName, workspace: workspace, processEnvironment: processEnvironment)
for (target, diagnostics) in diagnostics {
let engine = builder.diagnosticsEngines.getOrInsert(target, { DiagnosticsEngine() })
for diag in diagnostics {
Expand Down
2 changes: 1 addition & 1 deletion Sources/SWBTaskExecution/BuildDescriptionManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ package final class BuildDescriptionManager: Sendable {
}()

// Create the build description.
return try await BuildDescription.construct(workspace: planRequest.workspaceContext.workspace, tasks: plan.tasks, path: path, signature: signature, buildCommand: planRequest.buildRequest.buildCommand, diagnostics: planningDiagnostics, indexingInfo: [], fs: fs, bypassActualTasks: bypassActualTasks, targetsBuildInParallel: buildGraph.targetsBuildInParallel, emitFrontendCommandLines: plan.emitFrontendCommandLines, moduleSessionFilePath: planRequest.workspaceContext.getModuleSessionFilePath(planRequest.buildRequest.parameters), invalidationPaths: plan.invalidationPaths, recursiveSearchPathResults: plan.recursiveSearchPathResults, copiedPathMap: plan.copiedPathMap, rootPathsPerTarget: rootPathsPerTarget, moduleCachePathsPerTarget: moduleCachePathsPerTarget, artifactInfoPerTarget: artifactInfoPerTarget, casValidationInfos: casValidationInfos.elements, staleFileRemovalIdentifierPerTarget: staleFileRemovalIdentifierPerTarget, settingsPerTarget: settingsPerTarget, delegate: delegate, targetDependencies: buildGraph.targetDependenciesByGuid, definingTargetsByModuleName: definingTargetsByModuleName, userPreferences: planRequest.workspaceContext.userPreferences)
return try await BuildDescription.construct(workspace: planRequest.workspaceContext.workspace, tasks: plan.tasks, path: path, signature: signature, buildCommand: planRequest.buildRequest.buildCommand, diagnostics: planningDiagnostics, indexingInfo: [], fs: fs, bypassActualTasks: bypassActualTasks, targetsBuildInParallel: buildGraph.targetsBuildInParallel, emitFrontendCommandLines: plan.emitFrontendCommandLines, moduleSessionFilePath: planRequest.workspaceContext.getModuleSessionFilePath(planRequest.buildRequest.parameters), invalidationPaths: plan.invalidationPaths, recursiveSearchPathResults: plan.recursiveSearchPathResults, copiedPathMap: plan.copiedPathMap, rootPathsPerTarget: rootPathsPerTarget, moduleCachePathsPerTarget: moduleCachePathsPerTarget, artifactInfoPerTarget: artifactInfoPerTarget, casValidationInfos: casValidationInfos.elements, staleFileRemovalIdentifierPerTarget: staleFileRemovalIdentifierPerTarget, settingsPerTarget: settingsPerTarget, delegate: delegate, targetDependencies: buildGraph.targetDependenciesByGuid, definingTargetsByModuleName: definingTargetsByModuleName, userPreferences: planRequest.workspaceContext.userPreferences, processEnvironment: planRequest.workspaceContext.userInfo?.processEnvironment ?? [:])
}

/// Encapsulates the two ways `getNewOrCachedBuildDescription` can be called, whether we want to retrieve or create a build description based on a plan or whether we have an explicit build description ID that we want to retrieve and we don't need to create a new one.
Expand Down
4 changes: 4 additions & 0 deletions Sources/SWBWindowsPlatform/Specs/WindowsLd.xcspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@
CommandLineFlag = "-sdk";
IsInputDependency = Yes;
},
{
Name = LD_OBJC_RUNTIME_ARGS;
DefaultValue = "";
},
{
Name = _LD_MULTIARCH;
Type = Boolean;
Expand Down
1 change: 1 addition & 0 deletions Tests/SWBBuildSystemTests/BuildOperationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,7 @@ fileprivate struct BuildOperationTests: CoreBasedTests {

tester.userInfo = UserInfo(user: "exampleUser", group: "exampleGroup", uid: 1234, gid:12345, home: Path("/Users/exampleUser"), environment: [
"ENV_KEY": "ENV_VALUE"])
tester.workspaceContext.updateUserInfo(tester.userInfo)

try await tester.checkBuild(runDestination: .host) { results in
// We expect one task with one line of output.
Expand Down
14 changes: 5 additions & 9 deletions Tests/SWBBuildSystemTests/ClangExplicitModulesTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,7 @@ fileprivate struct ClangExplicitModulesTests: CoreBasedTests {
}
}

@Test(.requireSDKs(.macOS), .disabled("setting global environment interferes concurrent tests"),
.bug("https://github.com/swiftlang/swift-build/issues/835"))
@Test(.requireSDKs(.macOS))
func explicitModulesEnvironment() async throws {
try await withTemporaryDirectory { tmpDirPath in
let testWorkspace = TestWorkspace(
Expand All @@ -392,8 +391,7 @@ fileprivate struct ClangExplicitModulesTests: CoreBasedTests {
"Debug",
buildSettings: [
"PRODUCT_NAME": "$(TARGET_NAME)",
"CLANG_ENABLE_MODULES": "YES",
Copy link
Collaborator

@owenv owenv Jan 5, 2026

Choose a reason for hiding this comment

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

This test is intended to check the behavior of modular builds, so we shouldn't drop these overrides

Copy link
Member Author

@PhantomInTheWire PhantomInTheWire Jan 5, 2026

Choose a reason for hiding this comment

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

When modules are enabled, Swift Build invokes clang -cc1 directly instead of the driver. The GCC_TREAT_WARNINGS_AS_ERRORS env var is only recognized by the clang driver, not -cc1 frontend mode, so the test mechanism doesn't work with modular builds. The userEnvironment test confirms env passing works correctly. How should we verify environment propagation in this test given the -cc1 limitation?

reproduction:

# Works (driver mode) - produces warning, not error:
GCC_TREAT_WARNINGS_AS_ERRORS=NO clang -Wmain -Werror -c file.c
# Fails (frontend mode) - still errors:
GCC_TREAT_WARNINGS_AS_ERRORS=NO clang -cc1 -Wmain -Werror -emit-obj file.c

Copy link
Collaborator

@owenv owenv Jan 5, 2026

Choose a reason for hiding this comment

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

In these cases the environment is read by the clang dependency scanner running in-process to construct the cc1 commands. We'd likely need a new entry point to clang_experimental_DependencyScannerWorkerScanSettings_create to inject a custom environment without setting it process-wide

Copy link
Member Author

@PhantomInTheWire PhantomInTheWire Jan 7, 2026

Choose a reason for hiding this comment

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

this would need a new libclang API. the current clang_experimental_DependencyScannerWorkerScanSettings_create only takes argc, argv, ModuleName, WorkingDirectory, and the module lookup callback, no environment parameter exists.

should i keep the tests like this(current approach) to validate the inherit-env=false fix works for task execution
or Keep original modular test but mark it .disabled with a .bug() annotation pointing to a tracking issue. or something else that i might be missing?

"_EXPERIMENTAL_CLANG_EXPLICIT_MODULES": "YES",
"CLANG_ENABLE_MODULES": "NO",
"OTHER_CFLAGS": "-Wmain -Werror",
])],
targets: [
Expand All @@ -414,14 +412,12 @@ fileprivate struct ClangExplicitModulesTests: CoreBasedTests {
"""
}

// FIXME: These two lines shouldn't be necessary. clang directly recognizes the same of this environment variable (coincidentally the same as our build setting). llbuild's shell tool has an `inherit-env` property which is true by default, and causes the _process_ environment of the build service to be propagated to build tasks like clang. Instead we should set `inherit-env` to false and merge `processEnvironment` from UserInfo into the task's environment, so that it is overridable from tests.
try POSIX.setenv("GCC_TREAT_WARNINGS_AS_ERRORS", "NO", 1)
defer { try? POSIX.unsetenv("GCC_TREAT_WARNINGS_AS_ERRORS") }

// Override GCC_TREAT_WARNINGS_AS_ERRORS via processEnvironment
tester.userInfo = UserInfo(user: "user", group: "group", uid: 1337, gid: 42, home: tmpDirPath.join("home"), processEnvironment: [:], buildSystemEnvironment: [:])
tester.userInfo = tester.userInfo.withAdditionalEnvironment(environment: ["GCC_TREAT_WARNINGS_AS_ERRORS": "NO"])
tester.workspaceContext.updateUserInfo(tester.userInfo)
try await tester.checkBuild(runDestination: .macOS) { results in
// Check that -Werror was overwritten by GCC_TREAT_WARNINGS_AS_ERRORS=NO.
// GCC_TREAT_WARNINGS_AS_ERRORS=NO should override -Werror
results.checkNoErrors()
results.checkWarning(.contains("only one parameter on 'main'"))
}
Expand Down