Package Manager Conditional Plugin#10119
Conversation
clive819
requested review from
bkhouri,
bripeticca,
cmcgee1024,
daveinglis,
daveyc123,
dschaefer2,
jakepetroules,
owenv,
plemarquand and
rconnell9
as code owners
| throw InternalError("could not determine package for module \(self)") | ||
| } | ||
|
|
||
| let enabledTraits = package.enabledTraits ?? [] |
There was a problem hiding this comment.
@bripeticca is this correct? Not sure where the latest refactorings left us.
There was a problem hiding this comment.
I believe we'd want to default to defaults here, since [] wrt traits API usage usually refers to the disabling of traits.
Also, there is an EnabledTrait model that is meant to be leveraged to make enabled trait checks easier. Might be good to continue the usage for that here; there is a map (EnabledTraitsMap) that is populated on the Workspace and passed through to the ModulesGraph.load(...) method; we could either add this as a property to the ModulesGraph itself since it's consumed by the BuildPlan, or it could just be passed as a parameter to the BuildPlan init. Note that model may or may not need some updates in order to handle plugins.
| public let traits: Set<String>? | ||
|
|
||
| public init(hostPlatforms: [Platform] = [], targetPlatforms: [Platform] = [], traits: Set<String>? = nil) { |
There was a problem hiding this comment.
I think traits should not be optional to align with the other APIs and default to [.defaults]
|
|
||
| for plugin in module.pluginDependencies(satisfying: buildParameters.buildEnvironment) { | ||
| let enabledTraits = package.enabledTraits ?? [] | ||
| for plugin in module.pluginDependencies( |
There was a problem hiding this comment.
Using the host build parameters here should be ok as the host is fixed, but using target build parameters here will not necessarily be correct if e.g. an iOS app embeds a watchOS app which links a package target. In that case the top level "target" would be iOS, but we'd want to evaluate platform conditions in the context of watchOS. Target platform plugin conditions may need to be represented in the PIF itself to model that correctly
There was a problem hiding this comment.
Thanks, that makes sense. This part is a bit tricky, took me a while. I’m not very familiar with the build system, so please correct me if I’m wrong.
Based on my understanding, SwiftPM currently invokes build tool plugins while generating the PIF, using the top-level target build parameters. The actual configured-target context is only available later in SwiftBuild. If we want to follow the proposal semantics, where non-matching plugins are not invoked, then fully supporting targetPlatforms seems to require deferring plugin invocation into SwiftBuild’s configured-target context.
I’m not sure whether that is the right direction, since it would mean SwiftBuild no longer merely consumes plugin results from SwiftPM, but starts participating in SwiftPM plugin planning.
Looking for guidance on what we should do here in a way that aligns with the vision for SwiftPM and SwiftBuild 🙏
5 participants
Implements swiftlang/swift-evolution#3285