Skip to content

Commit 76e7221

Browse files
bripeticcabripeticca
authored
[6.2] Cherry-pick: Fix pre-computation of traits when loading dependency manifests (#9136)
- **Explanation**: Fixes some errors in trait computation that hadn't considered when default traits `["default"]` were being appended to the `EnabledTraitsMap`, which should only consider a list of flattened traits + *explicitly enabled traits* by a user/parent package. The inclusion of `default` in this dictionary was resulting in an inaccurate computation of which traits were enabled, since pre-computation serves to flatten the list of transitively enabled traits for future reference. There were also cases where we were re-computing transitively enabled traits in areas where we had already filled out the `enabledTraitsMap`, so we now default to simply fetching the entry from the dictionary instead of computing the traits all over again. - **Scope**: Fixes trait-related computation in dependency resolution - **Issues**: - **Original PRs**: #9057 - **Risk**: Low risk - **Testing**: Added new fixtures + regression tests to address the behaviour that was previously incorrect. - **Reviewers**: @dschaefer2
1 parent 96e6ae1 commit 76e7221

File tree

11 files changed

+371
-83
lines changed

11 files changed

+371
-83
lines changed

‎Fixtures/Traits/PackageConditionalDeps/Package.swift‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// swift-tools-version: 6.1
2+
3+
import PackageDescription
4+
5+
let package = Package(
6+
name: "PackageConditionalDeps",
7+
products: [
8+
.library(
9+
name: "PackageConditionalDeps",
10+
targets: ["PackageConditionalDeps"]
11+
),
12+
],
13+
traits: [
14+
.default(enabledTraits: ["EnablePackage1Dep"]),
15+
"EnablePackage1Dep",
16+
"EnablePackage2Dep"
17+
],
18+
dependencies: [
19+
.package(path: "../Package1"),
20+
.package(path: "../Package2"),
21+
],
22+
targets: [
23+
.target(
24+
name: "PackageConditionalDeps",
25+
dependencies: [
26+
.product(
27+
name: "Package1Library1",
28+
package: "Package1",
29+
condition: .when(traits: ["EnablePackage1Dep"])
30+
),
31+
.product(
32+
name: "Package2Library1",
33+
package: "Package2",
34+
condition: .when(traits: ["EnablePackage2Dep"])
35+
)
36+
]
37+
),
38+
]
39+
)

‎Fixtures/Traits/PackageConditionalDeps/Sources/PackageConditionalDeps/PackageConditionalDeps.swift‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
public func nothingHappens() {
2+
// Do nothing.
3+
}

‎Sources/PackageGraph/ModulesGraph.swift‎

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -497,26 +497,14 @@ public func loadModulesGraph(
497497
return condition.isSatisfied(by: parentTraits)
498498
}.map(\.name)
499499

500-
var enabledTraitsSet = explicitlyEnabledTraits.flatMap { Set($0) }
501-
let precomputedTraits = enabledTraitsMap[dependency.identity]
502-
503-
if precomputedTraits == ["default"],
504-
let enabledTraitsSet {
505-
enabledTraitsMap[dependency.identity] = enabledTraitsSet
506-
} else {
507-
// unify traits
508-
enabledTraitsSet?.formUnion(precomputedTraits)
509-
if let enabledTraitsSet {
510-
enabledTraitsMap[dependency.identity] = enabledTraitsSet
511-
}
500+
if let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) }) {
501+
let calculatedTraits = try manifest.enabledTraits(
502+
using: enabledTraitsSet,
503+
.init(parent)
504+
)
505+
enabledTraitsMap[dependency.identity] = calculatedTraits
512506
}
513507

514-
let calculatedTraits = try manifest.enabledTraits(
515-
using: enabledTraitsSet ?? ["default"],
516-
.init(parent)
517-
)
518-
519-
enabledTraitsMap[dependency.identity] = calculatedTraits
520508
let result = visited.insert(dependency.identity)
521509
if result.inserted {
522510
try dependencies(of: manifest, dependency.productFilter)

‎Sources/PackageGraph/PackageGraphRoot.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,8 +164,8 @@ public struct PackageGraphRoot {
164164
return condition.isSatisfied(by: rootEnabledTraits)
165165
}.map(\.name)
166166

167-
var enabledTraitsSet = enabledTraits.flatMap { Set($0) } ?? ["default"]
168-
enabledTraitsSet.formUnion(enabledTraitsMap[dep.identity])
167+
var enabledTraitsSet = enabledTraitsMap[dep.identity]
168+
enabledTraitsSet.formUnion(enabledTraits.flatMap({ Set($0) }) ?? [])
169169

170170
return PackageContainerConstraint(
171171
package: dep.packageRef,

‎Sources/PackageGraph/PackageModel+Extensions.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ extension Manifest {
4242
return condition.isSatisfied(by: enabledTraits)
4343
}.map(\.name)
4444

45-
var enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) }) ?? ["default"]
45+
let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) }) ?? ["default"]
4646

