Update SE-0387: Cross-Compilation Destination Bundles #1942
Conversation
|
|
||
| We find that properties dedicated to tools configuration are useful outside of the cross-compilation context. Due to | ||
| that, a separate `toolset.json` file is introduced: | ||
| that, separate toolset configuration files are introduced: |
There was a problem hiding this comment.
Now that you're removing the host variant directories, have you thought about how to have these bundles use different tools based on the host, particularly on linux? Would be good to specify that, whether you plan to have different toolsets based on the build-time triple or place them in certain directories by convention.
There was a problem hiding this comment.
I removed host variant directories to follow what the artifact bundles proposal does. It mentions them, but ended up not using them in the example code, and I'd like to follow it as closely as possible in its implementation and bundle layout examples.
There was a problem hiding this comment.
OK, I took a look at that SE-0305 proposal, that I was not familiar with. It specifically mentions listing out paths to different tools based on the build-time triple needed, which is missing from this proposal. That creates an ambiguity after this pull, where you don't know which build-time triple each of these toolsets is built for, that wasn't there with the host variant directories before, with each presumably containing tools that run on that build host.
There was a problem hiding this comment.
Yes, it does that, but then check out example code for SwiftPM command plugins that utilize artifact bundles. Host variant directories are not present there and looks like it's convention-based after all.
There was a problem hiding this comment.
Sorry, not command plugins, but extensible build tools. I'm referring to this one https://github.com/abertelrud/swiftpm-buildtool-plugin-examples/tree/main/Example%201%20-%20SwiftGen/Binaries/SwiftGenBinary.artifactbundle
There was a problem hiding this comment.
IMO that's over-engineering toolsets for what they weren't designed for (a SwiftPM's answer to CMake toolchains, and those don't support multiple triples in a single toolchain file either). If you need different tools for a different build-time or run-time triple and Universal binaries aren't available, just create a separate toolset.
and you could use those toolsets without bundles too.
I don't think there's anything that prevents using toolsets without bundles in their current state.
There was a problem hiding this comment.
If you need different tools for a different build-time or run-time triple and Universal binaries aren't available, just create a separate toolset.
As you said above, that won't be enough for bundles, a whole new destination is needed.
I don't think there's anything that prevents using toolsets without bundles in their current state.
I said "those toolsets," meaning a toolset config that works on multiple types of hosts without bundles, whereas this approach you're suggesting will require separate buildtime-specific toolset configs.
I think this is worth consolidating in the toolset spec now, as the location of the build tools for different build-time triples is a single line that shouldn't require a whole new destination that is identical except for that, but up to you.
There was a problem hiding this comment.
Ok, I think I see what you mean. Would a toolset at the level of artifacts reusable across multiple variants specified in info.json help this in case? I initially had it in one of the previous commits, but felt that it reached too many levels to keep in mind when maintaining toolsets for such destinations.
There was a problem hiding this comment.
Unsure what you're proposing, but my suggestion is to add an optional dictionary to the toolset spec, that maps each of the supportedTriples to their own subdirectory of toolsetRootPath. That way SPM knows where all the build tools are and a single destination installed on linux/Windows can support multiple build-time triples, just like on Darwin.
There was a problem hiding this comment.
Have you thought about how you want to handle this?
|
|
||
| If `sdkRootPath` is specified and `swiftResourcesPath` is not, the latter is inferred to be | ||
| `"\(sdkRootPath)/usr/lib/swift"` when linking the Swift standard library dynamically, `"swiftStaticResourcesPath"` is | ||
| inferred to be `"\(sdkRootPath)/usr/lib/swift_static"` when linking it dynamically. Similarly, `includeSearchPaths` is |
There was a problem hiding this comment.
The second time: dynamically -> statically. Also, won't these be used just to build static libraries too, so maybe it should be made more general to when linking libraries dynamically in the first instance?
There was a problem hiding this comment.
Thanks, that's addressed now.
won't these be used just to build static libraries too
I'm not aware of that. Can you point to specific place in the source code or documentation that supports this statement?
There was a problem hiding this comment.
I just built SwiftSyntax, which has several static libraries, on linux to check, and it appears to build the object files in a SPM target by looking in the dynamic resource directory for the stdlib interface files, then simply use ar to package them together. I don't know if that's a mistake, as the static resource directory has its own interface files, but what you wrote about the static stdlib initially is correct.
| If `sdkRootPath` is specified and `swiftResourcesPath` is not, the latter is inferred to be | ||
| `"\(sdkRootPath)/usr/lib/swift"` when linking the Swift standard library dynamically, `"swiftStaticResourcesPath"` is | ||
| inferred to be `"\(sdkRootPath)/usr/lib/swift_static"` when linking it dynamically. Similarly, `includeSearchPaths` is | ||
| inferred as `["\(sdkRootPath)/usr/include"]`, `librarySearchPaths` as `["\(sdkRootPath)/usr/lib"]`. |
There was a problem hiding this comment.
Isn't this changing the current behavior, ie these -I/-L flags are not added by default to the SDK path? Maybe these two include/lib paths should be kept empty by default.
There was a problem hiding this comment.
We won't populate them in SwiftPM explicitly with -I/-L when these properties are absent or empty in destination.json, but that's my understanding of what the behavior is when only -sdk option is passed to swiftc.
There was a problem hiding this comment.
that's my understanding of what the behavior is when only -sdk option is passed to swiftc.
No, it doesn't on linux, see the output from swiftlang/swift#63416. The reason is that the SDK is the C sysroot and these are Swift include/lib flags, so the compiler only looks in the Swift resource directory, not the normal non-Swift SDK locations you list here.
There was a problem hiding this comment.
I think if you update this doc to mention that nothing is done by default, should be okay.
This brings the implementation in line with changes proposed in swiftlang/swift-evolution#1942.
| Additionally, we find that preventing tools from being launched from arbitrary paths can't be technically enforced | ||
| without sandboxing, and there's no cross-platform sandboxing solution available for SwiftPM. Until such sandboxing | ||
| solution is available, we'd like to keep the existing approach where setting `PATH` environment variable behaves in a | ||
| predictable way and is consistent with established CLI conventions. |
There was a problem hiding this comment.
I completely disagree with most of this paragraph. Nobody is suggesting that the bundle be "fully self-contained," rather that tools should only be invoked from the bundle or the external Swift toolchain that's building the Swift code, ie only disabling the fallback of system PATH lookups for the build tools. Since the bundle already requires a Swift toolchain to build anything and that Swift toolchain already has the tools mentioned like clang-13 and swift-frontend, there would be no need to put such tools in a bundle.
There is no need for sandboxing whatsoever: all we'd have to do is disable the four invocations of findTool(), which looks for build tools in the system PATH, for the cases when SPM knows we're building a bundle with this destination 3.0 spec or later.
This is a trivially easy change that I believe will save some bundlers some headaches down the line, but it is a minor fit and finish issue that I'm not going to hold up this review over. I'm simply explaining this again because after all this discussion, what I was trying to say apparently hasn't got through.
For configuring destination bundles that can't be self-contained and have to reference absolute paths we need new to provide a CLI. This comes as a new `swift experimental-destination configuration` subcommand per the proposed changes in swiftlang/swift-evolution#1942. `DestinationCommand` protocol is refined to wait for the observability handler to flush all output. Existing destination-related subcommands are modified to conform to it. Additionally, it now passes a newly initialized observability scope and configuration values. Two new subcommands are added: `configuration show` and `configuration reset`. For actually updating configuration values a third subcommand `configuration set` will be added in a separate PR.
|
The dashboard and proposals.json only show Evan's first name. |
This addresses feedback provided by the community in the proposal review thread. Some sections were expanded with examples, while properties of some of the configuration files were updated. General fixes for typos, formatting, and wording were also applied.