Allow passing extra arguments to plugin compilation #5966

drodriguez · 2022-12-10T02:10:57Z

Allow customizing the compilation of plugins by passing -Xbuild-tools-swiftc parameters to the swift build invocation, the same that customizing the compilation of the manifest was possible with -Xmanifest parameters.

Motivation:

For most cases, compiling against the host toolchain does not need extra arguments, but in some complicated setups, the host toolchain might need extra arguments for search paths and such. In the case of the manifest, the option of using -Xmanifest arguments to customize the manifest compilation has existed for a while, but in the case of plugins, the same option was not available and made consuming packages with plugins very complicated.

Modifications:

The changes in this commit modify the former -Xmanifest to understand an alias -Xbuild-tools-swiftc and apply the provided arguments to both the manifest and the plugin compilation, allowing complicated setups to provide their values and compile and use plugins.

Includes modifications in one of the plugin tests to show that the -Xbuild-tools-swiftc parameters are passed to the plugin compilation (I could not find a similar test for -Xmanifest).

Result:

Setups that need to use a complicated environment can compile plugins by passing -Xbuild-tools-swiftc arguments to the swift build invocation and the plugin compilation will use such arguments.

neonichu · 2022-12-14T17:32:35Z

@swift-ci please smoke test

abertelrud · 2022-12-14T18:36:37Z

Thanks for the contribution! So -Xhost will apply to both manifest compilation and plugin compilation (and potentially any future host compilation as well)? It would probably be worth noting this in the CHANGELOG.md document as part of the PR.

drodriguez · 2022-12-19T13:14:02Z

Added a note in the changelog about the new -Xhost and its relation to -Xmanifest. I will appreciate feedback about the language or the message.

I checked the failing checks, and they did not seem to be related to these changes.

neonichu · 2022-12-19T18:13:48Z

@swift-ci please smoke test

drodriguez · 2022-12-20T11:08:39Z

Fixed the conflicts with the main branch.
Removed a test that was not finish because it was not working.
Removed whitespace changes that were uninteresting.

neonichu · 2022-12-20T17:20:53Z

@swift-ci please smoke test

CHANGELOG.md

neonichu · 2022-12-21T18:20:58Z

Sources/CoreCommands/Options.swift

            parsing: .unconditionalSingleValue,
-            help: ArgumentHelp("Pass flag to the manifest build invocation",
+            help: ArgumentHelp("Pass flag to build invocation targetting the host (manifest and plugins)",


I'm not sure I like the idea of -Xmanifest being an alias here, that feels unexpected, especially if we are doing it in perpetuity. I think these should be separate with -Xhost affecting everything and -Xmanifest to continue to only affect manifests specifically and emitting a warning about it being deprecated. Then we can eventually remove -Xmanifest and the support for separate manifest flags.

That seems like a good idea. And if you specify both, then -Xmanifest wins, I'd assume.

+1 on @neonichu idea.

also, how about -Xhost-swiftc? -Xhost seems a bit too generic, and this only apply to swiftc and this way we can also add -Xhost-clang etc

This is a good point, but I think -Xswiftc-host might be a better spelling?

I agree with @neonichu. Being able to, in essence, apply -host as a suffix to any of the tools makes a lot of sense — each of them could really be used for the host in addition to the destination, so it's really another axis. And I think a suffix probably makes things more readable than a prefix (it's a nitpick of course), but it flows better).

This PR doesn't have to be as generic as that, but picking a spelling that will scale naturally to that later makes sense.

I'm not sure I like the idea of -Xmanifest being an alias here, that feels unexpected, especially if we are doing it in perpetuity. I think these should be separate with -Xhost affecting everything and -Xmanifest to continue to only affect manifests specifically and emitting a warning about it being deprecated. Then we can eventually remove -Xmanifest and the support for separate manifest flags.

