[Amendment] SE-0303: Package Manager Extensible Build Tools

[tomerd]

The review for an amendment to SE-0303: Package Manager Extensible Build Tools, begins now and runs through Sept 13, 2021.

The amendment goal is to address ergonomic issues in the original design of the API caused by using a top-level entry point with contextual information provided by globals. Most notably, SwiftPM plugins will now use @main instead of top-level code for entry points. Using @main allows customized entry points for each kind of plugin, makes it clearer what the inputs and expected outputs of each plugin are by replacing the global variables with method parameters and return values, and will make it easier to add new kinds of plugins in the future.

---

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager or direct message in the Swift forums).

What goes into a review of a proposal?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.

When reviewing a proposal, here are some questions to consider:

• What is your evaluation of the proposal?
• Is the problem being addressed significant enough to warrant a change to Swift?
• Does this proposal fit well with the feel and direction of Swift?
• If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Thank you for helping improve the Swift programming language and ecosystem.

Tom Doron
Review Manager

[filip-sakel]

I have some questions regarding the API:

1. What's the rationale for TargetBuildContext, ToolInfo, FileList, FileInfo and Path being protocols instead of structs? (If it’s for internal testing, I think it’s better to wrap them in a struct since client code shouldn’t be able to create conforming types.
2. Why is PluginCapability a class instead of an enum?
3. Is there any reason for Diagnostics to be a struct instead of an enum?

Other feedback:

1. I think making Diagnostics' static methods imperative would clarify the API — e.g. emitError(_:file:line:).
2. I think Path.string should be renamed to either stringRepresentation or removed entirely since Path refines CustomStringConvertible.
3. I don't think PluginUsage is a good name; perhaps the simple Plugin would work or PluginConfiguration for as a more descriptive approach.
4. Rename TargetBuildContext.pluginWorkDirectory to pluginWorkingDirectory for more clarity.

@tomerd

What is your evaluation of the proposal?

The above are minor requests; I'm definitely +1 on the amendment.

@tomerd

Is the problem being addressed significant enough to warrant a change to Swift?

I think simplicity, especially in the package manager, will help beginners and boost adoption.

@tomerd

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

—

@tomerd

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

I read the amended proposal and the pitch thread.

[Karl]

+1, I think it's an improvement.

That said, and as I mentioned in the pitch thread, I think the main function should be async.

The examples in the proposal show plugins generating files, and it's not a stretch to consider that some may require network requests (perhaps downloading a package of resources appropriate for the target). Even without dependencies on external libraries, async APIs for these things will imminently be a part of Foundation, and we should expect that developers will use them and create all manner of async helper functions and other code which they will want to use in their plugins.

Any code which uses plugins will require a Swift 5.6 toolchain anyway. On non-Apple platforms, that means developers can assume that async code is supported, and presumably swift-corelibs-foundation will have lots of async APIs ready for them to use unconditionally.

For Apple platforms, the only question is over people on older OSes, running a 5.6 toolchain which they downloaded or built themselves. I don't think those questions are significant enough to go for a non-async entrypoint. I understand that the core team are not yet ready to make promises about back-deployment, but realistically, any sort of back-deployment will at the very least require some way to execute an async function. So running an async function should be considered "likely, but not guaranteed", which I think is sufficient considering who would be affected.

I don't think we should be introducing new entrypoints which will immediately become obsolete. IMO, we should either table this proposal, or use an async function and leave compatibility with older Apple OSes an open question until we know more about back-deployment.

[abertelrud (Anders Bertelrud)]

@filip-sakel

1. What's the rationale for TargetBuildContext , ToolInfo , FileList , FileInfo and Path being protocols instead of structs? (If it’s for internal testing, I think it’s better to wrap them in a struct since client code shouldn’t be able to create conforming types.

These were represented a protocol in the original proposal so that implementations could choose the most suitable way of representing them — clients that just use the API shouldn't necessarily need to know how it was implemented.

But it's a good point that making them structs would make it clear that this isn't something that clients should implement. This proposed amendment to use @main doesn't touch this part of the original, but it may be appropriate to extend it to address this as well.

@filip-sakel

2. Why is PluginCapability a class instead of an enum?

This was modeled on some of the existing types such as Target and Product which are classes with static constructors that make them look similar to enums. But it's a good point that it should at least be a struct instead of a class.

More broadly, since it seems that enums can now (as of Swift 5.1) have default values for any parameters, these types in PackageDescription could now be enums as well.

@filip-sakel

3. Is there any reason for Diagnostics to be a struct instead of an enum?

This was to be consistent with some of the existing APis in PackageDescription, but it's a good point that this could do better as an enum, especially now that enum cases can have individual availability annotations (which wasn't true at the time PackageDescription was created).

@filip-sakel

1. I think making Diagnostics ' static methods imperative would clarify the API — e.g. emitError(_:file:line:) .

I agree, and will update this part of the suggested amendment.

@filip-sakel

2. I think Path.string should be renamed to either stringRepresentation or removed entirely since Path refines CustomStringConvertible .

The proposed amendment doesn't change this part of the proposal, but I see the point. If the amendment can be broadened then I think that suggestion make sense.

@filip-sakel

3. I don't think PluginUsage is a good name; perhaps the simple Plugin would work or PluginConfiguration for as a more descriptive approach.

It reads a little awkwardly but the idea was to capture not just a configuration or a plugin but specifically the intent to use the plugin for that target. I'm concerned that Plugin here wouldn't be specific enough, since that's also the name for defining a plugin, and to me at least, PluginConfiguration doesn't strongly enough suggest that this indicates the use of a particular plugin. But I agree that PluginUsage is a bit awkward.

@filip-sakel

4. Rename TargetBuildContext.pluginWorkDirectory to pluginWork ing Directory for more clarity.

Makes sense. On the one hand, this is another case that this proposed amendment to adopt @main doesn't touch, but on the other hand, it would be good to refine the API before it has real use.

[abertelrud (Anders Bertelrud)]

@abertelrud

More broadly, since it seems that enums can now (as of Swift 5.1) have default values for any parameters, these types in PackageDescription could now be enums as well.

I've looked into this a bit more, and it seems that one of the reasons to use a struct instead of an enum is that it's easier to extend a struct with new properties over time than it is to extend an enum. New cases can be added, with appropriate availability annotations, but new associated values cannot be added to existing cases.

[tomerd]

This review for this SE-0303 amendment has concluded and the proposal was accepted with modifications.

Thank you to everyone for the feedback and contributions to this proposal.