Allow command plugin dependencies to be conditional

[samdeane]

The new command plugins are great, and I like the idea that a package manifest can include all the instructions it needs for building associated tools.

However, there is a category of tools - such as code formatters - that are potentially useful for any package, but are only really needed by the package developer.

Whilst an argument could be made for these tools just being installed some other way, I do think that there are compelling reasons for them being attached to the manifest.

Unfortunately adding these tools to the package is problematic as it introduces unnecessary dependencies for clients of the package.

There are potentially multiple solutions to this problem, but one practical solution that I've currently settled on is to conditionalise the dependency based on an condition which is only true when running swift package plugin ....

Currently I'm having to do this with an environment variable that I set externally before invoking swift:

import Foundation
if ProcessInfo.processInfo.environment["RESOLVE_COMMAND_PLUGINS"] != nil {
    package.dependencies.append(contentsOf: [
        .package(url: "https://github.com/elegantchaos/ActionBuilderPlugin.git", from: "1.0.3"),
        .package(url: "https://github.com/elegantchaos/SwiftFormatterPlugin.git", from: "1.0.2")
        ])
}

It would be better if SPM managed this itself, such that I could write something along the lines of:

import Foundation
if package.isBuildingCommands {
    package.dependencies.append(contentsOf: [
        .package(url: "https://github.com/elegantchaos/ActionBuilderPlugin.git", from: "1.0.3"),
        .package(url: "https://github.com/elegantchaos/SwiftFormatterPlugin.git", from: "1.0.2")
        ])
}

Is this worth pitching? Is there another workaround already? Or something planned in a future evolution proposal?

[NeoNacho (Boris Buegling)]

I think these should automatically be optional for transitive dependencies because we only include dependencies which are required by a product or a build tool plugin which is used by a target that's included through a product in the dependency graph.

This was actually even too aggressive prior to [5.7] Mark plugin-only dependencies in required targets as required by neonichu · Pull Request #5658 · apple/swift-package-manager · GitHub

[SDGGiesbrecht (Jeremy David Giesbrecht)]

@samdeane

Unfortunately adding these tools to the package is problematic as it introduces unnecessary dependencies for clients of the package.

This is not true in the way you seem to be describing. i.e. in A → B → Command, A does not inherit Command; Command is unreachable and never resolved. (On the other hand, in A → B → Build‐Tool, A does resolve Build‐Tool. But Build‐Tool is actually needed, so it should be no surprise.)

In case the advice that reached you was tangled, the instance where commands do bleed dependencies is where A → B[Module] and at the same time B[Command] → C. In this case A resolves C for no reason, and it is (sort of) B’s fault for providing Command alongside Module. At the moment, your environment workaround is viable for this too. If you want to help find a real solution, I would start by reading from the PR where we realized the problem.

In general, if you care about cleaning up extraneous dependencies, you could help plead with the team to finish reviewing #4038 so that the rest of #3838 can be merged.

[samdeane]

@SDGGiesbrecht

This is not true in the way you seem to be describing. i.e. in A → B → Command, A does not inherit Command; Command is unreachable and never resolved.

Does A not inherit B's dependencies though? Including dependencies that are only there to facilitate building Command?

[samdeane]

@SDGGiesbrecht

In general, if you care about cleaning up extraneous dependencies, you could help plead with the team to finish reviewing #4038 so that the rest of #3838 can be merged.

Thanks, will follow up these links when I have a moment.

[SDGGiesbrecht (Jeremy David Giesbrecht)]

@samdeane

Does A not inherit B's dependencies though? Including dependencies that are only there to facilitate building Command?

If it truly is a command (not a build tool), it is by definition not a dependency of any target, just a dependency of the package. Since it is not a dependency of any target, it is not a transitive dependency of any product. Since it is not a dependency of any product, it is unreachable to any clients. Since Swift 5.2 or so, SwiftPM does not go looking for anything unless it is reachable through a product. Ergo commands plugins have no effect on the dependency graphs of clients (first‐level dependencies being a deliberate exception). I even tested it before commenting in case there were some unexpected bug that caused it to behave differently than intended; the command did not participate in resolution.

The improvement in the PR’s I mentioned is about narrowing the inheritance from just any product to the particular one that was actually requested, and ignoring any neighbouring untouched products. See SE‐0226 for more details. I mention this, because if the command happens to be coming from a package that also vends some different product that one of your targets actually uses, then you would be seeing any command‐specific dependencies included in the graph. But that is nothing particular to a command, that is just because it is an unused product in an otherwise used package. And that is precisely what those PRs cull from resolution.

[samdeane]

@SDGGiesbrecht

If it truly is a command (not a build tool), it is by definition not a dependency of any target, just a dependency of the package.

