I'd like to propose amending SE-0303 so that SwiftPM plugins use @main instead of top-level code for entry points. While it's a little more verbose, this would allow customized entry points for each kind of plugin, and would also make it clearer what the inputs and expected outputs of each plugin are.
I would love to hear any thoughts and opinions on this approach.
That's a good idea. Since that would require a new proposal to allow plugin scripts to depend on libraries, I'm wondering if it would be better to add async versions of the APIs at that time, though? The switch to using a protocol and @main should make it easier to introduce new versions of APIs (tied to the tools version).
Looks like an improvement! Specific comments follow.
The Diagnostics type
Other than it being in a global, I kind of prefer the old DiagnosticsEmitter design with a protocol containing instance methods, rather than an empty concrete type containing static methods. Because it was just a protocol, you could have written a mock DiagnosticsEmitter for testing; that capability seems to have been lost here. Is the new design really an improvement over tucking a DiagnosticsEmitter (possibly renamed) into a PluginContext property?
On the other hand, if you do keep it as a set of static methods grouped together in a type, shouldn't that type be an uninhabited enum instead of a struct so that the compiler can correctly reason about the fact that there aren't any instances of it?
The BuildToolPlugin protocol
If run(context:target:) is going to be an instance method, you will need an initializer in your protocol so that its main() can create the instance.
But as long as you're adding an initializer, I might think about defining the protocol with context and perhaps target as properties so that they don't have to be passed around quite as much between methods of the plugin instance. In either case, the memberwise initializer should normally save users from having to write an initializer by hand.
Is run only executed once per instance, or could it be executed several times on a single instance, perhaps with a different target each time? This was not really an open question with main.swift, but since it's now a method, users might wonder what the lifecycle is.
The name run seems unnecessarily nondescript—why not name it something like makeBuildCommands that describes what the method needs to accomplish? And once you've done that, would it make sense to split prebuild commands into a separate makePrebuildCommands method? This would eliminate mixing of prebuild and build commands.
Renaming run would open the possibility that, once there are additional plugin capabilities in the future, a single plugin could implement several different capabilities by implementing several different methods. But this would have knock-on effects on the rest of the design—to give some obvious examples, BuildToolPlugin might be better off with a different name and Target.plugin should probably take an array of PluginCapability instances instead of just one (or it should assume all plugins have all capabilities and plugins will just use default no-op implementations of the methods for capabilities they don't want to provide). Does that sound like a good idea to you? If so, what else needs to change?
You say CommandCreator at one point where I believe you mean CommandConstructor.
I'd like to see a full listing of the PluginContext type somewhere in the proposal body; you list a bunch of declarations that have been removed and show one that was added, but I had to refer back to the original and do some mental gymnastics to get a good picture of this type.
That's a good point. Unfortunately I think it would mean that plugins would also be usable only on macOS 12 or later (when I try to use async I get an error that I need a minimum deployment target of 12).
Thanks for clarifying. Since plugins require a main toolchain (which will eventually be 5.6?) the compilation part won't be a problem — the main concern was about being able to run plugin scripts on older OSes. I will check up on back-deployment. Thanks!
That's a very good point. Adding a factory type to the context as you suggest would make sense and allow the global to be removed while still allowing those methods to be instance methods. I'll refine the pitch proposal.
The intent with the proposed Diagnostics change in particular was to simplify the verbiage a bit. But your point about the static methods points out a limitation I hadn't considered.
That's a good point, and that was an oversight. In my view your suggestion of a context property is probably better, though.
This is something I considered, but since we know that we want to allow plugin configuration in the future, it seemed to me to make sense to reserve those properties for declared parameters of the plugin.
In short it seemed clearer to say that the function parameters are the inputs from SwiftPM, the return values are the outputs to SwiftPM, and then (in a future version) the instance's properties are the parameters of the plugin (with values provided by the invocation). Perhaps we could then also use attributes to let the plugin declare its configurable properties, in the manner of SwiftArgumentParser.
The intent was to have a separate plugin invocation for each target, i.e. the same as described SE-0303, to prevent leaking of state from one invocation to the other in the same process. It might certainly be more effficient to invoke it repeatedly, but that seemed like a larger change than the API refinements proposed here.
For example, SE-0303 says that if a target uses multiple build tool plugins, they are applied in the order in which they are listed. If two targets list the same two plugins but in opposite order, that would mean that at least in some cases there would need to be multiple plugin invocations with one target each. That might be a good future optimization but it seems as if it can get a bit subtle.
That makes sense, though in this particular case I don't think that it's necessarily desirable to separate out the build commands from prebuild commands (ideally we wouldn't even need prebuild commands, if regular commands in the build system could generate new work that wasn't known until the command ran, but that isn't something that will be addressed soon).
But I see your broader point. It's worth considering whether more varied names would be better here, especially since it would allow multiple capabilities in a single plugin.
Meanwhile, I had actually revised the proposal to get away from the CommandConstructor approach to instead return different kinds of commands via an enum, which seems a little cleaner.
Allowing any plugin to provide any capability has a conceptual appeal (essentially getting rid of the predeclared capabilities). The reason the capability declaration was added to the manifest was so that SwiftPM will know when to invoke the plugin, without having to invoke it just to find out. But allowing multiple capabilities in the same declaration would work, without having a lot of plugin invocations to happen "just in case".
And if the plugin can be statically analyzed to determine what protocols it implements, then that might be a way to avoid having to declare specific capabilities in the first place.
Thanks for pointing out that error!
Good point. Some of the earlier feedback was to focus on only the differences, but seeing the end result would be important here. I'll add that back in.
Thanks a lot for your feedback! A lot of things to consider there...
I think what I'm going to do as far as a pitch for an amendment for SE-0303 is to pare this back to focus only on the @main part, and not on taking that opportunity to also add more package model properties "for future use". Putting that into a new pitch and proposal should make it it's easier to discuss separately.
Some API in the release form is easy to be converted, like TargetBuildContext.targetDirectory to Target.directory
However I did not find a corresponding release API to get the old TargetBuildContext.inputFiles property.
Any suggestion on this?
// Original Proposal API
func createBuildCommands(context: TargetBuildContext) throws -> [Command]
// Released API
func createBuildCommands(context: PluginContext, target: Target) async throws -> [Command]
After looking at the relavent MR in the GitHub repo, I find the example plugin and the corresponding new usage.
Maybe we should reflect/update this change in SE-0303? cc @abertelrud