4747
return PackageContainerConstraint(
4848
package: $0.packageRef,

‎Sources/PackageModel/EnabledTraitsMap.swift‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,22 @@ public struct EnabledTraitsMap: ExpressibleByDictionaryLiteral {
3131

3232
public subscript(key: PackageIdentity) -> Set<String> {
3333
get { storage[key] ?? ["default"] }
34-
set { storage[key] = newValue }
34+
set {
35+
// Omit adding "default" explicitly, since the map returns "default"
36+
// if there is no explicit traits declared. This will allow us to check
37+
// for nil entries in the stored dictionary, which tells us whether
38+
// traits have been explicitly declared.
39+
guard newValue != ["default"] else { return }
40+
if storage[key] == nil {
41+
storage[key] = newValue
42+
} else {
43+
storage[key]?.formUnion(newValue)
44+
}
45+
}
46+
}
47+
48+
public subscript(explicitlyEnabledTraitsFor key: PackageIdentity) -> Set<String>? {
49+
get { storage[key] }
3550
}
3651

3752
public var dictionaryLiteral: [PackageIdentity: Set<String>] {

‎Sources/PackageModel/Manifest/Manifest+Traits.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ extension Manifest {
313313
let areDefaultsEnabled = enabledTraits.remove("default") != nil
314314

315315
// We have to enable all default traits if no traits are enabled or the defaults are explicitly enabled
316-
if /*explictlyEnabledTraits == nil*//*enabledTraits.isEmpty && */explictlyEnabledTraits == ["default"] || areDefaultsEnabled {
316+
if explictlyEnabledTraits == ["default"] || areDefaultsEnabled {
317317
if let defaultTraits {
318318
enabledTraits.formUnion(defaultTraits.flatMap(\.enabledTraits))
319319
}

‎Sources/Workspace/Workspace+Manifests.swift‎

Lines changed: 21 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -313,26 +313,15 @@ extension Workspace {
313313
}
314314
}
315315

316-
// should calculate enabled traits here.
317-
let explicitlyEnabledTraits = dependency.traits?.filter {
318-
guard let condition = $0.condition else { return true }
319-
return condition.isSatisfied(by: node.enabledTraits)
320-
}.map(\.name)
316+
let enabledTraitsSet = workspace.enabledTraitsMap[dependency.identity]
321317

322318
return try manifestsMap[dependency.identity].map { manifest in
323-
// Calculate all transitively enabled traits for this manifest.
324-
325-
var allEnabledTraits: Set<String> = ["default"]
326-
if let explicitlyEnabledTraits
327-
{
328-
allEnabledTraits = Set(explicitlyEnabledTraits)
329-
}
330319

331320
return try GraphLoadingNode(
332321
identity: dependency.identity,
333322
manifest: manifest,
334323
productFilter: dependency.productFilter,
335-
enabledTraits: allEnabledTraits
324+
enabledTraits: enabledTraitsSet
336325
)
337326
}
338327
}
@@ -566,10 +555,9 @@ extension Workspace {
566555
return condition.isSatisfied(by: parentEnabledTraits)
567556
}).map(\.name)
568557

569-
let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) })
570-
let enabledTraits = enabledTraitsSet?.union(self.enabledTraitsMap[dep.identity]) ?? self.enabledTraitsMap[dep.identity]
571-
572-
self.enabledTraitsMap[dep.identity] = enabledTraits
558+
if let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) }) {
559+
self.enabledTraitsMap[dep.identity] = enabledTraitsSet
560+
}
573561

574562
let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: parentEnabledTraits)
575563
return isDepUsed
@@ -612,10 +600,9 @@ extension Workspace {
612600
return condition.isSatisfied(by: parentEnabledTraits)
613601
}).map(\.name)
614602

615-
let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) })
616-
let enabledTraits = enabledTraitsSet?.union(self.enabledTraitsMap[dep.identity]) ?? self.enabledTraitsMap[dep.identity]
617-
618-
self.enabledTraitsMap[dep.identity] = enabledTraits
603+
if let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) }) {
604+
self.enabledTraitsMap[dep.identity] = enabledTraitsSet
605+
}
619606

