Package Manager Conditional Plugin#10119
Conversation
| 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 🙏
There was a problem hiding this comment.
then fully supporting targetPlatforms seems to require deferring plugin invocation into SwiftBuild’s configured-target context.
yeah, I think this will be required in some form to properly support destination-platform conditions. It'll likely require new API for inspecting the build graph post-dependency resolution and injecting new CustomTasks before we go to construct the task graph. I'm not entirely sure what that should look like at the moment.
There was a problem hiding this comment.
Please review 1ee5075 and companion change to SwiftBuild to add platformFilters to CustomTasks
There was a problem hiding this comment.
I think the issue with using platform filters for this is that the plugin script still runs even if the resulting tasks are filtered out, which might be a surprise to plugin authors. Ideally I think we'd fully interleave build planning and plugin execution to avoid that, although maybe there's a way to make incremental progress towards that without immediately needing to refactor everything
There was a problem hiding this comment.
The plugins are supposed to be an extension of the package manifest. It makes sense the plugins run every time (well every time the script changes).
I'd prefer we add the platform conditions to the Commands the plugin returns an extension to the Plugin API, not on the application of the plugin itself. And this is where the platform filter on the CustomTasks created for the Commands would come into play.
There was a problem hiding this comment.
Not sure what the exact process is, but this feels like a good segue to kick off the review discussion - the remaining points seem more like implementation details that would benefit from broader input.
I don't have strong opinion on the approach as either would met the proposal intent from plugin consumer's perspective. Haven't wrote any plugin myself to understand the downside of invoking plugin "unnecessarily." The filter approach seems like a good compromise to avoid a big refactor, and is somewhat similar to SE-0273 where unused dependencies are still resolved.
There was a problem hiding this comment.
Build tool plugins are supposed to be super fast. All they do is build up a list of Commands.
My point is that since the Commands turn into CustomTasks, it's the commands that should have platform filters, not the plugins. That would allow you to add different commands for different platforms which I find would be more useful.
Companion change: swiftlang/swift-build#1439 # Conflicts: # Sources/SwiftBuildSupport/PIFBuilder.swift # Tests/SwiftBuildSupportTests/PIFBuilderTests.swift
1ee5075 to
7ab6e1b
Compare
6 participants
Implements swiftlang/swift-evolution#3285