From 121a0489352300fa69b6fc749f784d55eb8c6a83 Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Mon, 11 Aug 2025 20:15:14 -0400 Subject: [PATCH 1/2] Fix for .when(traits:) condition not working for multiple traits The culprit here is the fact that we were essentially doing an AND boolean check across all the traits in the target dependency condition, when we should be doing an OR boolean check. If at least one trait is enabled in the list of traits in the target dependency condition, then the target dependecy should be considered enabled as well. --- .../Manifest/Manifest+Traits.swift | 17 +-- Tests/WorkspaceTests/WorkspaceTests.swift | 107 ++++++++++++++++++ 2 files changed, 117 insertions(+), 7 deletions(-) diff --git a/Sources/PackageModel/Manifest/Manifest+Traits.swift b/Sources/PackageModel/Manifest/Manifest+Traits.swift index 9f9364ea3a9..e5258a7a188 100644 --- a/Sources/PackageModel/Manifest/Manifest+Traits.swift +++ b/Sources/PackageModel/Manifest/Manifest+Traits.swift @@ -95,7 +95,7 @@ extension Manifest { _ parentPackage: PackageIdentifier? = nil ) throws { guard supportsTraits else { - if explicitlyEnabledTraits != ["default"] /*!explicitlyEnabledTraits.contains("default")*/ { + if explicitlyEnabledTraits != ["default"] { throw TraitError.traitsNotSupported( parent: parentPackage, package: .init(self), @@ -116,7 +116,7 @@ extension Manifest { let areDefaultsEnabled = enabledTraits.contains("default") // Ensure that disabling default traits is disallowed for packages that don't define any traits. - if !(explicitlyEnabledTraits == nil || areDefaultsEnabled) && !self.supportsTraits { + if !areDefaultsEnabled && !self.supportsTraits { // We throw an error when default traits are disabled for a package without any traits // This allows packages to initially move new API behind traits once. throw TraitError.traitsNotSupported( @@ -449,15 +449,18 @@ extension Manifest { let traitsToEnable = self.traitGuardedTargetDependencies(for: target)[dependency] ?? [] - let isEnabled = try traitsToEnable.allSatisfy { try self.isTraitEnabled( + // Check if any of the traits guarding this dependency is enabled; + // if so, the condition is met and the target dependency is considered + // to be in an enabled state. + let isEnabled = try traitsToEnable.contains(where: { try self.isTraitEnabled( .init(stringLiteral: $0), enabledTraits, - ) } + ) }) return traitsToEnable.isEmpty || isEnabled } /// Determines whether a given package dependency is used by this manifest given a set of enabled traits. - public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set/* = ["default"]*/) throws -> Bool { + public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set) throws -> Bool { if self.pruneDependencies { let usedDependencies = try self.usedDependencies(withTraits: enabledTraits) let foundKnownPackage = usedDependencies.knownPackage.contains(where: { @@ -478,8 +481,8 @@ extension Manifest { // if target deps is empty, default to returning true here. let isTraitGuarded = targetDependenciesForPackageDependency.isEmpty ? false : targetDependenciesForPackageDependency.compactMap({ $0.condition?.traits }).allSatisfy({ - let condition = $0.subtracting(enabledTraits) - return !condition.isEmpty + let isGuarded = $0.intersection(enabledTraits).isEmpty + return isGuarded }) let isUsedWithoutTraitGuarding = !targetDependenciesForPackageDependency.filter({ $0.condition?.traits == nil }).isEmpty diff --git a/Tests/WorkspaceTests/WorkspaceTests.swift b/Tests/WorkspaceTests/WorkspaceTests.swift index 667d61aee2f..cdfd563f3ec 100644 --- a/Tests/WorkspaceTests/WorkspaceTests.swift +++ b/Tests/WorkspaceTests/WorkspaceTests.swift @@ -16257,6 +16257,113 @@ final class WorkspaceTests: XCTestCase { } } + func testManyTraitsEnableTargetDependency() async throws { + let sandbox = AbsolutePath("/tmp/ws/") + let fs = InMemoryFileSystem() + + func createMockWorkspace(_ traitConfiguration: TraitConfiguration) async throws -> MockWorkspace { + try await MockWorkspace( + sandbox: sandbox, + fileSystem: fs, + roots: [ + MockPackage( + name: "Cereal", + targets: [ + MockTarget( + name: "Wheat", + dependencies: [ + .product( + name: "Icing", + package: "Sugar", + condition: .init(traits: ["BreakfastOfChampions", "DontTellMom"]) + ), + ] + ), + ], + products: [ + MockProduct(name: "YummyBreakfast", modules: ["Wheat"]) + ], + dependencies: [ + .sourceControl(path: "./Sugar", requirement: .upToNextMajor(from: "1.0.0")), + ], + traits: ["BreakfastOfChampions", "DontTellMom"] + ), + ], + packages: [ + MockPackage( + name: "Sugar", + targets: [ + MockTarget(name: "Icing"), + ], + products: [ + MockProduct(name: "Icing", modules: ["Icing"]), + ], + versions: ["1.0.0", "1.5.0"] + ), + ], + traitConfiguration: traitConfiguration + ) + } + + + let deps: [MockDependency] = [ + .sourceControl(path: "./Sugar", requirement: .exact("1.0.0"), products: .specific(["Icing"])), + ] + + let workspaceOfChampions = try await createMockWorkspace(.enabledTraits(["BreakfastOfChampions"])) + try await workspaceOfChampions.checkPackageGraph(roots: ["Cereal"], deps: deps) { graph, diagnostics in + XCTAssertNoDiagnostics(diagnostics) + PackageGraphTesterXCTest(graph) { result in + result.check(roots: "Cereal") + result.check(packages: "cereal", "sugar") + result.check(modules: "Wheat", "Icing") + result.check(products: "YummyBreakfast", "Icing") + result.checkTarget("Wheat") { result in + result.check(dependencies: "Icing") + } + } + } + + let dontTellMomAboutThisWorkspace = try await createMockWorkspace(.enabledTraits(["DontTellMom"])) + try await dontTellMomAboutThisWorkspace.checkPackageGraph(roots: ["Cereal"], deps: deps) { graph, diagnostics in + XCTAssertNoDiagnostics(diagnostics) + PackageGraphTesterXCTest(graph) { result in + result.check(roots: "Cereal") + result.check(packages: "cereal", "sugar") + result.check(modules: "Wheat", "Icing") + result.check(products: "YummyBreakfast", "Icing") + result.checkTarget("Wheat") { result in + result.check(dependencies: "Icing") + } + } + } + + let allEnabledTraitsWorkspace = try await createMockWorkspace(.enableAllTraits) + try await allEnabledTraitsWorkspace.checkPackageGraph(roots: ["Cereal"], deps: deps) { graph, diagnostics in + XCTAssertNoDiagnostics(diagnostics) + PackageGraphTesterXCTest(graph) { result in + result.check(roots: "Cereal") + result.check(packages: "cereal", "sugar") + result.check(modules: "Wheat", "Icing") + result.check(products: "YummyBreakfast", "Icing") + result.checkTarget("Wheat") { result in + result.check(dependencies: "Icing") + } + } + } + + let noSugarForBreakfastWorkspace = try await createMockWorkspace(.disableAllTraits) + try await noSugarForBreakfastWorkspace.checkPackageGraph(roots: ["Cereal"], deps: deps) { graph, diagnostics in + XCTAssertNoDiagnostics(diagnostics) + PackageGraphTesterXCTest(graph) { result in + result.check(roots: "Cereal") + result.check(packages: "cereal") + result.check(modules: "Wheat") + result.check(products: "YummyBreakfast") + } + } + } + func makeRegistryClient( packageIdentity: PackageIdentity, packageVersion: Version, From c5b78899d770126d85e62d70919a5df43584ec8f Mon Sep 17 00:00:00 2001 From: Bri Peticca Date: Wed, 13 Aug 2025 16:28:11 -0400 Subject: [PATCH 2/2] Update trait fixtures to include test case for this scenario --- Fixtures/Traits/Example/Package.swift | 6 +++ .../Example/Sources/Example/Example.swift | 8 +++ Fixtures/Traits/Package10/Package.swift | 7 +++ .../Sources/Package10Library2/Library.swift | 3 ++ Tests/FunctionalTests/TraitTests.swift | 54 ++++++++++++++++++- 5 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 Fixtures/Traits/Package10/Sources/Package10Library2/Library.swift diff --git a/Fixtures/Traits/Example/Package.swift b/Fixtures/Traits/Example/Package.swift index 6eaf1479afa..b5358cb9d3b 100644 --- a/Fixtures/Traits/Example/Package.swift +++ b/Fixtures/Traits/Example/Package.swift @@ -25,6 +25,7 @@ let package = Package( "BuildCondition1", "BuildCondition2", "BuildCondition3", + "ExtraTrait", ], dependencies: [ .package( @@ -101,6 +102,11 @@ let package = Package( package: "Package10", condition: .when(traits: ["Package10"]) ), + .product( + name: "Package10Library2", + package: "Package10", + condition: .when(traits: ["Package10", "ExtraTrait"]) + ) ], swiftSettings: [ .define("DEFINE1", .when(traits: ["BuildCondition1"])), diff --git a/Fixtures/Traits/Example/Sources/Example/Example.swift b/Fixtures/Traits/Example/Sources/Example/Example.swift index 5f1d871df50..d1edafb6bb2 100644 --- a/Fixtures/Traits/Example/Sources/Example/Example.swift +++ b/Fixtures/Traits/Example/Sources/Example/Example.swift @@ -21,6 +21,10 @@ import Package9Library1 #endif #if Package10 import Package10Library1 +import Package10Library2 +#endif +#if ExtraTrait +import Package10Library2 #endif @main @@ -49,6 +53,10 @@ struct Example { #endif #if Package10 Package10Library1.hello() + Package10Library2.hello() + #endif + #if ExtraTrait + Package10Library2.hello() #endif #if DEFINE1 print("DEFINE1 enabled") diff --git a/Fixtures/Traits/Package10/Package.swift b/Fixtures/Traits/Package10/Package.swift index 4236e806cff..60767db5e7a 100644 --- a/Fixtures/Traits/Package10/Package.swift +++ b/Fixtures/Traits/Package10/Package.swift @@ -9,6 +9,10 @@ let package = Package( name: "Package10Library1", targets: ["Package10Library1"] ), + .library( + name: "Package10Library2", + targets: ["Package10Library2"] + ), ], traits: [ "Package10Trait1", @@ -18,6 +22,9 @@ let package = Package( .target( name: "Package10Library1" ), + .target( + name: "Package10Library2" + ), .plugin( name: "SymbolGraphExtract", capability: .command( diff --git a/Fixtures/Traits/Package10/Sources/Package10Library2/Library.swift b/Fixtures/Traits/Package10/Sources/Package10Library2/Library.swift new file mode 100644 index 00000000000..8d1dd343c75 --- /dev/null +++ b/Fixtures/Traits/Package10/Sources/Package10Library2/Library.swift @@ -0,0 +1,3 @@ +public func hello() { + print("Package10Library2 has been included.") +} \ No newline at end of file diff --git a/Tests/FunctionalTests/TraitTests.swift b/Tests/FunctionalTests/TraitTests.swift index 9475ab9a5d8..050e05ca359 100644 --- a/Tests/FunctionalTests/TraitTests.swift +++ b/Tests/FunctionalTests/TraitTests.swift @@ -454,7 +454,7 @@ struct TraitTests { let json = try JSON(bytes: ByteString(encodingAsUTF8: dumpOutput)) guard case .dictionary(let contents) = json else { Issue.record("unexpected result"); return } guard case .array(let traits)? = contents["traits"] else { Issue.record("unexpected result"); return } - #expect(traits.count == 12) + #expect(traits.count == 13) } } @@ -653,4 +653,56 @@ struct TraitTests { } } } + + @Test( + .IssueSwiftBuildLinuxRunnable, + .IssueProductTypeForObjectLibraries, + .tags( + Tag.Feature.Command.Run, + ), + arguments: SupportedBuildSystemOnAllPlatforms, BuildConfiguration.allCases, + ) + func traits_whenManyTraitsEnableTargetDependency( + buildSystem: BuildSystemProvider.Kind, + configuration: BuildConfiguration, + ) async throws { + try await withKnownIssue( + """ + Linux: https://github.com/swiftlang/swift-package-manager/issues/8416, + Windows: https://github.com/swiftlang/swift-build/issues/609 + """, + isIntermittent: (ProcessInfo.hostOperatingSystem == .windows), + ) { + try await fixture(name: "Traits") { fixturePath in + // Test various combinations of traits that would + // enable the dependency on Package10Library2 + let traitCombinations = ["ExtraTrait", "Package10", "ExtraTrait,Package10"] + // We expect no warnings to be produced. Specifically no unused dependency warnings. + let unusedDependencyRegex = try Regex("warning: '.*': dependency '.*' is not used by any target") + + for traits in traitCombinations { + let (stdout, stderr) = try await executeSwiftRun( + fixturePath.appending("Example"), + "Example", + configuration: configuration, + extraArgs: ["--traits", traits], + buildSystem: buildSystem, + ) + + var prefix = traits.contains("Package10") ? "Package10Library1 trait1 disabled\nPackage10Library1 trait2 enabled\nPackage10Library2 has been included.\n" : "" + prefix += traits.contains("ExtraTrait") ? "Package10Library2 has been included.\n" : "" + #expect(!stderr.contains(unusedDependencyRegex)) + #expect(stdout == """ + \(prefix)DEFINE1 disabled + DEFINE2 disabled + DEFINE3 disabled + + """) + } + } + } when: { + (ProcessInfo.hostOperatingSystem == .windows && (CiEnvironment.runningInSmokeTestPipeline || buildSystem == .swiftbuild)) + || (buildSystem == .swiftbuild && ProcessInfo.hostOperatingSystem == .linux && CiEnvironment.runningInSelfHostedPipeline) + } + } }