620607
let isDepUsed = try manifest.isPackageDependencyUsed(dep, enabledTraits: parentEnabledTraits)
621608
return isDepUsed
@@ -657,27 +644,14 @@ extension Workspace {
657644
return condition.isSatisfied(by: node.item.enabledTraits)
658645
}.map(\.name)
659646

660-
var enabledTraitsSet = explicitlyEnabledTraits.flatMap { Set($0) }
661-
let precomputedTraits = self.enabledTraitsMap[dependency.identity]
662-
// Shouldn't union here if enabledTraitsMap returns "default" and we DO have explicitly enabled traits, since we're meant to flatten the default traits.
663-
if precomputedTraits == ["default"],
664-
let enabledTraitsSet {
665-
self.enabledTraitsMap[dependency.identity] = enabledTraitsSet
666-
} else {
667-
// Unify traits
668-
enabledTraitsSet?.formUnion(precomputedTraits)
669-
if let enabledTraitsSet {
670-
self.enabledTraitsMap[dependency.identity] = enabledTraitsSet
671-
}
647+
if let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) }) {
648+
let calculatedTraits = try manifest.enabledTraits(
649+
using: enabledTraitsSet,
650+
.init(node.item.manifest)
651+
)
652+
self.enabledTraitsMap[dependency.identity] = calculatedTraits
672653
}
673654

674-
let calculatedTraits = try manifest.enabledTraits(
675-
using: self.enabledTraitsMap[dependency.identity],
676-
.init(node.item.manifest)
677-
)
678-
679-
self.enabledTraitsMap[dependency.identity] = calculatedTraits
680-
681655
// we also compare the location as this function may attempt to load
682656
// dependencies that have the same identity but from a different location
683657
// which is an error case we diagnose an report about in the GraphLoading part which
@@ -688,7 +662,7 @@ extension Workspace {
688662
identity: dependency.identity,
689663
manifest: manifest,
690664
productFilter: dependency.productFilter,
691-
enabledTraits: calculatedTraits
665+
enabledTraits: self.enabledTraitsMap[dependency.identity]
692666
),
693667
key: dependency.identity
694668
) :
@@ -783,26 +757,14 @@ extension Workspace {
783757
return condition.isSatisfied(by: parentTraits)
784758
}.map(\.name)
785759

786-
var enabledTraitsSet = explicitlyEnabledTraits.flatMap { Set($0) }
787-
let precomputedTraits = self.enabledTraitsMap[dependency.identity]
788-
// Shouldn't union here if enabledTraitsMap returns "default" and we DO have explicitly enabled traits, since we're meant to flatten the default traits.
789-
if precomputedTraits == ["default"],
790-
let enabledTraitsSet {
791-
self.enabledTraitsMap[dependency.identity] = enabledTraitsSet
792-
} else {
793-
// Unify traits
794-
enabledTraitsSet?.formUnion(precomputedTraits)
795-
if let enabledTraitsSet {
796-
self.enabledTraitsMap[dependency.identity] = enabledTraitsSet
797-
}
760+
if let enabledTraitsSet = explicitlyEnabledTraits.flatMap({ Set($0) }) {
761+
let calculatedTraits = try manifest.enabledTraits(
762+
using: enabledTraitsSet,
763+
.init(parent)
764+
)
765+
self.enabledTraitsMap[dependency.identity] = calculatedTraits
798766
}
799767

800-
let calculatedTraits = try manifest.enabledTraits(
801-
using: self.enabledTraitsMap[dependency.identity],
802-
.init(parent)
803-
)
804-
805-
self.enabledTraitsMap[dependency.identity] = calculatedTraits
806768
let result = visited.insert(dependency.identity)
807769
if result.inserted {
808770
try dependencies(of: manifest, dependency.productFilter)

‎Sources/_InternalTestSupport/MockPackage.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public struct MockPackage {
3535
targets: [MockTarget],
3636
products: [MockProduct] = [],
3737
dependencies: [MockDependency] = [],
38-
traits: Set<TraitDescription> = [.init(name: "default")],
38+
traits: Set<TraitDescription> = [],
3939
versions: [String?] = [],
4040
revisionProvider: ((String) -> String)? = nil,
4141
toolsVersion: ToolsVersion? = nil

0 commit comments

Comments
 (0)