-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
public func hello() { | ||
print("Package10Library2 has been included.") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<String>/* = ["default"]*/) throws -> Bool { | ||
public func isPackageDependencyUsed(_ dependency: PackageDependency, enabledTraits: Set<String>) throws -> Bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: { | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Here's that introduces the |
||
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) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.