SE-0332: Package Manager Command Plugins

The review of SE-0332: Package Manager Command Plugins, begins now and runs through Dec 10th, 2021.

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?

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

6 Likes

Future versions of SwiftPM will almost certainly add to this set of possible intents, using availability annotations gated on the tools version to conditionally make new enum cases available.

Does this work? When was that capability added? How is it spelled?

I haven't fully reviewed, and this may have come up in the pitch, but it seems like PluginCommandIntent should just be a struct, not an enum, as you'd never really want to switch over it.

1 Like

I'm not sure of when it was added, but it's used in some places in SwiftPM's PackageDescription already, for example swift-package-manager/LanguageStandardSettings.swift at b4e94f9f78463882659917eb8565e78357f4ef17 · apple/swift-package-manager · GitHub

    @available(_PackageDescription, introduced: 5.4)
    case c17

What cannot be done however is to change the associated parameters of a case; it only works for adding new cases.

This would be a struct with static factory functions? Even in that case I think that the return value of those functions would be an enum, it's conceptually a choice among possibilities.

Hisotrically PackageDescription has used a a lot of static factory functions because it wasn't yet possible to annotate enum cases with availability annotations to individual enum cases. Even with that ability, I can see the argument for doing so here, since it allows for default parameter values and the possibility of adding new parameters in future SwiftPM versions.

Yes, structs are generally more flexible, you just usually lose exhaustiveness checking when switching over a value. If that's not how this return value is typically used, a struct would usually be preferred, and then have the two common cases provided as static values or functions.

That makes sense, and is consistent with existing PackageDescription types such as SystemPackageProvider and LibraryType. Those are actually implemented as enums, since there can be only one choice among a set, but they use static functions for creation, as you suggest.

One additional thing that has come up is that for commands that produce output, it would be helpful to allow the user to provide an output path that could be passed to the plugin and which SwiftPM would add to the sandbox to allow the plugin to write to it.

In the first example (DocC), the plugin writes the output to a directory inside its assigned work directory, which is the only place in which it's allowed to modify the file system unless it asks for write access to the package directory. The example has the plugin printing out the path of the directory so the user can access the files.

But this is inconvenient, so the suggestion is to allow an optional --output option the plugin invocation, e.g. swift package generate-docs --output ./my/output/path and to make the main entry point be performCommand(context:, targets:, arguments:, output:) with the last parameter being a Path? depending on whether the user provided a path. The output could be either a file or a directory.

2 Likes

I wanted to try the implementation before leaving a review, but it was crashing the 5.5.1 release compiler ([SR-15536] Compiler crash while trying to build SwiftPM - Swift). Do you know if that's been fixed?

Update: it has not.

Thanks for trying it out — I wasn't aware of that crash. At this point the bulk of the implementation has been merged behind a feature flag environment variable, so in a nightly toolchain setting SWIFTPM_ENABLE_COMMAND_PLUGINS=1 should let you try this out. This requires a package tools version of 999.0, since it's an experimental feature.

All the proposed APIs are in place, but the proposed commands to list plugins and to control permissions are not yet in place. The repository GitHub - abertelrud/swiftpm-command-plugin-examples: Some examples of SwiftPM command plugins to go along with SE-0332 contains the examples in this proposal.

2 Likes

Would it make sense to allow plugins to define their own arguments? output might make sense for a document generation, but what about specifying flags that get passed to a sass command for example.

Overall the proposal is a +1 from me. Specifying plugins in the package description makes sense and will make it easy to integrate in the upcoming VSCode extension.

1 Like

Definitely, and I'm thinking that some way similar to how SwiftArgumentParser allows a command to define arguments would make sense for plugins as well. SwiftPM or the IDE could then query the plugin up-front (in a manner conceptually similar to how SwiftArgumentParser supports emitting shell completions) and then use that information to allow parameters to be specified even in an IDE. I think that should be part of a separate PR, however, to keep this one bounded.

I realize that having a more generalized approach would make a specialized output argument moot (though there are some nuances, such as also having to add this path to the sandbox so that the plugin can write it it), but what I'm suggesting is to add this now and then deal with the more general problem in a follow-on PR.

1 Like

Another suggestion that has come up in separate conversation is that it would be useful for the plugin to report status. This came up during the pitch as well, but seemed like something to leave for a future proposal. This is yet another area where there is a tradeoff between doing something simple now (e.g. just having a way for the plugin to periodically report percent complete and a message) vs waiting until a much more complete future proposal (e.g. providing nested status with possible attachment files, etc). Are there any strong opinions between providing simple support now that can be replaced later vs waiting to do anything until there is a more complete future proposal?

I'm thinking about something along the lines of:

extension PackageManager {
    public func reportProgress(_ message: String, completeness: Double?)
}

where completeness is between 0.0 and 1.0 and optional (with indeterminate progress if not specified).

This seems fine - I worked on a Progress type design for Swift Concurrency a while back but we didn't get to pitch it yet, I think that type would work well with what is proposed here. That call would of course need to be thread-safe but I'm assuming that's a given :slight_smile:

Yes, it would need to be thread-safe — that's a good point. There are other nuances, such as whether or not it would allow the progress to go backwards.

I guess the question is whether it's useful enough in that simplistic form to be included in this proposal as an amendment, or whether it would be better to wait and include it in a future proposal.

1 Like

I’d be okey with this being the simplest thing possible — we’ll handle properly reporting only “forward” progress in the APIs that call this.

In a way, this is the “ui element” of somehow drawing a progress bar in the cli, and it’s fine to force people to use it correctly.

The Progress type we prototyped had facilities to handle this all :) (as does todays NSProgress)

Right, this would map fairly naturally to both the CLI and IDEs, I think.

I forgot that NSProgress is actually available in open source Foundation. That would be an alternative, except for this note:
In swift-corelibs-foundation, Key Value Observing is not yet available
which makes it tricky to know when to send progress updates to the plugin host. It seems unfortunate to do something different from what's in Foundation, but it's not clear to me how a simple API involving Progress would look in the absence of KVO to notify the plugin host of when it changes.

No, no - we should not just throw in NSProgress here into SwiftPM because we can do the right thing with a progress api that will actually work with swift concurrency (NSProgress is very painful to use there, but possible). We’ll get to it soon enough and hooking it all up does not need much APIs — the API you proposed to add will be ideal to easily integrate either progress api and I think that’s good enough :slight_smile:

1 Like

Makes sense to me. To be clear, some of the APIs assume that the plugin will use Foundation for things like launching subprocesses, but it's a good point that using Foundation types in the API taking things to another level.

Since diagnostics currently expressed through a Diagnostics type that's available to all kinds of plugins, perhaps Progress should be the same, e.g.

struct Progress {
    public static func reportProgress(_ message: String, completeness: Double?)
}

We could also keep that for a later evolution proposal once your progress type is available. That might be the best since this proposal already has a lot in it.

1 Like

As the review period is winding down and the feedback seems generally positive, I have started a PR to adjust the proposal based on the review feedback so far: WIP: SE-0332 proposal adjustments based on review feedback by abertelrud · Pull Request #1496 · apple/swift-evolution · GitHub.

2 Likes
Terms of Service

Privacy Policy

Cookie Policy