Do not add -sysroot flag when ANDROID_NDK_ROOT environment is set

[marcprux]

When the environment ANDROID_NDK_ROOT is set, the driver manually adds it as a --sysroot flag. This breaks building with an Android SDK when this variable is set, with confusing errors. I first saw this at finagolfin/swift-android-sdk#207 (comment), and more recently ran into it again (https://github.com/swift-android-sdk/swift-docker/actions/runs/14584558227/job/40907674425) while working on the official Android SDK build script, where it fails a build with:

<unknown>:0: error: missing required module 'SwiftAndroid'

In both cases, manually un-setting ANDROID_NDK_ROOT (which is set by default in GitHub workflows) resolved the issue.

This flag has some history (#1560, #1681, and #1811), but I feel like it should just be excised altogether since the Android SDK can specify its own sdkRootPath in the swift-sdk.json file. If Windows needs it due to lack of Swift SDK support, then it could be gated inside an #if os(Windows) check.

And BTW, the #if arch(x86_64) check is incorrect: the Android NDK works on both Intel and ARM macOS machines, despite being stored under "darwin-x86_64", as the binaries are universal:

zap ~ % file $ANDROID_HOME/ndk/27.0.12077973/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang
~/Library/Android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang: Mach-O universal binary with 2 architectures: [x86_64:Mach-O 64-bit executable x86_64] [arm64]
~/Library/Android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang (for architecture x86_64):	Mach-O 64-bit executable x86_64
~/Library/Android/sdk/ndk/27.0.12077973/toolchains/llvm/prebuilt/darwin-x86_64/bin/clang (for architecture arm64):	Mach-O 64-bit executable arm64

[marcprux]

I've marked this ready for review, @jakepetroules, @finagolfin, and @compnerd.

See also the discussion at swiftlang/swift-build#495 (comment)

[finagolfin]

I agree with getting rid of this. As much as the idea is to automatically set things up and make things automatically work, I think it's better to have the user configure this themselves. For example, this env variable is automatically set on the github CI, so even if the user explicitly configures the sdkRootPath to some other NDK, that will be overridden by this environment variable.

Better to not have such magic and make the setup more explicit, as it would be after this pull.

[compnerd]

This is going to break all of our builds, please do not commit this as is. We need the ability to be able to pass the -sysroot through. I don't think that this is the right approach.

[marcprux]

This is going to break all of our builds

Then how about gating it within an #if os(Windows) check? This issue a blocker for Android builds on macOS and Linux, so we need some solution.

[jakepetroules]

This is going to break all of our builds, please do not commit this as is. We need the ability to be able to pass the -sysroot through. I don't think that this is the right approach.

This change isn't removing the ability to do that, right? It's just removing the default fallback. From what I understand, explicitly passing -sysroot is still gonna work, and that's what we want build systems to do anyways. Am I understanding incorrectly?

[marcprux]

@compnerd Any thoughts on either @jakepetroules's questions or my suggestion? I'd really like to reach some solution so we can un-block the Android SDK work.

[compnerd]

This is going to break all of our builds, please do not commit this as is. We need the ability to be able to pass the -sysroot through. I don't think that this is the right approach.

This change isn't removing the ability to do that, right? It's just removing the default fallback. From what I understand, explicitly passing -sysroot is still gonna work, and that's what we want build systems to do anyways. Am I understanding incorrectly?

Ah, right; this still will break our builds as we do rely on the default. Can we wire up the default through swift-build before we remove this?

[jakepetroules]

As far as I know, swift-build is already doing the right thing here, e.g. it will always pass a value for -sdk when compiling for Android.

[finagolfin]

@compnerd or @weliveindetail, could you apply this change to your CI and let us know if it causes any error and what the full output is? Because that may turn up an underlying bug that is better fixed rather than papering over it with this Android default.

[marcprux]

This hasn't made it into the swift-6.2 brach, has it? I'm still seeing the build failures when I have ANDROID_NDK_ROOT set, which is especially problematic because the developer needs to set ANDROID_NDK_HOME to run the SDK setup script initially, but needs to un-set ANDROID_NDK_ROOT. These are both set to the same value in GitHub actions.

@finagolfin Can you add this to the project board?

[jakepetroules]

Can we get this updated and landed? @compnerd do you still have concerns?

[compnerd]

I think that we should wait until @finagolfin's frontend/driver changes are done and then re-test before merging this. The --sysroot flag is needed to allow us to pull content from the NDK without copying it into the Swift SDK, so I would prefer that we do not merge this change.

[finagolfin]

My frontend change in swiftlang/swift#79621 only affects -sdk usage, does not affect this -sysroot usage at all.

It is unclear why this env variable detection and -sysroot flag insertion is so important to your builds, as you could just detect it manually in your build scripts and add it in your config yourself. This is really just a convenience method for swift-driver to do that for you, but the problem is that it breaks other build environments that have multiple NDKs and don't use the env variable but manually pass in the NDK path.

Is there some ordering issue or something, on the TBC CI builds with this pull? If you guys would simply try this patch and replace it with manually passing in this flag to your build config, we could determine if this was really necessary to you or not, from the resulting error output, if any.

[finagolfin]

@marcprux, mind rebasing?

[finagolfin]

@swift-ci test

[finagolfin]

@swift-ci test windows

[finagolfin]

@Steelskin, can you apply this pull to your Windows CI, along with passing in this -sysroot ANDROID_NDK_ROOT flag manually through wherever your Android build config is, similar to how the Powershell script build.ps1 is already doing this, and report any build errors that you see?

The problem with this default is that it forces everyone to use the NDK at ANDROID_NDK_ROOT or have to unset that variable each time, when many have alternate NDKs they want to use through swift sdk configure instead, swiftlang/swift-package-manager#8856.

[Steelskin]

@Steelskin, can you apply this pull to your Windows CI, along with passing in this -sysroot ANDROID_NDK_ROOT flag manually through wherever your Android build config is, similar to how the Powershell script build.ps1 is already doing this, and report any build errors that you see?

The problem with this default is that it forces everyone to use the NDK at ANDROID_NDK_ROOT or have to unset that variable each time, when many have alternate NDKs they want to use through swift sdk configure instead, swiftlang/swift-package-manager#8856.

I am in the process of changing our build to more closely match what is being done in buid.ps1, but this is still a work in progress. In the meantime, it is going to be a bit challenging to get this to run so bear with me.
We run our smoke android test with the following:

          swift build `
            --package-path ${{ github.workspace }}/SourceCache/cassowary `
            --triple ${{ matrix.arch }}-unknown-linux-android${{ inputs.ANDROID_API_LEVEL }} `
            --sdk "${{ steps.android-swift-env.outputs.sysroot }}" `
            -Xswiftc -sdk -Xswiftc "${{ steps.android-swift-env.outputs.sdkroot }}" `
            -Xswiftc -sysroot -Xswiftc "${{ steps.android-swift-env.outputs.sysroot }}" `
            -Xswiftc -I -Xswiftc "${{ steps.android-swift-env.outputs.sdkroot }}\usr\include" `
            -Xswiftc -Xclang-linker -Xswiftc -resource-dir -Xswiftc -Xclang-linker -Xswiftc ${{ steps.android-swift-env.outputs.clang-resource-dir }}

Does that sound about correct to you or would you like me to edit some of the flags and/or the environment here?

[finagolfin]

Are you sure you need both --sdk sdk-path and -Xswiftc -sdk -Xswiftc sdk-path? The latter seems redundant, but perhaps there is a bug I'm unaware of.

Other than that, I'd like you to run these flags you've pasted but using a swift-driver with this pull of Marc's applied: see if removing this ANDROID_NDK_ROOT detection causes any problem on your CI when you're passing it in with -Xswiftc -sysroot -Xswiftc ndk-path instead.

[Steelskin]

Are you sure you need both --sdk sdk-path and -Xswiftc -sdk -Xswiftc sdk-path? The latter seems redundant, but perhaps there is a bug I'm unaware of.

Not sure, this is how the build is configured. I can toy with what arguments we actually need at a later time.

Other than that, I'd like you to run these flags you've pasted but using a swift-driver with this pull of Marc's applied: see if removing this ANDROID_NDK_ROOT detection causes any problem on your CI when you're passing it in with -Xswiftc -sysroot -Xswiftc ndk-path instead.

Alright, got it. All I did was change the swift-driver repo to point to swift-everywhere:main. The build is ongoing, I'll let you know how it goes.

[Steelskin]

@finagolfin Our CI succeeded with the current swift-everywhere:main branch.

[finagolfin]

@compnerd, if you would approve or let us know if you need to test anything else, I'd like to get this in.

[finagolfin]

Pinging @compnerd again, would like to get this into 6.2 next, as I have had to work around this override on my Android CI and so have others. If you have no further concerns, please approve and we'll get this in.

[jakepetroules]

Thanks for the explanation, this is making things a little clearer. Can you also clarify whether you're building via SwiftPM, or building with a different build system?

If sdkRootPath is supposed to correspond to the Swift SDK bits and not the platform sysroot, is SwiftPM doing the right thing today? It looks like -sdk is being passed for Swift, but this one for the C compiler needs correction.

if let sdkDir = swiftSDK.pathsConfiguration.sdkRootPath {
    let sysrootFlags = [triple.isDarwin() ? "-isysroot" : "--sysroot", sdkDir.pathString]
    self.extraFlags.cCompilerFlags.insert(contentsOf: sysrootFlags, at: 0)
}

but that largely has not been implemented yet.

What's still missing?

This pull reverts the env variable checking because it inserts that new -sysroot flag regardless of which pair of cross-compilation flags above are being used, breaking most current Android cross-compilation with the -sdk/-resource-dir flags.

Could this have some smarter detection? e.g. don't add the default --sysroot flag if -sdk is set and points to something that looks like an NDK sysroot?

[finagolfin]

Can you also clarify whether you're building via SwiftPM, or building with a different build system?

The build system is irrelevant, as these are Swift compiler flags that any build system has to use. I merely focused on SwiftPM because it is what most use, but these Swift compiler flags used for cross-compilation are the same regardless of build system, whether CMake or Bazel.

If sdkRootPath is supposed to correspond to the Swift SDK bits and not the platform sysroot, is SwiftPM doing the right thing today?

OK, I can see you are confused, so let me get into the slightly longer expanation. Until recently, there were supposed to be two options to cross-compile using the -sdk flag. You could specify "-sdk, for the platform C/C++ sysroot" alone, contrary to what you just wrote, and "-resource-dir, for the Swift runtime modules and libraries." If you happened to have a non-Darwin C/C++ sysroot that also contained the Swift runtime in <sdk>/usr/lib/swift/, you could specify -sdk alone, but only with the legacy C++ Driver: that was never brought over to this new swift-driver (I'm working on a pull to bring that capability over to the swift-frontend, which this swift-driver invokes to find such flags, swiftlang/swift#79621).

Well, when the -sdk/-sysroot switch was proposed in the cross-compilation document I linked above, my guess is that its authors thought they could reuse that capability to look in -sdk for the Swift runtime libraries instead, and split off the platform C/C++ sysroot into the new -sysroot flag. However, they were likely unaware that this new swift-driver does not look in -sdk for the Swift runtime libraries, and their effort to switch to -sdk/-sysroot appears to have collapsed.

The SwiftPM flag you pasted is correct under the -sdk/-resource-dir cross-compilation model almost everybody is now using, but you are right that it may cause problems if we ever made the switch to the -sdk/-sysroot model that almost nobody is now using and is mostly unimplemented.

The only reason the TBC CI is able to use -sdk/-sysroot above is because either Swift cross-compilation flag pair is then used to automatically set a bunch of -I/-L/--sysroot flags internal to the Swift compiler, so they replace that with a bunch of manually passed-in flags like those shown above. That is brittle and tends to break though, better to choose a pair of front-facing cross-compilation flags and keep it simple.

What's still missing?

The main one is actually having this swift-driver look in -sdk for the Swift runtime libraries! After a year of my pointing out that that basic capability doesn't work to those who wanted to switch to -sdk/-sysroot and not getting a response until recently, I finally stepped up to implement it myself. Another is that -sysroot still has bugs, since almost nobody is using it.

Could this have some smarter detection? e.g. don't add the default --sysroot flag if -sdk is set and points to something that looks like an NDK sysroot?

Given all the -sdk/-sysroot problems detailed above, best to just take it out for now, as Marc does in this pull. Another issue is that there's no guarantee ANDROID_NDK_ROOT is the NDK that users want, so we'd need to have a story in place of how to override it.

A final aspect that I don't like is that we have an explicit flag for this in SwiftPM, better to use that than these magic environment variables. You think you're making life easier for the user, but you're really just introducing hidden variables that can break things in hard to debug ways.

Let me finally note that I'm not against the -sdk/-sysroot switch, but I do have doubts that it can be made to work and most importantly, we shouldn't be pushing in this -sysroot flag for Android alone until the underlying -sdk/-sysroot support is fully implemented.

[finagolfin]

Ping @compnerd or @jakepetroules, can we go ahead and get this in? The behavior this pull reverts is clearly not needed on Windows, where it can always be passed in manually, and as detailed above, it breaks most Android builds when used with SDK bundles, which only support -sdk/-resource-dir and not the -sysroot flag it adds.

This pull gets Android building again on CI environments which set this environment variable, where Marc and I have had to unset this variable to avoid this broken behavior and warn others to do the same.

Since -sdk/-sysroot cross-compilation currently doesn't work well, with even the Windows build above for Android having to add several flags to make up for those problems, it is best to not jam in the -sysroot flag like this. Once we get it working, and I'm still working on that swift-frontend pull to help get it there, we can revisit adding something like this later on down the line.

[artemcm]

I chatted with @jakepetroules, @owenv, @compnerd and @etcwilde and have a proposal.

One key thing we’d like to do is to better delineate is the use of the various discussed flags here. The ones we’re generally concerned with are:

• -sysroot: Native system root on platforms that have one.
• -sdk: Swift SDK root
• -resource-dir: Compiler toolchain-specific contents and headers. In the case of Swift, these are header files used by the ClangImporter, SwiftShims, etc. It is our expectation that this should generally never be specified by the user.

One some platforms, sys root and SDK root are the same (read Apple platforms), in which case no explicit -sysroot is necessary, only -sdk is needed. On Windows there is not really a concept of a sysroot, so similarly only -sdk is required. All other platforms should specify both -sdk and -sysroot.

It’s also critical that we do not conflate the required Swift bits (resource dir and SDK) and the native system sysroot and keep those concepts separate.

Regarding ANDROID_NDK_HOME, there is value in continuing to use this environment variable to match other toolchains, and to minimize the option set needed from a user when using an Android triple - either by just building with swiftc or via a build system like SwiftPM.

For the matter of this PR, we would like to establish the above semantics for each of these options, but it would also be clearly valuable to ease with the transition here and one thing we can do is change this PR to disable ANDROID_NDK_HOME inference only when -resource-dir is specified to accommodate the current workflows. With a long term goal of removing this change.

[finagolfin]

Compiler toolchain-specific contents and headers. In the case of Swift, these are header files used by the ClangImporter, SwiftShims, etc. It is our expectation that this should generally never be specified by the user.

All other platforms should specify both -sdk and -sysroot.

This is currently aspirational and doesn't really work on any platform, hence all the flags listed for Windows above too.

Regarding ANDROID_NDK_HOME, there is value in continuing to use this environment variable to match other toolchains, and to minimize the option set needed from a user when using an Android triple - either by just building with swiftc or via a build system like SwiftPM.

There would be value if -sdk/-sysroot currently worked on any platform, it currently does not. To take just one example, the Android doc has listed the -sdk/-resource-dir flags to build a simple executable for the last five years, since I switched it over along with simplifying for the NDK's unified sysroot. There has never been a -sdk/-sysroot equivalent that worked, without adding a bunch more flags to make up for the fact it doesn't work.

For the matter of this PR, we would like to establish the above semantics for each of these options, but it would also be clearly valuable to ease with the transition here and one thing we can do is change this PR to disable ANDROID_NDK_HOME inference only when -resource-dir is specified to accommodate the current workflows. With a long term goal of removing this change.

-sdk/-sysroot will require significantly more work for it to be viable to target non-Darwin platforms, better to just take this out for now as Marc does here. If you doubt that, take a look at swiftlang/swift#79657 and swiftlang/swift#81842 to dive into the issues that remain. Until those issues are meaningfully fixed, foisting this broken -sdk/-sysroot approach on users is actually breaking builds: it is not helping.

If and when -sdk/-sysroot works well, and I'm trying to help that with my frontend pull swiftlang/swift#79621, it would make sense to discuss inserting it like this then, but not now when it doesn't work.

[marcprux]

I'd like to raise attention to this issue again, since it is still the case that trying to build for Android with ANDROID_NDK_HOME set will cause the build to fail on every platform except Windows, and requires workarounds like this.

If improving this PR to special-case Windows is not satisfactory, what would be an acceptable solution? Is there any way we can get some solution — even a stopgap one — in place before we start publishing the SDK?

[finagolfin]

Their compromise will work to elide the broken code for most SDK bundles, but I think we should remove the broken -sysroot flag insertion altogether, as it mostly doesn't work right now, in case someone mistakenly doesn't set the -resource-dir flag and hits that mostly broken env variable code path.

Up to you what you want to do...

[ephemer]

Am I understanding correctly that having ANDROID_NDK_HOME set will set flags in the Swift compiler that would be unset otherwise? But the flags it sets are actually redundant: the settings they refer to can be – and in many cases in practice indeed are – configured by other flags?

If so, the way I understand this, we have an environment variable that is intended to be used as a fallback actually overwriting things it shouldn't.

While I understand the motivation for using a platform-standard environment variable (ANDROID_NDK_HOME) for configuring certain settings, the issue is that we have multiple redundant flags for kind of doing the same thing. This makes using the environment variable as a fallback "only" is not effective. The redundancy in question cannot be trivially removed though, and doing so would be a much larger fix.

Is there an alternative of ensuring only one of the two sets of flags has an effect?

[finagolfin]

Am I understanding correctly that having ANDROID_NDK_HOME set will set flags in the Swift compiler that would be unset otherwise?

Yes

But the flags it sets are actually redundant: the settings they refer to can be – and in many cases in practice indeed are – configured by other flags?

Not merely redundant, they actually break the build in the vast majority of cases if that environment variable is set, as all SDK bundles currently set the -resource-dir flag instead, which leads to conflicts with the -sysroot flag set here.

we have an environment variable that is intended to be used as a fallback actually overwriting things it shouldn't.

The way I would phrase it is that this is a convenience method to find the Android sysroot for you. However, since almost everybody uses -sdk/-resource-dir right now, that convenience method is instead breaking almost all CI builds! To me, that is a no-brainer, breaking builds should always take priority over convenience methods.

Artem and the others above have put forth a compromise, ie only invoke this convenience method if -resource-dir is not set. I would rather we just took out this mostly broken convenience method, as this pull does now, but it is up to Marc, who submitted this pull.

[marcprux]

I'll plead once more for attention to this. With the imminent release of the Swift SDK for Android, anyone who happens to have ANDROID_NDK_ROOT set in their environment is going to encounter a mystifying error when they try to build their packages (including GitHub actions):

<unknown>:0: error: missing required module 'SwiftAndroid'

I just don't see why we are consulting this environment at all, at least on non-Windows platforms.

[finagolfin]

One key thing we’d like to do is to better delineate is the use of the various discussed flags here. The ones we’re generally concerned with are:

-sysroot: Native system root on platforms that have one.
-sdk: Swift SDK root
-resource-dir: Compiler toolchain-specific contents and headers. In the case of Swift, these are header files used by the ClangImporter, SwiftShims, etc. It is our expectation that this should generally never be specified by the user.
One some platforms, sys root and SDK root are the same (read Apple platforms), in which case no explicit -sysroot is necessary, only -sdk is needed. On Windows there is not really a concept of a sysroot, so similarly only -sdk is required. All other platforms should specify both -sdk and -sysroot.

@artemcm, let me demonstrate a core way in which this understanding you have laid out is blatantly wrong on all platforms. There is currently a test in the compiler validation suite that ensures that the resource directory, whether implicit or explicitly passed in with the -resource-dir flag, is looked in first for runtime swiftmodules for the target platform:

// If the compiler's resource directory *does* contain a runtime swiftmodule, we
// should *not* use the one in the SDK. (We assume that the resource directory
// built with this compiler does contain a standard library.)

// RUN: %empty-directory(%t/mcp)
// RUN: %target-swift-frontend(mock-sdk: -sdk %t/bad-sdk) -module-cache-path %t/mcp -typecheck -verify -disable-implicit-concurrency-module-import -disable-implicit-string-processing-module-import %s

This test is currently only run on Darwin CI, but the underlying logic is the same on all platforms, which is why I was able to enable it on linux and Windows also with a couple minor tweaks to the test format alone, ie no changes to the Swift compiler.

This directly contradicts your claim that the -sdk is where Swift SDK files like the platform runtime swiftmodules are always looked for. In fact, the Swift resource directory is always where the compiler looks for Swift runtime modules first, only falling back to the -sdk if it doesn't find them there. On top of that, there are other gaping holes in your desired approach of looking for Swift SDK files like the runtime libraries in the passed-in -sdk, #1562, which makes the -sdk/-sysroot approach you espouse unworkable right now, unless you manually make up for it with a dozen more flags like Fabrice is above.

All SDK bundles currently use the long-time -sdk/-resource-dir approach to cross-compilation, while almost nobody is using the new -sdk/-sysroot alternative you are pushing. That is because of significant flaws in this new approach.

I am not saying let's not move to the new approach: I'm simply saying let's not break the old approach by pushing the new approach in when it doesn't really work yet. This pull by Marc would give us more time to fix the new -sysroot approach before we come back to it later, as I proposed months ago.

[jakepetroules]

@swift-ci test

[jakepetroules]

@compnerd I think we should merge this at this point, since SwiftPM is already passing the necessary flags to activate the NDK without relying on the env var, and also: the code here is behind #if arch(x86_64). So it's basically not even taking effect in the first place on macOS, nor for a number of users on other platforms as well.

I think most of the other discussion on this PR is perhaps better had in swiftlang/swift#79657

[jakepetroules]

@swift-ci test windows

[compnerd]

Discussed with @jakepetroules offline. The variable here is correct. This is the older variable. ANDROID_NDK_ROOT predates ANDROID_NDK_HOME and is used by many projects (including OpenSSL and godot).

I think that we can merge this for now, but be okay with the idea of a quick revert if we find that things are broken. We are still fighting a Debug Info generation issue that was introduced in main and so cannot test this right away.

I will want to see this restored in the future, as this avoids the need to pass -sysroot in favour of an environment variable that can be set once.

[finagolfin]

The issue is not the name of the environment variable: the issue is that you only use this new -sysroot flag for Android cross-compilation on Windows hosts, whereas SDK bundles use -sdk/-resource-dir instead, which passing in this incompatible -sysroot flag breaks.

As such, it will not make sense to ever bring these inserted flags back until there is some change in that SDK bundle JSON format that allows it. I realize you don't use SDK bundles on Windows yet, but since you can always pass this -sysroot flag in manually or in a local config file, better to take out this magic method that is currently breaking Android CI, as we had to unset this env var in the official swiftlang workflows to work around this -sysroot bug.

Regarding what to do in the future about helping Android devs configure this easier, we can discuss that more with Jake and decide, since he is baking some NDK support into the swift-build package.

Merging now since we are in agreement and we can remove those linked CI workarounds once this fix is merged, then let's discuss more elsewhere and come up with a better solution if needed.

[marcprux]

Can we cherry-pick this for 6.3?