Fix for .when(traits:) condition not working for multiple traits #9015
Conversation
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.
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test windows |
|
|
||
| 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.
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.
| @@ -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; | |||
There was a problem hiding this comment.
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.
| // 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.
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
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.