[Investigate] Android SDK/NDK will be moved to root folder (C:\) on Windows

[AlenaSviridenko]

Hey 👋 !

We were reported an issue with Android NDK path on Windows: #1074

[Under investigation]In order to fix it we are going to move Android SDK to the root folder C:\ in the scope of this PR on July, 22 #1077

Please, let us know through this issue if you have any concerns.
Thanks!

[AlenaSviridenko]

Adding 1 week to target date to give more space for proper validation.

[AlenaSviridenko]

After multiple discussions we decided not to make this change in the near future since the possible impact is unpredictable.
We will monitor if the original issue breaks someone else and will come up with the final decision.

[MarijnS95]

We will monitor if the original issue breaks someone else and will come up with the final decision.

@AlenaSviridenko We are still facing this issue, see the linked PR and one of our failed runs. It's worked around by setting up a symlink from a folder without spaces (the inverse of #1077) and overriding environment variables which is far from ideal. Are there any plans to resurrect and land this fix?

[AlenaSviridenko]

Hey @MarijnS95,
due to the small amount of reported issues caused by this space we still don't have any plans to fix it, as the possible impact is unpredictable, the first possible impacted group that comes to mind is customers that might have hard-coded path and there could be more of them.

It would be much appreciated if you could post your workaround in case if anyone else will have this issue in future.
Thank you.

[MarijnS95]

due to the small amount of reported issues caused by this space we still don't have any plans to fix it

That's unfortunate, from what I have seen most scripts still pull in the SDK/NDK by hand without knowing these are already available in GH images which is a likely cause for the absence of reports. Or efforts to switch to this resulted in the same error, without time available to investigate a fix or workaround.

I want to reiterate again that the NDK is hardcoded to warn/error on spaces and pretty much not function at all; this is broken as it is right now.

the first possible impacted group that comes to mind is customers that might have hard-coded path and there could be more of them.

That should be addressed by the PR creating a symlink from the old to the new directory? Then this only leaves room for hardcoded paths that perform some comparison to this variable.

It would be much appreciated if you could post your workaround in case if anyone else will have this issue in future.

Sure, the direct commits are rust-mobile/ndk@b89ecba and rust-mobile/ndk@f0fc14b, together:

    - if: runner.os == 'Windows'
      name: Create symlink to Android SDK/NDK without spaces
      run: |
        $oldAndroidPath = $env:ANDROID_HOME
        $sdk_root = "C:\Android"
        New-Item -Path $sdk_root -ItemType SymbolicLink -Value $oldAndroidPath
        echo "ANDROID_SDK_ROOT=$sdk_root" >> $env:GITHUB_ENV
        echo "ANDROID_NDK_ROOT=$sdk_root\ndk-bundle" >> $env:GITHUB_ENV
        # Update legacy path for ndk-build:
        echo "ANDROID_HOME=$sdk_root" >> $env:GITHUB_ENV
        # "Unset" legacy paths:
        echo "ANDROID_NDK_HOME=" >> $env:GITHUB_ENV
        echo "ANDROID_NDK_PATH=" >> $env:GITHUB_ENV

Note that this is not literally unsetting the variables, merely overwriting them with an empty string. I'll gladly update this code listing if that's possible one way or another. If these variables need to point to a valid NDK in your environment, set them to $sdk_root\ndk-bundle instead.

[AlenaSviridenko]

Thank you @MarijnS95 for the detailed explanation, I will re-open this issue to perform deeper investigation on possible impact and implementation ideas.

[dsame]

First of all, the reason of the problem is the content of C:\Program Files (x86)\Android\android-sdk\ndk-bundle\ndk-build.cmd

It contains a line

%~dp0\build\ndk-build.cmd %*

Namely %~dp0 is expanded to the path with spaces.

It must have double quotes to work properly

"%~dp0\build\ndk-build.cmd" %*

Might be we should patch the file and create the ticket for Google?

[MarijnS95]

@dsame I won't be surprised if the answer is "use paths without spaces". There is a warning in the NDK as well as the Android Studio installer, someone must have tried to fix this before and seeing breakage in obscure places. These wrapper scripts (on Linux as well) are just an obvious first utterance of the problem.

Let's hope that assumption is wrong, the scripts are the only missing piece to support whitespace paths, and Google is willing to accept a patch or fix it :)

[dsame]

@MarijnS95 I agree with you there might be some other issues related to the path with the spaces, but the changing the value of ANDROID_HOME variable seems to be the dangerous breaking change which affects the very wide group of users. This is why i prefer to stay with the fixing of just one script so far.

[MarijnS95]

@dsame Don't forget to replace similar issues in all the clang wrapper scripts, which is what I came here for.

[vvb2060]

android/ndk#1400

[vvb2060]

My tests show that patch %ANDROID_SDK_ROOT%\ndk\21.3.6528147\ndk-build.cmd does not solve the problem, requires a lot of changes in %ANDROID_SDK_ROOT%\ndk\21.3.6528147\build\core\. Google says this is extremely difficult to do correctly in a make based system and may just warn about spaces.
Changing the environment variable path is the only solution.

[MarijnS95]

@vvb2060 Indeed, changing just ndk-build.cmd is not (nearly) enough. We depend on all the compiler wrapper scripts in ./toolchains/llvm/prebuilt/windows-x86_64/bin/<architecture>-clang*.cmd being capable of handling spaces, and have not gone beyond checking what else might break.

Again, it seems unlikely that this project can quickly and consistently fix all whitespace issues given that Google engineers working on the NDK have not done so yet and advise against even trying because of mentioned nesting in eval.

[dsame]

@MarijnS95 @vvb2060 i got your statements. One more question before going the way you've suggested: don't you think it would be enough to change ANDROID_NDK_ROOT only and to do not touch ANDROID_SDK_ROOT? I almost the answer is "no" but just in case.

[vvb2060]

AGP uses ANDROID_SDK_ROOT to determine the path of auto downloaded NDK, ANDROID_NDK_ROOT does not seem to exist in the doc.

[MarijnS95]

@dsame Not that I know of, we only use the NDK. Since its location sits within the SDK it would be weird not to keep them in sync. Again, how easy would it be to move the entire thing to C:\Android or something like that, and set up a link / directory junction from C:\Program Files\Android to that (done in #1077)?

@vvb2060 That page only lists SDK variables, not a single mention of the NDK (other applications will use the 10+ variants of NDK environment variables though 😞). Anyway, on an "obscure" unrelated page (Vulkan) the docs now state ANDROID_NDK_ROOT is deprecated again: https://developer.android.com/ndk/guides/graphics/getting-started#testing 🤷

[dsame]

@MarijnS95 @vvb2060 finally the PR is merged. ANDROID_SDK_ROOT to be changed in the next rollout

[MarijnS95]

@dsame Awesome, thanks for fully moving it in the end!

I do have some questions about hardlink vs symlink vs junction though, see the PR.

[miketimofeev]

We've finished windows image deployments with the changes!