Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump to xamarin/xamarin-android-tools/master@f5fcb9fd #4640

Merged
merged 2 commits into from
May 7, 2020

Conversation

jonpryor
Copy link
Member

Copy link
Contributor

@brendanzagaeski brendanzagaeski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Draft release notes

No worries if the answer to this is that it doesn't need release notes, but while the topic is fresh, does dotnet/android-tools@f5fcb9f need any release notes? Maybe something brief about "Xamarin.Android is now compatible with the new cmdline-tools directory path in recent versions of the Android SDK"? I haven't read dotnet/android-tools@f5fcb9f carefully, but maybe there's also an error this resolves? For example, if users had tried to update to too new a version of the Android SDK without this compatibility update, would they have seen an error that the release notes could mention as now resolved?

Thanks!

@jonpryor
Copy link
Member Author

Thank you for the reminder that I don't think about release notes often enough. :-)

does dotnet/android-tools@f5fcb9f need any release notes?

Probably not.

The thing is that the added AndroidSdkInfo.GetCommandLineToolsPaths() method is not used by xamarin-android. As such, nothing within xamarin-android will start using cmdline-tools.

Whether that's "the right thing to do" is a different question, requiring auditing all use of AndroidSdkInfo within this repo, and:

% git grep AndroidSdkInfo
build-tools/Xamarin.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/JdkInfo.cs:			var androidSdk    = new AndroidSdkInfo (logger, AndroidSdkPath, AndroidNdkPath, JavaSdkPath);
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/KeyToolTests.cs:			var androidSdk = new AndroidSdkInfo ((level, message) => {
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/NdkUtilTests.cs:				MonoAndroidHelper.AndroidSdk = new AndroidSdkInfo ((arg1, arg2) => { }, sdkDir, ndkDir);
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs:			MonoAndroidHelper.AndroidSdk = new AndroidSdkInfo ((arg1, arg2) => {}, sdkDirectory, ndkDirectory);
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/JarContentBuilder.cs:						throw new Exception ($"AndroidSdkInfo {level}: {value}");
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/JarContentBuilder.cs:						Console.WriteLine ($"AndroidSdkInfo {level}: {value}");
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Android/JarContentBuilder.cs:			var androidSdk = new AndroidSdkInfo (logger, androidSdkPath: sdkPath, androidNdkPath: ndkPath);
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Utilities/DexUtils.cs:			var androidSdk = new AndroidSdkInfo ((l, m) => {
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs:		public static AndroidSdkInfo    AndroidSdk;
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs:			AndroidSdk  = new AndroidSdkInfo (logger, sdkPath, ndkPath, javaPath);

None of which use tools.

MonoAndroidHelper.cs requires more checking, as MonoAndroidHelper.AndroidSdk is a public field, so:

% git grep '\<AndroidSdk\>' src/Xamarin.Android.Build.Tasks | grep '\.cs:'
src/Xamarin.Android.Build.Tasks/Tasks/JarToXml.cs:			var jarpath = Path.Combine (MonoAndroidHelper.AndroidSdk.TryGetPlatformDirectoryFromApiLevel (AndroidApiLevel, MonoAndroidHelper.SupportedVersions), "android.jar");
src/Xamarin.Android.Build.Tasks/Tasks/Legacy/ResolveAndroidTooling.cs:					var apiPlatformDir = MonoAndroidHelper.AndroidSdk.TryGetPlatformDirectoryFromApiLevel (id, MonoAndroidHelper.SupportedVersions);
src/Xamarin.Android.Build.Tasks/Tasks/NdkUtils.cs:			string toolchainDir = Path.Combine (androidNdkPath, "toolchains", "llvm", "prebuilt", MonoAndroidHelper.AndroidSdk.AndroidNdkHostPlatform);
src/Xamarin.Android.Build.Tasks/Tasks/NdkUtilsOld.cs:				string path = Path.Combine (platbase, "prebuilt", MonoAndroidHelper.AndroidSdk.AndroidNdkHostPlatform, "bin", GetNdkToolchainPrefix (arch) + tool + extension);
src/Xamarin.Android.Build.Tasks/Tasks/NdkUtilsOld.cs:				string path = Path.Combine (androidNdkPath, "prebuilt", MonoAndroidHelper.AndroidSdk.AndroidNdkHostPlatform, "bin", tool);
src/Xamarin.Android.Build.Tasks/Tasks/ResolveAndroidTooling.cs:			foreach (var dir in MonoAndroidHelper.AndroidSdk.GetBuildToolsPaths (AndroidSdkBuildToolsVersion)) {
src/Xamarin.Android.Build.Tasks/Tasks/ResolveSdksTask.cs:			AndroidNdkPath = MonoAndroidHelper.AndroidSdk.AndroidNdkPath;
src/Xamarin.Android.Build.Tasks/Tasks/ResolveSdksTask.cs:			AndroidSdkPath = MonoAndroidHelper.AndroidSdk.AndroidSdkPath;
src/Xamarin.Android.Build.Tasks/Tasks/ResolveSdksTask.cs:			JavaSdkPath    = MonoAndroidHelper.AndroidSdk.JavaSdkPath;
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/NdkUtilTests.cs:				MonoAndroidHelper.AndroidSdk = new AndroidSdkInfo ((arg1, arg2) => { }, sdkDir, ndkDir);
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs:			MonoAndroidHelper.AndroidSdk = new AndroidSdkInfo ((arg1, arg2) => {}, sdkDirectory, ndkDirectory);
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs:		public static AndroidSdkInfo    AndroidSdk;
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs:			AndroidSdk  = new AndroidSdkInfo (logger, sdkPath, ndkPath, javaPath);
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs:			var platformPath = MonoAndroidHelper.AndroidSdk.TryGetPlatformDirectoryFromApiLevel (platform, MonoAndroidHelper.SupportedVersions);
src/Xamarin.Android.Build.Tasks/Utilities/MonoAndroidHelper.cs:				var expectedPath = MonoAndroidHelper.AndroidSdk.GetPlatformDirectoryFromId (platform);

which likewise doesn't use the tools directory.

Consequently, this change doesn't impact anything within this repo.

Instead, it's for consistency with other repos which may use this new method, such as the IDEs.

Then we can ponder the OpenJDK detection commit, but that's of limited utility until PR #4567 lands, at which point PR #4567 should contain any needed release notes.

Comment on lines +3 to +5
* [GitHub PR 79](https://github.com/xamarin/xamarin-android-tools/commit/f473ff9):
May address an `ArgumentException` observed on Windows machine when the
Registry contains an invalid path to the Android SDK.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on if it would it be OK to mention the Developer Community item? I was sneaky and added a draft release note idea for that xamarin-android-tools PR in a post-merge comment there:

#### Application and library build and deployment

  * [Developer Community](https://developercommunity.visualstudio.com/content/problem/883179/ilegal-characters-in-path-after-fresh-install.html):
    *System.ArgumentException: Illegal characters in path* could prevent
    successful automatic detection of the Android SDK location during builds in
    some cases if an `AndroidSdkDirectory` registry value was set for
    Xamarin.Android that contained unexpected characters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"OK"? Yes? I was kinda hoping to avoid mentioning it, because we're not entirely sure it'll fix the problem. (We think it will, but as repro'ing that issue is "weird," we could be wrong.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I'd say "yes, we should mention devcom issues." It's this case which makes me wary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, that's fair. Works for me. Thanks for the sanity check. Feel free to merge with the current wording.

Changes: dotnet/android-tools@12f52ac...f5fcb9f

  * dotnet/android-tools@f5fcb9f: [Xamarin.Android.Tools.AndroidSdk] Add support for cmdline-tools (dotnet#83)
  * dotnet/android-tools@f473ff9: [AndroidSdkWindows] Guard against exception checking registry (dotnet#79)
  * dotnet/android-tools@36d7fee: JetBrains OpenJDK 11 detection (dotnet#82)
@jonpryor jonpryor merged commit 314e48b into dotnet:master May 7, 2020
jonpryor added a commit that referenced this pull request May 8, 2020
Changes: dotnet/android-tools@12f52ac...23c4fe0

  * dotnet/android-tools@23c4fe0: [Xamarin.Android.Tools.AndroidSdk] Add support for cmdline-tools (#83)
  * dotnet/android-tools@cf9d325: [AndroidSdkWindows] Guard against exception checking registry (#79)
  * dotnet/android-tools@310c5cf: JetBrains OpenJDK 11 detection (#82)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants