SwiftDriver: initial work to properly handle android cross-compilation #1560
Conversation
| if let sdk = parsedOptions.getLastArgument(.sdk)?.asSingle ?? env["SDKROOT"], !sdk.isEmpty { | ||
| rsrc = try VirtualPath(path: AbsolutePath(validating: sdk) | ||
| .appending(components: "usr", "lib", "swift") | ||
| .pathString) |
There was a problem hiding this comment.
Note that if -resource-dir is not specified, this behavior is already the default, so the only reason to do this is to override -resource-dir. Any reason why you need that? I think this is a bad idea to silently override it.
There was a problem hiding this comment.
This is not the default - this is going to try to prefer the resources in the SDK rather than the toolchain (which allows us to have platform specific resources).
There was a problem hiding this comment.
Huh, never mind, what I'm saying was true of the old C++ Driver, but is not true of this new swift-driver, after manually testing it on linux.
I think this is a genuine discrepancy between the two drivers that we should find the root cause and fix, rather than putting in partial workarounds like this.
There was a problem hiding this comment.
I agree that it's best to push swiftlang/swift#72409 over the line to avoid this particular workaround for SDK-relative logic here.
|
With this set of changes I am down to: The bits that I would like to remove: This feels like we are failing to setup the library search path when we have a This is due to my builds missing clang_rt.builtins-... for Android and so we need to pull some files from the NDK. Addressing those two issues would give us the following invocation: and with |
|
The reason this entire approach doesn't already work is because of a regression in this new Swift Driver, see #1562. We should fix that for all platforms, not paper over it with these Android-specific changes. |
|
I'm not sure I'm following the "papering over" here. The idea here is to default the |
|
There are two aspects to this pull, having this new swift driver read environment variables like I don't think we should hard-code such environment variables in this driver, but always "explicitly specify" them, as you say. As for finding build files, the reason that doesn't work already is #1562: take a look at my detailed writeup from a couple weeks ago for why. |
|
I disagree with you on that point. I would like the default value behavior for the targets where possible, much as it works on macOS. Fixing the behavior to work with the parameters however should be done. |
The intent here is to permit the Windows/macOS style cross-compilation for Android. This involves passing `-sdk` with the path to the "Swift SDK" which overlays the system's native SDK (NDK). The `ANDROID_NDK_ROOT` is a well-defined environment variable (setup by the SDK installer as well as a general expectation for Android development) that identifies the root of the installation of the NDK. This allows us to locate the native SDK root (`--sysroot`) for driving the linker driver amongst other paths.
|
@artemcm, as I already said, this should not be merged, as this is largely an Android-specific hack to paper over this problem till the real fix for all platforms, swiftlang/swift#72409, which has not been worked on for the last three months. |
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
Sources/SwiftDriver/Jobs/GenericUnixToolchain+LinkerSupport.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDriver/Jobs/GenericUnixToolchain+LinkerSupport.swift
Outdated
Show resolved
Hide resolved
| if let sdk = parsedOptions.getLastArgument(.sdk)?.asSingle ?? env["SDKROOT"], !sdk.isEmpty { | ||
| rsrc = try VirtualPath(path: AbsolutePath(validating: sdk) | ||
| .appending(components: "usr", "lib", "swift") | ||
| .pathString) |
There was a problem hiding this comment.
I agree that it's best to push swiftlang/swift#72409 over the line to avoid this particular workaround for SDK-relative logic here.
|
@compnerd, I think you've found a real bug in this repo with locating the Swift SDK overlay modules and headers in an SDK, which his swiftlang/swift#72409 fixes more generally. Can you try reverting this pull in your local build, applying that frontend patch instead, and see if it fixes most of these issues? If it doesn't, you can pare this down to fix what's left, as that frontend fix will be needed for all platforms and should fix most of your issues here. |
|
@artemcm I kept the 'swiftrt.o' adjustment based on the SDK path in this patch. swiftlang/swift#72409 does not work for Android specifically as I mentioned in that PR, as with that change Swift is unable to find the clang builtin headers next to the toolchain, which are separate from the Android SDK. |
|
@swift-ci please test |
|
@artemcm do you think this needs further changes? |
|
At a high-level, I think that this is the right direction. But I would also like to ask for something of a vision document listing out what each of the files in the current Linux toolchain/SDK are tied to so that when they move, it doesn't come as a surprise and there is something we can discuss and point to instead of a smattering of compiler changes. When it comes down to it, the toolchain files should be pretty easy to move. Move the file and update the compiler at the same time, and since they're tied to that specific compiler, if anyone complains about a missing file, it means they were living life on the edge and were getting very lucky regarding ABI. SDK content is maybe a bit weirder, but given that, for now anyway, the Linux SDKs are sort of tied to a specific compiler build through ABI instability and rev-locked swiftmodule files, maybe it's okay? Apple platforms and Windows both need to support the split builds due to how the SDKs and toolchains are split in Xcode and how libraries work on Windows, which is why they both look remarkably similar in terms of the layout. swiftrt.o(bj) is only relevant to Windows because the dynamic loader on Darwin knows how to unpack the runtime metadata from binaries automatically so it isn't necessary. Linux is the odd one where you can build against the system you're running on, and unfortunately that allowed for a disorganized Linux build. As you noted, @finagolfin, the Linux installs have the compiler running on a different Swift runtime than the one it was built against. We've gotten very lucky that things have played out as well as they have. Untangling it will require moving files to the appropriate locations though. |
|
Honestly, the See the discussion on swiftlang/swift#72409 for more info on what they're doing. |
|
I suspect this is causing macOS PR testing failures, not clear yet how. |
The intent here is to permit the Windows/macOS style cross-compilation for Android. This involves passing
-sdkwith the path to the "Swift SDK" which overlays the system's native SDK (NDK). TheANDROID_NDK_ROOTis a well-defined environment variable (setup by the SDK installer as well as a general expectation for Android development) that identifies the root of the installation of the NDK. This allows us to locate the native SDK root (--sysroot) for driving the linker driver amongst other paths.