Skip to content

Fix for .when(traits:) condition not working for multiple traits #9015

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.

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions Fixtures/Traits/Example/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ let package = Package(
"BuildCondition1",
"BuildCondition2",
"BuildCondition3",
"ExtraTrait",
],
dependencies: [
.package(
Expand Down Expand Up @@ -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"])),
Expand Down
8 changes: 8 additions & 0 deletions Fixtures/Traits/Example/Sources/Example/Example.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ import Package9Library1
#endif
#if Package10
import Package10Library1
import Package10Library2
#endif
#if ExtraTrait
import Package10Library2
#endif

@main
Expand Down Expand Up @@ -49,6 +53,10 @@ struct Example {
#endif
#if Package10
Package10Library1.hello()
Package10Library2.hello()
#endif
#if ExtraTrait
Package10Library2.hello()
#endif
#if DEFINE1
print("DEFINE1 enabled")
Expand Down
7 changes: 7 additions & 0 deletions Fixtures/Traits/Package10/Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ let package = Package(
name: "Package10Library1",
targets: ["Package10Library1"]
),
.library(
name: "Package10Library2",
targets: ["Package10Library2"]
),
],
traits: [
"Package10Trait1",
Expand All @@ -18,6 +22,9 @@ let package = Package(
.target(
name: "Package10Library1"
),
.target(
name: "Package10Library2"
),
.plugin(
name: "SymbolGraphExtract",
capability: .command(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
public func hello() {
print("Package10Library2 has been included.")
}
17 changes: 10 additions & 7 deletions Sources/PackageModel/Manifest/Manifest+Traits.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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(
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: can we write some automated tests that would validate this function in isolation. This would allow us to validate various scenarios, include fault injection et al. If we are unable to control the test the was we want, we can rework the function to make it testable.

// 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<String>/* = ["default"]*/) throws -> Bool {
public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set<String>) throws -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: can we write some automated tests that would validate this function in isolation. This would allow us to validate various scenarios, include fault injection et al. If we are unable to control the test the was we want, we can rework the function to make it testable.

if self.pruneDependencies {
let usedDependencies = try self.usedDependencies(withTraits: enabledTraits)
let foundKnownPackage = usedDependencies.knownPackage.contains(where: {
Expand All @@ -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
Expand Down
54 changes: 53 additions & 1 deletion Tests/FunctionalTests/TraitTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider adding this as a test parameterization.

Have a look at #9012 that combines BuildSystem and BuildConfiguration into a single array, allow us to send a second array as argument.

Here's that introduces the getBuildData(...) function and here's an application

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)
}
}
}
107 changes: 107 additions & 0 deletions Tests/WorkspaceTests/WorkspaceTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down