I considered this approach but left it aside because I think as currently implemented will automatically work for people that used to use -Xmanifest and found out about plugins not getting the same flags applied to (in my mind -Xmanifest was always "flags that my host needs to compile, not my target". I was also hoping to have an easier upgrade path for our scripts (which are used with stable versions of SwiftPM and the main version of SwiftPM) until all of the versions supported -Xhost and then switch. I will see what it takes to do as you ask, but I wanted to see a different PoV.

That seems like a good idea. And if you specify both, then -Xmanifest wins, I'd assume.

I imagine that you mean if any -Xmanifest is provided, only the manifest compilation will not use any of the -Xhost arguments? Not that -Xmanifest will be used even for plugins, right? I don't know if someone will need that behaviour explicitly, and I am a little bit afraid of implementing it, since someone will end up relying on it. I will give it a shot if this allow this to be merged (as I understand it as explained at the start of this paragraph).

+1 on @neonichu idea.

also, how about -Xhost-swiftc? -Xhost seems a bit too generic, and this only apply to swiftc and this way we can also add -Xhost-clang etc

This is a good point, but I think -Xswiftc-host might be a better spelling?

I think there's no -Xmanifest-swiftc and -Xmanifest-clang because -Xmanifest can be composed with other -Xfoo accepted by swiftc. We use this to correctly pass the right arguments to each piece of the toolchain (example: -Xmanifest -Xcc -Xmanifest -nostdlibinc -Xmanifest -Xclang-linker -Xmanifest --sysroot=/var/empty). This might be because the manifest only invoked swiftc, and plugins only seem to do so as well. Is there any case you all think that products that need to be compiled for the host will need to invoke something else but swiftc? I will keep this suggestion out of the rework. If you really think this change is important, I will abide, but I really think it makes things more complicated than needed.

That say, if I have some vote in the spelling, I think -Xhost-swiftc is slightly better (having host first indicates earlier that this is not a normal -Xswiftc.

great points @drodriguez, to add a few thoughts:

manifests are by definition swift only, but we should consider if plugins could include other languages in the future. if they do we should design the flags to support different compilers.

I agree with your argument that -Xhost-swiftc is better.

@neonichu @abertelrud, thoughts?

Is there any case you all think that products that need to be compiled for the host will need to invoke something else but swiftc?

Yes, this was the motivation for suggesting to broaden the discussion here. There are flags for other tools (-Xcc, -Xlinker, etc) and for each of those it's quite possible that the flags will need to be different for the host vs the destination during a regular build (as opposed to compilation of a manifest or plugin, which today is handled outside the build system).

I didn't realize that the compiler driver allows these flags to be composed, however. It's important to support Clang as well, but if it also supports composition, then doing -Xmanifest -Xlinker -Xmanifest -lfoo (for example) seems like a way around that.

I still think that the name -Xhost is too generic in this case, if it only applies to compilation of manifests and plugins, since I'd expect that with that name it would also apply to products built for the host.

I agree with your argument that -Xhost-swiftc is better.

This order seems fine.

I still think that the name -Xhost is too generic in this case, if it only applies to compilation of manifests and plugins, since I'd expect that with that name it would also apply to products built for the host.

I used the name because it is the name of the "destination" used for the manifest. I am fine changing it if someone can find a good common name that applies to both manifest and plugins.

If we follow compnerd recommendation (#5966 (comment)) and want to follow the GCC naming (https://gcc.gnu.org/onlinedocs/gccint/Configure-Terms.html), one possibility would be -Xbuild, but that might be confusing with internal terminology (and the fact that the concepts of "target" and "host" are a little bit dilutted by LLVM architecture).

Finally, if we want to add this as -X<whatever>-swiftc in this PR I am fine doing that change. -Xmanifest will be left as a deprecated detail that did not support several tools.

drodriguez · 2022-12-23T10:46:45Z

/cc @compnerd I forgot to look for the original author of -Xmanifest. I hope the changes does not affect you, or even help you. You might have some opinion on the discussion above about the spelling and how to deal with -Xmanifest.

compnerd · 2022-12-25T17:39:24Z

I think that -Xhost is a poor choice of an option. "host" means the host where the artifacts will execute in cross-compilation scenarios. The spelling to use is -Xbuild as the build is what the build runs on. However, that might not be particularly obvious as the SPM build is not well setup for cross-compilation (destination.json is under documented). Perhaps -Xplugin is the right balance here - and matches -Xmanifest as well.

drodriguez · 2022-12-27T08:11:12Z

I think that -Xhost is a poor choice of an option. "host" means the host where the artifacts will execute in cross-compilation scenarios. The spelling to use is -Xbuild as the build is what the build runs on. However, that might not be particularly obvious as the SPM build is not well setup for cross-compilation (destination.json is under documented). Perhaps -Xplugin is the right balance here - and matches -Xmanifest as well.

I used "host" because that's the internal name of the toolchain used to compile the manifest and the plugins: https://github.com/apple/swift-package-manager/blob/14d05ccaa13b768449cd405fff81d630a520e04a/Sources/PackageModel/Destination.swift#L159

I know it is not the right terminology, but it is the one that fits with the current model, which I think is more important. It is also a hidden option, so it is not like we are exposing bad terminology to most users, only those who need it.

I did not want to add -Xplugin because, in most case, one will need the same set of values for both -Xmanifest and -Xplugin. That was why I was trying to choose a neutral term.

If somebody changes their minds to these new terms, the spelling is relatively easy to change, but I think a common -X… fits better with my mental model (I cannot see the reason for compiling the manifest with one toolchain, and the plugins with another).

drodriguez · 2022-12-27T09:45:58Z

Split -Xmanifest and -Xhost. They are not aliases anymore.
Apply -Xhost to plugin compilation. Only apply -Xhost to manifest compilation if no explicit -Xmanifest is provided.
If -Xmanifest is provided, show a deprecation message.

tomerd · 2023-01-03T16:53:43Z

Sources/CoreCommands/SwiftTool.swift

@@ -335,6 +335,10 @@ public final class SwiftTool {
        if let _ = options.security.netrcFilePath, options.security.netrc == false {
            observabilityScope.emit(.mutuallyExclusiveArgumentsError(arguments: ["--disable-netrc", "--netrc-file"]))
        }
+
+        if !options.build._deprecated_manifestFlags.isEmpty {
+            observabilityScope.emit(warning: "'-Xmanifest' option is deprecated; use '-Xhost' instead")


@natecook1000 does argument parser support deprecated arguments? I can see this being super useful in SwiftPM use case, where one can mark a flag as deprecate and provide a deprecation message

We don't have built-in support for that — could you open an issue? I could see it added to the help for an argument.

tomerd · 2023-01-03T16:54:14Z

Sources/CoreCommands/Options.swift

+                shouldDisplay: false))
+    public var _deprecated_manifestFlags: [String] = []
+
+    var manifestFlags: [String] {


CHANGELOG.md

drodriguez · 2023-01-11T20:30:27Z

Renamed -Xhost to -Xhost-swiftc. I do not know if this is the preferred spelling, but this PR seem to have been blocked because -Xhost was not adequate, so let's see if that spelling fares better.
Moved the block in the changelog from 5.8 to a new "v.Next" section (I saw that in the history, I hope that's correct). Added a reference to this PR URL.
Checked the FunctionalTests locally (they are still commented out), and it still passes.

If someone can have a look at the new version and fire up the testing, I will appreciate it.

Edit: also changed from shouldDisplay: false to visibility: .hidden.

neonichu · 2023-01-17T09:40:22Z

@swift-ci please smoke test

drodriguez · 2023-01-17T18:52:37Z

Rebase on main.
Remove conflict in CHANGELOG.md.

drodriguez · 2023-01-30T17:50:32Z

Is there something else I need to do for this to be considered for merging? Did I miss some of the feedback?

drodriguez · 2023-02-01T00:34:47Z

Format with SwiftFormat.

drodriguez · 2023-02-01T17:25:08Z

Added deprecation note in help string.

MaxDesiatov · 2023-02-01T17:46:27Z

@swift-ci smoke test

MaxDesiatov

LGTM, thanks! 👍

drodriguez · 2023-02-13T17:58:26Z

Rebased on top of main to remove the conflicts.

Can this be tested and merged in its current state? Do the maintainers require any more changes? I don't know if we are happy with the current spelling or not, but I have not seen any other proposals, or any proposal that everybody is happy with. Since this is a hidden option for a very special use case, can we not merge the current spelling and if anyone came up with a better name use the new name then?

tomerd · 2023-02-13T18:21:18Z

personally I am happy with this and would very much like to see this over the finish line.

IIUC @neonichu @abertelrud @MaxDesiatov may still have some concerns about naming. also adding @compnerd since he may have opinion / suggestions for a name.

@neonichu to your point about "Extensible Build Tools" proposal, I agree and think we should retitle it to "Extensible Tools" or "SwiftPM Plugins", happy to make that PR if @abertelrud et all agree.

In any case, let's try to get this merged. If by the end of this week we cannot agree on a name, I can help decide on one.

drodriguez · 2023-03-07T18:19:35Z

With the announcement of Swift 5.9 release process, I would like to give another gentle ping to this PR to see if it can move forward and be merged before the cut. I am also happy to try to implement changes for any feedback that can still appear.

neonichu · 2023-03-10T00:51:51Z

I'm not super happy with the name, but let's move forward with this anyway.

neonichu · 2023-03-10T00:52:01Z

@swift-ci please smoke test

For most case, compiling against the host toolchain does not need extra arguments, but in some complicated setups, the host toolchain might need extra arguments for search paths and such. In the case of the manifest, the option of using `-Xmanifest` arguments to customize the manifest compilation has existed for a while, but in the case of plugins, the same option was not available and made consuming packages with plugins very complicated. The changes in this commit add `-Xbuild-tools-swiftc` to affect both manifest and plugin compilation, allowing complicated setups to provide their values and compile and use plugins. `-Xmanifest` is kept for backwards compatibility, but it will only affect manifest compilation, as before, and it will print a warning if used. Includes modifications in one of the plugin tests to show that the `-Xbuild-tools-swiftc` parameters are passed to the plugin compilation (I could not find a similar test for `-Xmanifest`).

drodriguez · 2023-03-10T23:41:22Z

Rebased on main to avoid the conflict in CHANGELOG.md.

I think the failed check on macOS for previous commit was not caused by these changes:

+ swift package update
/Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm: error: package at '/Users/ec2-user/jenkins/workspace/swift-package-manager-with-xcode-self-hosted-PR-osx/branch-main/swiftpm' is using Swift tools version 5.7.0 but the installed version is 5.5.0

MaxDesiatov · 2023-03-10T23:46:07Z

@swift-ci smoke test

tomerd · 2023-03-11T00:06:30Z

the Swift Test macOS Platform failure (wrong swift version) is safe to ignore, the infra will be upgrade soon to address this. cc @shahmishal

tomerd · 2023-03-11T00:12:27Z

Sources/CoreCommands/Options.swift

+            self._deprecated_manifestFlags
+    }
+
+    var pluginSwiftCFlags: [String] {


not blocking, but wasnt the idea to also expose this is a user facing flag? or is that for next PR so we can debbate the name? :D

Do you mean supporting both -Xbuild-tools-swiftc which applies to every build time tool and a -Xplugin-swiftc which only applies to plugins? I never got that impression from the feedback, but I might have missed that facet. I am not sure that I can see an scenario in which someone has different sets of flags for the same native machine depending if they are compiling a plugin or some other build time tool. The solution as implemented can grow in that direction, if needed.

could have been my misunderstanding of he goal of this PR. in any case as you say this sets the ground work if needed, and this feedback is by no means to hold the PR back 👍

tomerd · 2023-03-11T04:07:02Z

@neonichu good to merge?

drodriguez · 2023-03-20T17:50:56Z

I wanted to give another gentle ping towards the end of last week, but I forgot. Is there something I need or can do to get this merged before the release 5.9 is branch off?

Thank you very much for all the feedback and the reviews.

MaxDesiatov · 2023-03-20T17:54:05Z

@swift-ci test windows

tomerd · 2023-03-20T20:31:55Z

thanks for your patience @drodriguez

drodriguez requested review from abertelrud, neonichu, tomerd and elsh as code owners December 10, 2022 02:10

drodriguez force-pushed the xhost-parameter branch from eceecda to 4b43fcf Compare December 19, 2022 13:12

drodriguez force-pushed the xhost-parameter branch from 4b43fcf to e013a62 Compare December 20, 2022 11:07

tomerd assigned neonichu Dec 21, 2022

neonichu reviewed Dec 21, 2022

View reviewed changes

MaxDesiatov changed the title ~~Allow passing extra arguments to plugin compilation.~~ Allow passing extra arguments to plugin compilation Dec 22, 2022

drodriguez force-pushed the xhost-parameter branch from e013a62 to 4781bd0 Compare December 27, 2022 09:43

tomerd reviewed Jan 3, 2023

View reviewed changes

MaxDesiatov reviewed Jan 10, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

drodriguez force-pushed the xhost-parameter branch from 4781bd0 to 7df5a4e Compare January 11, 2023 20:27

drodriguez force-pushed the xhost-parameter branch from 7df5a4e to 21e1595 Compare January 11, 2023 20:33

drodriguez force-pushed the xhost-parameter branch from 21e1595 to 2b1a27d Compare January 17, 2023 18:50

drodriguez force-pushed the xhost-parameter branch from 7d67288 to b75d814 Compare February 1, 2023 00:34

drodriguez force-pushed the xhost-parameter branch from b75d814 to 714b2b7 Compare February 1, 2023 17:24

MaxDesiatov approved these changes Feb 1, 2023

View reviewed changes

MaxDesiatov requested review from neonichu and tomerd February 7, 2023 18:27

drodriguez force-pushed the xhost-parameter branch from 714b2b7 to 4d911fa Compare February 13, 2023 17:54

drodriguez force-pushed the xhost-parameter branch from 4d911fa to f53f876 Compare March 10, 2023 00:43

drodriguez force-pushed the xhost-parameter branch from f53f876 to ba2310c Compare March 10, 2023 23:40

tomerd approved these changes Mar 11, 2023

View reviewed changes

tomerd reviewed Mar 11, 2023

View reviewed changes

neonichu approved these changes Mar 20, 2023

View reviewed changes

neonichu enabled auto-merge (squash) March 20, 2023 18:48

neonichu merged commit 111093f into swiftlang:main Mar 20, 2023

drodriguez deleted the xhost-parameter branch March 20, 2023 19:52

Allow passing extra arguments to plugin compilation #5966

Allow passing extra arguments to plugin compilation #5966

Conversation

drodriguez commented Dec 10, 2022 • edited Loading

Motivation:

Modifications:

Result:

neonichu commented Dec 14, 2022

abertelrud commented Dec 14, 2022

drodriguez commented Dec 19, 2022

neonichu commented Dec 19, 2022

drodriguez commented Dec 20, 2022

neonichu commented Dec 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abertelrud Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drodriguez commented Dec 23, 2022

compnerd commented Dec 25, 2022

drodriguez commented Dec 27, 2022

drodriguez commented Dec 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drodriguez commented Jan 11, 2023 • edited Loading

neonichu commented Jan 17, 2023

drodriguez commented Jan 17, 2023

drodriguez commented Jan 30, 2023

drodriguez commented Feb 1, 2023

drodriguez commented Feb 1, 2023

MaxDesiatov commented Feb 1, 2023

MaxDesiatov left a comment

Choose a reason for hiding this comment

drodriguez commented Feb 13, 2023

tomerd commented Feb 13, 2023

drodriguez commented Mar 7, 2023

neonichu commented Mar 10, 2023

neonichu commented Mar 10, 2023

drodriguez commented Mar 10, 2023

MaxDesiatov commented Mar 10, 2023

tomerd commented Mar 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

tomerd commented Mar 11, 2023

drodriguez commented Mar 20, 2023

MaxDesiatov commented Mar 20, 2023

tomerd commented Mar 20, 2023

drodriguez commented Dec 10, 2022 •

edited

Loading

abertelrud Dec 22, 2022 •

edited

Loading

drodriguez commented Jan 11, 2023 •

edited

Loading

tomerd Mar 11, 2023 •

edited

Loading