[WIP] Move traits into resolution phase #8205
Conversation
Currently, target dependencies that are guarded by traits are being calculated in the modules graph loading phase - there is a desire to move this functionality into the resolution phase of SwiftPM to avoid doing redundant calculations on dependencies that will not be used further down the pipeline. When a Workspace calls computeDependencies, there is a new set of package identities being considered in the result that represent the set of unused dependencies that are described in the root package's set of dependencies - this assures that we can track which manifests will be unnecessary to load, and also includes dependencies that are guarded by traits that aren't enabled. TODO: * fully implement trait configurations passed along to manifest * testing of the graph loading node correctness; see if dependencies that need to be omitted are in fact omitted by graph loading time * as above, assure that unused dependencies aren't being downloaded as well * refactor/replacement of extensions to appropriate files * clean up comments/prints * assure all paths to resolution consider traits
- trait confiugration is propagated to where its needed in most places, which ultimately affects how each package will compute its enabled traits + used deps - fixed modules graph node successors calculation for nodes that were initially omitted if they were trait-guarded, causing errors later on related to unfound package dependencies - print cleanups
| @@ -560,3 +563,171 @@ extension Manifest: Encodable { | |||
| try container.encode(self.packageKind, forKey: .packageKind) | |||
| } | |||
| } | |||
|
|
|||
| /// Helper methods that enable data collection through traits configurations in manifests. | |||
| extension Manifest { | |||
There was a problem hiding this comment.
suggestion (possibly-blocking): In order to ensure this extension behaves as intended, can we write small tests that verify the behaviour of each [public] method in isolation, taking into account fault injections (whenever possible), boundary conditions, collections (empty, single item, two items, multiple items), etc...
If we are unable to control the test the way we want, we may need to update the production code to make it support testability.
| @@ -43,6 +43,27 @@ public struct TargetDescription: Hashable, Encodable, Sendable { | |||
| } | |||
| } | |||
|
|
|||
| public var name: String { | |||
There was a problem hiding this comment.
suggestion (possibly-blocking): Same comment here about creating small tests to verify the function/attributes in isolation.
| @@ -135,6 +135,11 @@ public struct FileSystemPackageContainer: PackageContainer { | |||
| public func getDependencies(at revision: String, productFilter: ProductFilter) throws -> [PackageContainerConstraint] { | |||
| fatalError("This should never be called") | |||
| } | |||
|
|
|||
| public func getEnabledTraits(traitConfiguration: TraitConfiguration?) async throws -> Set<String> { | |||
There was a problem hiding this comment.
suggestion (possibly-blocking): Can we add tests to verify the expected behaviour of this function in isolation?
| self.init(enabledTraits: traitConfiguration.enabledTraits, enableAllTraits: traitConfiguration.enableAllTraits) | ||
| } | ||
|
|
||
| public init?(_ traitConfiguration: TraitConfiguration?) { |
There was a problem hiding this comment.
thought (non-blocking): There is a Null Object Design Pattern that can be used instead of having to deal with nil objects. Essentially, the nil case is encoded in the Null Object. This way, the software is implemented as though it has an object.
Consider creating a NullTraitConfiguration and sending this object to the init instead of having another initializer.
* modify traits helper methods on manifest to consider cases where packages don't support traits/don't have traits
* assure that manifests without trait configurations are considered; to add tests * clean up comments + prints
Currently, target dependencies that are guarded by traits are being calculated in the modules graph loading phase - there is a desire to move this functionality into the resolution phase of SwiftPM to avoid doing redundant calculations on dependencies that will not be used further down the pipeline.
When a Workspace calls computeDependencies, there is a new set of package identities being considered in the result that represent the set of unused dependencies that are described in the root package's set of dependencies - this assures that we can track which manifests will be unnecessary to load, and also includes dependencies that are guarded by traits that aren't enabled.
TODO:
[One line description of your change]
Motivation:
[Explain here the context, and why you're making that change. What is the problem you're trying to solve.]
Modifications:
[Describe the modifications you've done.]
Result:
[After your change, what will change.]