Fix NDK path detection

e2e/bitrise.yml

@@ -132,24 +132,6 @@ workflows:
         inputs:
         - gradle_task: assembleDebug
 
-  test_support_library:
-    envs:
-    - SAMPLE_APP_URL: https://github.com/bitrise-io/sample-apps-android-x.git
-    - BRANCH: master
-    - GRADLE_BUILD_FILE_PATH: build.gradle
-    - GRADLEW_PATH: ./gradlew
-    before_run:
-    - _setup
-    steps:
-    - script:
-        title: Uninstall Android SDK
-        inputs:
-        - content: |-
-            #!/usr/bin/env bash
-            sdkmanager --uninstall "extras;android;m2repository"
-    after_run:
-    - _run_and_build
-
   test_build_tools_not_installed:
     envs:
     - SAMPLE_APP_URL: https://github.com/bitrise-io/Bitrise-Android-Sample.git
@@ -172,24 +154,6 @@ workflows:
 
   _setup:
     steps:
-    - script:
-        title: Enable JDK 11
-        description: This is still needed on the vs4mac stack
-        run_if: $.IsCI
-        inputs:
-        - content: |-
-            #!/usr/bin/env bash
-            set -ex
-            if [[ "$OSTYPE" == "linux-gnu"* ]]; then
-              sudo update-alternatives --set javac /usr/lib/jvm/java-11-openjdk-amd64/bin/javac
-              sudo update-alternatives --set java /usr/lib/jvm/java-11-openjdk-amd64/bin/java
-              export JAVA_HOME="/usr/lib/jvm/java-11-openjdk-amd64"
-              envman add --key JAVA_HOME --value "/usr/lib/jvm/java-11-openjdk-amd64"
-            elif [[ "$OSTYPE" == "darwin"* ]]; then
-              jenv global 11 || jenv global 11.0
-              export JAVA_HOME="$(jenv prefix)"
-              envman add --key JAVA_HOME --value "$(jenv prefix)"
-            fi
     - script:
         title: Clean _tmp dir
         inputs:
@@ -209,6 +173,9 @@ workflows:
         - repository_url: $SAMPLE_APP_URL
         - clone_into_dir: .
         - branch: $BRANCH
+    - set-java-version:
+        inputs:
+        - set_java_version: "17"
 
   _run_and_build:
     steps:
@@ -238,6 +205,10 @@ workflows:
             #!/usr/bin/env bash
             set -ex
 
+            # If there is no ndk-bundle (only side-by-side NDKs), we don't need to back it up
+            if [ ! -d $ANDROID_HOME/ndk-bundle ]; then
+              exit 0
+            fi
             cp -R $ANDROID_HOME/ndk-bundle $ANDROID_HOME/ndk-bundle-original
 
   _restore_ndk_bundle:

main.go