Ok, that's great, and simplifies things. It isn't the behaviour that I thought I'd observed, hence this post and my workaround. By the sound of it I made a mistake (and if it turns out that I can get a test case which reproduces what I thought I saw, I'll report it as a bug).

[samdeane]

@SDGGiesbrecht

The improvement in the PR’s I mentioned is about narrowing the inheritance from just any product to the particular one that was actually requested, and ignoring any neighbouring untouched products.

Understood.

[samdeane]

I do still find that I have a use case for this workaround.

The situation where I find it useful is simply when package A includes a dependency on package B, which defines a command plugin.

If I'm not running or listing the plugins, but just building/testing A, currently the full dependency graph seems to be evaluated, pulling in B and any dependencies that it has.

This is not a problem for clients of A, which is what I was originally worried about, but it's still a minor disincentive to adding command plugins. It will, for example, cause a CI test build to do more work and generate more network traffic than it strictly needs to.

I don't think SE-0226 will solve this, as in this example A does not define a plugin target of its own, so the dependency on B could not be moved to such a target.

I think to solve this properly, command plugin dependencies would have to be specified using a different mechanism from the normal dependencies property, and then SPM would need to be changed to only resolve those dependencies for swift package plugin invocations.

My workaround solves it in a cruder fashion, by conditionalising those dependencies, and my proposed flag would make this workaround a little cleaner and easier to adopt in the absence of a more complete fix.

[samdeane]

@samdeane

I don't think SE-0226 will solve this, as in this example A does not define a plugin target of its own, so the dependency on B could not be moved to such a target.

Actually I guess SE-0226 might solve it, if B was rewritten such that all the dependencies that it pulled in were attached to the plugin target, thus SPM could know to ignore them unless it actually needed to build the plugin. That wouldn't necessarily be a very natural thing for the author of B to do though, so I can imagine it might often not be done.

[SDGGiesbrecht (Jeremy David Giesbrecht)]

@samdeane

The situation where I find it useful is simply when package A includes a dependency on package B, which defines a command plugin.

If I'm not running or listing the plugins, but just building/testing A, currently the full dependency graph seems to be evaluated, pulling in B and any dependencies that it has.

This is not a problem for clients of A, which is what I was originally worried about, but it's still a minor disincentive to adding command plugins. It will, for example, cause a CI test build to do more work and generate more network traffic than it strictly needs to.

This is the precise scope of the remaining problem. You have now been read in well enough to fully understand where we are now.

@samdeane

Actually I guess SE-0226 might solve it

No it will not. SE‐0332, as written, defined every first level command as “unconditionally needed”, thus denying from them any of the benefits other product types get from SE‐0226. Until SE‐0332 is revised, I will continue advising everyone never to put a command plugin in the same package as any other product.

@samdeane

My workaround solves it in a cruder fashion, by conditionalising those dependencies, and my proposed flag would make this workaround a little cleaner and easier to adopt in the absence of a more complete fix.

Your workaround is more or less a variant of option 2ii from here, but with extra manual tuning required. As written, it would require passing through the evolution process to add the new API to the PackageDescription module, so I do not think it is a short cut. It also undermines Package.resolved.

Your rationale sounds like a plea for option 1 and/or 2i. That is my long term preference too, for exactly the reasons you describe. I implemented option 3 only as a temporary measure to unblock both features while any of the other solutions make their way through the review process (which would be required by any of the other solutions due to user‐facing changes to the manifest and/or pins). But I have been maintaining the PR in the queue for six months now, so my confidence is waning that any of this will ever be fixed.

[samdeane]

That sounds frustrating, I agree.

It seems to me that there is likely to be a reason why the PR has been in the queue for six months, other than it just being overlooked.

I am of course stating the obvious, but whatever the actual reason, making it explicit might be the first step towards unblocking.

Perhaps it is that someone, somewhere agrees with both of us that option 3 is ultimately not the way to go, and disagrees with the pragmatic choice of temporarily implementing it? Could we encourage that someone to say so?!

[samdeane]

@SDGGiesbrecht

Your rationale sounds like a plea for option 1 and/or 2i.

Yes, I think a hybrid of 1 and 2i, if I've understood them correctly.

I believe it would be helpful to make an explicit distinction in the manifest between command plugins that a package wants to use, and other products/targets - as they seem to serve a quite different purpose (despite the similarities in the way that they are resolved and built, and the superficial similarities between command plugins and build plugins).

I can also see the value of each command having a separate dependency graph and being pinned in a separate file.

[sidenote: this relates to another wish of mine, which is to hide Package.resolved (eg as .swiftpm/Package.resolved). I can see the point of it, and of committing it to git, but I don't like having a machine-generated file visible at the top-level. Also the naming is annoying as it auto-completes before Package.swift :)]