@@ -13,8 +13,10 @@ import (
 	"github.com/bitrise-io/go-steputils/tools"
 	"github.com/bitrise-io/go-utils/fileutil"
 	"github.com/bitrise-io/go-utils/log"
+	"github.com/bitrise-io/go-utils/v2/env"
 	"github.com/bitrise-io/go-utils/v2/errorutil"
 	. "github.com/bitrise-io/go-utils/v2/exitcode"
+	"github.com/bitrise-io/go-utils/v2/log/colorstring"
 	"github.com/bitrise-steplib/steps-install-missing-android-tools/androidcomponents"
 	"github.com/hashicorp/go-version"
 	"github.com/kballard/go-shellquote"
@@ -117,14 +119,14 @@ func (i AndroidToolsInstaller) Run(config Config) error {
 		}
 
 		if err := updateNDK(config.NDKVersion, androidSdk); err != nil {
-			return fmt.Errorf("failed to install new NDK package: %w", err)
+			return fmt.Errorf("install new NDK package: %w", err)
 		}
 	} else {
 		log.Infof("Clearing NDK environment")
 		log.Printf("Unset ANDROID_NDK_HOME")
 
 		if err := os.Unsetenv("ANDROID_NDK_HOME"); err != nil {
-			return fmt.Errorf("failed to unset environment variable: %w", err)
+			return fmt.Errorf("unset environment variable: %w", err)
 		}
 
 		if err := tools.ExportEnvironmentWithEnvman("ANDROID_NDK_HOME", ""); err != nil {
@@ -173,58 +175,74 @@ func ndkVersion(ndkPath string) string {
 	return ""
 }
 
-func currentNDKHome() string {
-	if v := os.Getenv(androidNDKHome); v != "" {
-		return v
+// targetNDKPath picks the correct path where the requested NDK version should be installed,
+// as well as returning if the path needs to be cleaned up before install.
+// There are many (deprecated) ways to install NDK, the logic is based on the following:
+// https://github.com/android/ndk-samples/wiki/Configure-NDK-Path
+// https://developer.android.com/tools/variables
+func targetNDKPath(envRepo env.Repository, requestedNDKVersion string) (string, bool) {
+	if v := envRepo.Get(androidNDKHome); v != "" {
+		// $ANDROID_NDK_HOME is old and AGP no longer takes it into account,
+		// but it's an explicit path, so use it if it's set on the system.
+		// And because we don't know if it's `ndk-bundle` or a specific side-by-side version,
+		// we return true for cleanup.
+		log.Warnf("$%s is set to %s", androidNDKHome, v)
+		log.Warnf("This variable is deprecated and modern Android Gradle Plugin versions no longer take it into account.")
+		return v, true
 	}
-	if v := os.Getenv("ANDROID_HOME"); v != "" {
-		// $ANDROID_HOME is deprecated
-		return filepath.Join(v, "ndk-bundle")
+	if androidHome := envRepo.Get("ANDROID_HOME"); androidHome != "" {
+		// The most modern way is to install NDK versions side-by-side at $ANDROID_HOME/ndk/version
+		// This is what `sdkmanager` does when installing a specific version (`sdkmanager "ndk;26.3.11579264"`).
+		ndkPath := filepath.Join(androidHome, "ndk", requestedNDKVersion)
+		return ndkPath, false
 	}
-	if v := os.Getenv("ANDROID_SDK_ROOT"); v != "" {
-		// $ANDROID_SDK_ROOT is preferred over $ANDROID_HOME
-		return filepath.Join(v, "ndk-bundle")
+	if sdkRoot := envRepo.Get("ANDROID_SDK_ROOT"); sdkRoot != "" {
+		// $ANDROID_SDK_ROOT is deprecated, so it's lower in priority than $ANDROID_HOME
+		ndkPath := filepath.Join(sdkRoot, "ndk", requestedNDKVersion)
+		return ndkPath, false
 	}
-	if v := os.Getenv("HOME"); v != "" {
-		return filepath.Join(v, "ndk-bundle")
+	if v := envRepo.Get("HOME"); v != "" {
+		// Worst case: just install to home (and later export as $ANDROID_NDK_HOME)
+		return filepath.Join(v, "ndk-bundle"), true
 	}
-	return "ndk-bundle"
+	return "ndk-bundle", true
 }
 
 // updateNDK installs the requested NDK version (if not already installed to the correct location).
-// NDK is installed to the `ndk/version` subdirectory of the SDK location, while updating $ANDROID_NDK_HOME for
-// compatibility with older Android Gradle Plugin versions.
-// Details: https://github.com/android/ndk-samples/wiki/Configure-NDK-Path
 func updateNDK(version string, androidSdk *sdk.Model) error {
-	currentNdkHome := currentNDKHome()
-
-	currentVersion := ndkVersion(currentNdkHome)
-	if currentVersion == version {
-		log.Donef("NDK %s already installed at %s", version, currentNdkHome)
-		return nil
+	envRepo := env.NewRepository()
+	targetNDKPath, doCleanup := targetNDKPath(envRepo, version)
+	currentVersionAtPath := ndkVersion(targetNDKPath)
+	if currentVersionAtPath != "" {
+		log.Printf("NDK %s found at %s", colorstring.Cyan(currentVersionAtPath), targetNDKPath)
+	} else {
+		log.Printf("NDK %s %s found at %s", colorstring.Cyan(version), colorstring.Yellow("not"), targetNDKPath)
 	}
 
-	if currentVersion != "" {
-		log.Printf("NDK %s found at: %s", currentVersion, currentNdkHome)
+	if currentVersionAtPath == version {
+		log.Donef("NDK %s is already installed", version)
+		return nil
 	}
 
-	log.Printf("Removing existing NDK...")
-	if err := os.RemoveAll(currentNdkHome); err != nil {
-		return err
+	if currentVersionAtPath != "" || doCleanup {
+		log.Printf("Removing existing NDK...")
+		if err := os.RemoveAll(targetNDKPath); err != nil {
+			return err
+		}
+		log.Printf("Done")
 	}
-	log.Printf("Done")
 
-	log.Printf("Installing NDK %s with sdkmanager", version)
+	log.Printf("Installing NDK %s with sdkmanager", colorstring.Cyan(version))
 	sdkManager, err := sdkmanager.New(androidSdk)
 	if err != nil {
 		return err
 	}
 	ndkComponent := sdkcomponent.NDK{Version: version}
 	cmd := sdkManager.InstallCommand(ndkComponent)
-	output, err := cmd.RunAndReturnTrimmedOutput()
+	output, err := cmd.RunAndReturnTrimmedCombinedOutput()
 	if err != nil {
 		log.Errorf(output)
-		return err
+		return fmt.Errorf("run %s: %w", cmd.PrintableCommandArgs(), err)
 	}
 	newNDKHome := filepath.Join(androidSdk.GetAndroidHome(), ndkComponent.InstallPathInAndroidHome())
 

main_test.go

@@ -0,0 +1,137 @@
+package main
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func Test_currentNDKHome(t *testing.T) {
+	type test struct {
+		name string
+		envs map[string]string
+		wantPath string
+		wantCleanup bool
+	}
+
+	requestedNDKVersion := "23.0.7599858"
+	tests := []test{
+		{
+			name: "ANDROID_NDK_HOME is set",
+			envs: map[string]string{
+				"ANDROID_NDK_HOME": "/opt/android-ndk",
+			},
+			wantPath: "/opt/android-ndk",
+			wantCleanup: true,
+		},
+		{
+			name: "both ANDROID_NDK_HOME and ANDROID_HOME are set",
+			envs: map[string]string{
+				"ANDROID_NDK_HOME": "/opt/android-ndk",
+				"ANDROID_HOME": "/opt/android-sdk",
+			},
+			wantPath: "/opt/android-ndk",
+			wantCleanup: true,
+		},
+		{
+			name: "only ndk-bundle is installed",
+			envs: map[string]string{
+				"ANDROID_HOME": "/home/user/android-sdk",
+			},
+			wantPath: "/home/user/android-sdk/ndk/23.0.7599858",
+			wantCleanup: false,
+		},
+		{
+			name: "ndk-bundle and side-by-side NDK is installed",
+			envs: map[string]string{
+				"ANDROID_HOME": "/home/user/android-sdk",
+			},
+			wantPath: "/home/user/android-sdk/ndk/23.0.7599858",
+			wantCleanup: false,
+		},
+		{
+			name: "the exact requested side-by-side NDK is installed",
+			envs: map[string]string{
+				"ANDROID_HOME": "/home/user/android-sdk",
+			},
+			wantPath: "/home/user/android-sdk/ndk/23.0.7599858",
+			wantCleanup: false,
+		},
+		{
+			name: "a different side-by-side NDK is installed than requested",
+			envs: map[string]string{
+				"ANDROID_HOME": "/home/user/android-sdk",
+			},
+			wantPath: "/home/user/android-sdk/ndk/23.0.7599858",
+			wantCleanup: false,
+		},
+		{
+			name: "both ANDROID_HOME and SDK_ROOT are set, pointing to the same dir",
+			envs: map[string]string{
+				"ANDROID_HOME": "/home/user/android-sdk",
+				"SDK_ROOT": "/home/user/android-sdk",
+			},
+			wantPath: "/home/user/android-sdk/ndk/23.0.7599858",
+			wantCleanup: false,
+		},
+		{
+			name: "both ANDROID_HOME and SDK_ROOT are set, pointing to different dirs",
+			envs: map[string]string{
+				"ANDROID_HOME": "/home/user/android-sdk-home",
+				"ANDROID_SDK_ROOT": "/home/user/android-sdk-root",
+			},
+			wantPath: "/home/user/android-sdk-home/ndk/23.0.7599858",
+			wantCleanup: false,
+		},
+		{
+			name: "only SDK_ROOT is set",
+			envs: map[string]string{
+				"ANDROID_SDK_ROOT": "/home/user/android-sdk-root",
+			},
+			wantPath: "/home/user/android-sdk-root/ndk/23.0.7599858",
+			wantCleanup: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			envRepo := fakeEnvRepo{envVars: tt.envs}
+			gotPath, gotCleanup := targetNDKPath(envRepo, requestedNDKVersion)
+			require.Equal(t, tt.wantPath, gotPath)
+			require.Equal(t, tt.wantCleanup, gotCleanup)
+		})
+	}
+
+}
+
+type fakeEnvRepo struct {
+	envVars map[string]string
+}
+
+func (repo fakeEnvRepo) Get(key string) string {
+	value, ok := repo.envVars[key]
+	if ok {
+		return value
+	} else {
+		return ""
+	}
+}
+
+func (repo fakeEnvRepo) Set(key, value string) error {
+	repo.envVars[key] = value
+	return nil
+}
+
+func (repo fakeEnvRepo) Unset(key string) error {
+	repo.envVars[key] = ""
+	return nil
+}
+
+func (repo fakeEnvRepo) List() []string {
+	envs := []string{}
+	for k, v := range repo.envVars {
+		envs = append(envs, fmt.Sprintf("%s=%s", k, v))
+	}
+	return envs
+}