Add JDK installation support (Microsoft OpenJDK)

[rmarinho]

Summary

Adds JDK installation support to Xamarin.Android.Tools.AndroidSdk using the Microsoft Build of OpenJDK (https://www.microsoft.com/openjdk).

Closes #270

API Surface

public class JdkInstaller : IDisposable
{
    // Construction — accepts optional HttpClient for VS proxy/auth and testing
    public JdkInstaller(HttpClient? httpClient = null, Action<TraceLevel, string>? logger = null);
    
    // Configuration
    public static int RecommendedMajorVersion { get; }            // 17
    public static IReadOnlyList<int> SupportedVersions { get; }   // [17, 21]
    
    // Core APIs
    public Task<IReadOnlyList<JdkVersionInfo>> DiscoverAsync(CancellationToken ct = default);
    public Task InstallAsync(int majorVersion, string targetPath, IProgress<JdkInstallProgress>? progress = null, CancellationToken ct = default);
    public bool IsValid(string jdkPath);
    
    // Utility
    public bool IsTargetPathWritable(string targetPath);
    public bool Remove(string jdkPath);
    public void Dispose();
}

Key Design Decisions

• Microsoft OpenJDK: Downloads from https://aka.ms/download-jdk with SHA-256 checksum verification
• HttpClient injection: Constructor accepts optional HttpClient for Visual Studio proxy/auth support (e.g. IVsHttpClientFactory) and unit testing with mocks
• IDisposable: Properly disposes owned HttpClient when the installer is disposed
• netstandard2.0 compatible: All code compiles on both netstandard2.0 and net9.0 so it can run inside Visual Studio
• CancellationToken support: All async operations properly propagate cancellation tokens to HTTP calls using the GetAsync + ReadAsStringAsync pattern for netstandard2.0 compatibility
• Logging: Uses the existing Action<TraceLevel, string> pattern from the codebase (matching JdkInfo, AndroidSdkInfo)
• Path validation: IsTargetPathWritable() normalizes paths with Path.GetFullPath() before checking, rejects Program Files paths on Windows, and verifies write access before downloading
• Safe directory replacement: Uses move-to-temp + move-new-in-place pattern to avoid the Directory.Delete/Move race condition on Windows
• Validation cleanup: If IsValid() fails after extraction, the invalid installation is automatically cleaned up
• Progress reporting: IProgress<JdkInstallProgress> with download percentage and phase tracking

Tests

20 unit tests covering:

• IsValid (null, empty, non-existent, faux JDK, system JDK)
• DiscoverAsync (versions returned, expected major versions, cancellation, resolved URL)
• InstallAsync (invalid version, null path, progress reporting with real download)
• IsTargetPathWritable (temp dir, null/empty, restricted paths)
• Remove (non-existent, existing directory)
• Constructor (default, custom HttpClient, custom logger)
• RecommendedMajorVersion and SupportedVersions properties

Review Feedback Addressed

• ✅ Implement IDisposable to properly dispose owned HttpClient
• ✅ Use GetAsync + ReadAsStringAsync pattern for cancellation token support
• ✅ Normalize paths with Path.GetFullPath() before Program Files check
• ✅ Clean up target directory if validation fails after extraction

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Pull request overview

This PR adds JDK installation support to Xamarin.Android.Tools.AndroidSdk by introducing a new JdkInstaller static class that enables automatic discovery and installation of JDK 17 and 21 from the Eclipse Adoptium (Temurin) API. The implementation provides three main APIs: DiscoverAsync() for querying available JDK versions, InstallAsync() for downloading and installing JDKs with progress reporting and SHA-256 checksum verification, and IsValid() for validating existing installations. The code is conditionally compiled for .NET 9.0+ only (excluded from netstandard2.0).

Changes:

• Adds JdkInstaller class with discovery, installation, and validation capabilities for JDKs
• Adds supporting types (JdkVersionInfo, JdkInstallProgress, JdkInstallPhase) for metadata and progress reporting
• Includes System.Text.Json package reference for .NET 9.0+ target framework with comprehensive test coverage

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs	New file implementing JDK discovery, download, extraction, and validation using Adoptium API
src/Xamarin.Android.Tools.AndroidSdk/Xamarin.Android.Tools.AndroidSdk.csproj	Adds conditional System.Text.Json package reference for non-netstandard2.0 targets
Directory.Build.targets	Pins System.Text.Json to version 9.0.1
tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInstallerTests.cs	New test file with 12 unit tests covering validation, discovery, and installation scenarios

Comments suppressed due to low confidence (12)

tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInstallerTests.cs:158

• The CI detection only checks for the 'CI' environment variable, but different CI systems use different variable names (e.g., GITHUB_ACTIONS, GITLAB_CI, TF_BUILD, JENKINS_HOME, CIRCLECI). Consider checking for multiple common CI environment variables or using a more robust CI detection method to ensure the test is properly skipped across different CI environments.

			if (Environment.GetEnvironmentVariable ("CI") != null) {
				Assert.Ignore ("Skipping download test in CI environment.");
				return;

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:160

• Using ArgumentNullException for an empty string is semantically incorrect. ArgumentNullException should only be thrown when the parameter is null. For empty strings, ArgumentException should be thrown instead. Consider changing to: 'if (string.IsNullOrEmpty (targetPath)) throw new ArgumentException ("Target path cannot be null or empty.", nameof (targetPath));' or separate the checks into 'if (targetPath == null) throw new ArgumentNullException (nameof (targetPath)); if (string.IsNullOrEmpty (targetPath)) throw new ArgumentException ("Target path cannot be empty.", nameof (targetPath));'

			if (string.IsNullOrEmpty (targetPath))
				throw new ArgumentNullException (nameof (targetPath));

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:244

• The catch block silently swallows all exceptions and returns null, which may hide API contract changes or JSON structure issues that developers should be aware of. Consider catching more specific exception types (KeyNotFoundException, InvalidOperationException for JSON operations) and logging unexpected exceptions to help with debugging API changes.

			catch (Exception) {
				return null;
			}

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:384

• The architecture detection defaults all non-Arm64 architectures to 'x64', which would incorrectly handle architectures like X86 (32-bit) or other architectures. While .NET 9 primarily targets x64 and Arm64, consider explicitly checking for Architecture.X64 and throwing an exception for unsupported architectures to provide a clearer error message: 'Architecture.X64 => "x64", Architecture.Arm64 => "aarch64", _ => throw new PlatformNotSupportedException(...)'

			return RuntimeInformation.OSArchitecture switch {
				Architecture.Arm64 => "aarch64",
				_ => "x64",
			};

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:296

• The progress message uses integer division for MB calculation, which will display "0 MB / 0 MB" at the start of downloads and may be confusing. Consider using floating-point division and formatting to one decimal place: '$"Downloaded {totalRead / (1024.0 * 1024.0):F1} MB / {totalBytes / (1024.0 * 1024.0):F1} MB"' for more accurate progress reporting.

					progress.Report (new JdkInstallProgress (JdkInstallPhase.Downloading, pct, $"Downloaded {totalRead / (1024 * 1024)} MB / {totalBytes / (1024 * 1024)} MB"));

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:51

• The JdkVersionInfo constructor does not validate its parameters. Consider adding null checks for the string parameters (versionString, releaseName, downloadUrl) and validation that majorVersion is positive and size is non-negative to ensure the object is constructed in a valid state.

		public JdkVersionInfo (int majorVersion, string versionString, string releaseName, string downloadUrl, long size, string? checksum)
		{
			MajorVersion = majorVersion;
			VersionString = versionString;
			ReleaseName = releaseName;
			DownloadUrl = downloadUrl;
			Size = size;
			Checksum = checksum;
		}

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:330

• The logic assumes that if there's exactly one directory in the extracted archive, it's the JDK root. However, if the archive contains both a directory and files at the top level, this will incorrectly use tempExtractPath as jdkRoot. Consider checking for the expected JDK structure (e.g., presence of 'bin' directory) to identify the correct root, or document the assumption that JDK archives will only have a single top-level directory.

				var extractedDirs = Directory.GetDirectories (tempExtractPath);
				var jdkRoot = extractedDirs.Length == 1 ? extractedDirs [0] : tempExtractPath;

tests/Xamarin.Android.Tools.AndroidSdk-Tests/JdkInstallerTests.cs:149

• There's no test case for InstallAsync with an empty string path, but the implementation checks for both null and empty (line 159 in JdkInstaller.cs). Consider adding a test: 'InstallAsync_EmptyPath_Throws' to ensure complete test coverage of the argument validation.

		[Test]
		public void InstallAsync_NullPath_Throws ()
		{
			Assert.ThrowsAsync<ArgumentNullException> (
				async () => await JdkInstaller.InstallAsync (17, null!));
		}

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:153

• The XML documentation at line 151 lists ArgumentException, InvalidOperationException, and IOException, but does not document ArgumentNullException which is thrown at line 160 when targetPath is null or empty. Add an exception tag: '<exception cref="ArgumentNullException">If targetPath is null or empty.</exception>'

		/// <exception cref="ArgumentException">If the version is not supported.</exception>
		/// <exception cref="InvalidOperationException">If no JDK is available for the current platform or checksum verification fails.</exception>
		/// <exception cref="IOException">If extraction fails.</exception>

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:176

• The installation process does not check available disk space before downloading or extracting the JDK. A typical JDK archive is 100-200 MB compressed but expands to 300-400 MB. Consider checking available disk space on the target drive before starting the download to provide a better error message and avoid wasting time on a download that will fail during extraction.

				// Download
				progress?.Report (new JdkInstallProgress (JdkInstallPhase.Downloading, 0, $"Downloading JDK {majorVersion}..."));
				await DownloadFileAsync (versionInfo.DownloadUrl, tempArchivePath, versionInfo.Size, progress, cancellationToken).ConfigureAwait (false);

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:251

• The variable 'ext' is computed but never used in the fallback URL construction. The line 'var ext = GetArchiveExtension ().TrimStart ('.');' appears to be dead code and should be removed, or if the archive extension is needed for some purpose, it should be utilized in the URL or documented why it's retrieved.

			var ext = GetArchiveExtension ().TrimStart ('.');

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:370

• The implementation shells out to the 'tar' command instead of using a managed library. While the paths are internally generated and not directly user-controlled, using Process execution adds external dependencies and potential portability issues. Consider using a managed tar/gzip library (like SharpZipLib or a built-in solution) for better cross-platform reliability and to eliminate the external 'tar' command dependency.

		static async Task ExtractTarGzAsync (string archivePath, string destinationPath, CancellationToken cancellationToken)
		{
			var psi = new ProcessStartInfo {
				FileName = "tar",
				Arguments = $"-xzf \"{archivePath}\" -C \"{destinationPath}\"",
				UseShellExecute = false,
				CreateNoWindow = true,
			};

			var stdout = new StringWriter ();
			var stderr = new StringWriter ();
			var exitCode = await ProcessUtils.StartProcess (psi, stdout: stdout, stderr: stderr, cancellationToken).ConfigureAwait (false);

			if (exitCode != 0)
				throw new IOException ($"Failed to extract archive '{archivePath}': {stderr}");
		}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.

Comments suppressed due to low confidence (2)

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:392

• The ReadAsStreamAsync call does not pass the cancellationToken, which means the stream reading operation cannot be cancelled. While the actual download loop does check the cancellation token during Read and Write operations, the initial stream acquisition could hang without being cancellable. Consider using the overload that accepts a CancellationToken if available for the target framework, or document this limitation.

			using var contentStream = await response.Content.ReadAsStreamAsync ().ConfigureAwait (false);

src/Xamarin.Android.Tools.AndroidSdk/JdkInstaller.cs:310

• The IsTargetPathWritable method creates the target directory as part of its validation check, which means that if InstallAsync subsequently fails (e.g., download failure, checksum mismatch, extraction error), an empty directory may be left at the target path. This could confuse users who see the directory but find it empty or invalid. Consider either cleaning up the directory on failure, or deferring directory creation until the extraction phase when you know the installation will proceed.

			try {
				Directory.CreateDirectory (targetPath);
				var testFile = Path.Combine (targetPath, $".write-test-{Guid.NewGuid ()}");
				using (File.Create (testFile, 1, FileOptions.DeleteOnClose)) { }
				return true;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The ZipFile.ExtractToDirectory method does not accept a cancellation token, which means the extraction operation on Windows cannot be cancelled even when the cancellationToken parameter is passed. This could cause the method to continue extracting a large archive even after cancellation is requested, wasting resources and time. Consider implementing custom extraction logic that checks the cancellation token periodically, or documenting this limitation for Windows users.

[rmarinho]

⚠️ Known limitation - ZipFile.ExtractToDirectory in .NET Standard 2.0 does not support cancellation tokens. This is a .NET API limitation. Options:

1. Document this limitation (Windows only - tar supports cancellation)
2. Implement manual entry-by-entry extraction with cancellation checks (adds complexity)

Leaving unresolved pending decision on approach.

[Copilot]

The GetStringAsync method does not support cancellation tokens, which means the checksum fetch operation cannot be cancelled even when the cancellationToken parameter is passed to DiscoverAsync. This could cause the method to hang or continue executing even after cancellation is requested. Consider using httpClient.GetAsync with cancellationToken and then reading the response content, or use an overload of GetStringAsync that accepts a CancellationToken if targeting a framework version that supports it.

Suggested change

	var checksumContent = await httpClient.GetStringAsync (info.ChecksumUrl).ConfigureAwait (false);
	using var checksumResponse = await httpClient.GetAsync (info.ChecksumUrl, cancellationToken).ConfigureAwait (false);
	checksumResponse.EnsureSuccessStatusCode ();
	var checksumContent = await checksumResponse.Content.ReadAsStringAsync ().ConfigureAwait (false);

[rmarinho]

⚠️ Still needs fix - Line 160 still uses GetStringAsync without cancellation token. Should use GetAsync pattern:

using var response = await httpClient.GetAsync(info.ChecksumUrl, cancellationToken).ConfigureAwait(false);
response.EnsureSuccessStatusCode();
var checksumContent = await response.Content.ReadAsStringAsync().ConfigureAwait(false);

[Copilot]

The GetStringAsync method does not support cancellation tokens. This means the checksum fetch operation cannot be cancelled when cancellationToken is passed to InstallAsync, potentially causing unnecessary network activity or delays when installation is cancelled. Consider using httpClient.GetAsync with cancellationToken and then reading the response content, or use an overload of GetStringAsync that accepts a CancellationToken if available for the target framework.

Suggested change

	var checksumContent = await httpClient.GetStringAsync (versionInfo.ChecksumUrl).ConfigureAwait (false);
	using var response = await httpClient.GetAsync (versionInfo.ChecksumUrl, cancellationToken).ConfigureAwait (false);
	response.EnsureSuccessStatusCode ();
	var checksumContent = await response.Content.ReadAsStringAsync ().ConfigureAwait (false);

[rmarinho]

⚠️ Still needs fix - Line 207 still uses GetStringAsync without cancellation token. Should use the GetAsync pattern as suggested in the original comment.

[Copilot]

When ExtractArchiveAsync encounters a failure after moving the existing JDK to a backup path, the original JDK remains in the backup location and is not restored. This leaves the system in an inconsistent state where the targetPath does not contain a valid JDK, and the user must manually locate and restore the backup. Consider implementing rollback logic that restores the backup if extraction or validation fails, or at minimum document this behavior so users know to look for .old-* directories.

[rmarinho]

⚠️ Still needs fix - Lines 455-461 delete the backup immediately after move without implementing rollback on failure. If extraction or validation fails after the move, the original JDK is lost.

Should implement try-catch around the final Directory.Move with rollback logic that restores from backup if the operation fails.

[rmarinho]

Review Response: PR #274 (JdkInstaller)

Thank you for the detailed review! Here's my analysis of each suggestion:

✅ ACCEPTED

1. IDisposable for HttpClient (Accept)

The class owns HttpClient when ownsHttpClient=true but doesn't dispose it.

Agree. This is a valid concern. The class should implement IDisposable to properly manage the HttpClient lifecycle when it owns the instance.

2. CancellationToken in GetStringAsync (Accept with modification)

GetStringAsync doesn't support cancellation tokens.

Agree. Should use SendAsync + ReadAsStringAsync pattern for netstandard2.0 compatibility:

using var response = await httpClient.GetAsync(url, cancellationToken);
response.EnsureSuccessStatusCode();
var content = await response.Content.ReadAsStringAsync();

3. Path traversal in Program Files check (Accept)

Should normalize path before StartsWith check.

Agree. A path like C:\Program Files\..\Users\evil could bypass the check. Should normalize with Path.GetFullPath() first.

4. Validation failure cleanup (Accept)

If IsValid fails, targetPath may be left invalid.

Agree. Should clean up the target directory if validation fails to avoid leaving corrupted installations.

---

❌ REJECTED

5. tar command injection (Reject)

Using string interpolation for tar arguments.

Disagree. The paths are internally generated:

• archivePath comes from Path.GetTempPath() + GUID
• destinationPath comes from Path.GetTempPath() + GUID

No user input reaches these paths. Additionally, UseShellExecute = false disables shell expansion. The suggestion to use ArgumentList is valid for general safety, but not a security issue here.

6. JdkVersionInfo constructor validation (Reject)

Constructor doesn't validate parameters.

Disagree. This is an internal construction pattern - all instances are created by BuildVersionInfo() with controlled values. Adding validation adds overhead for no security benefit since the constructor isn't exposed to untrusted input.

7. ZipFile.ExtractToDirectory cancellation (Reject)

Can't cancel extraction on Windows.

Disagree on priority. Local disk extraction is fast (typically <30 seconds for a JDK). Adding manual entry-by-entry extraction with cancellation checks would add significant complexity for minimal benefit. The tar extraction on Unix IS cancellable via the process API.

8. Architecture defaulting to x64 (Reject)

Non-Arm64 defaults to x64, ignores x86.

Disagree. Microsoft OpenJDK only provides x64 and aarch64 builds. x86 builds don't exist. The _ => ""x64"" fallback is correct - x86 Windows can run x64 JDK via WoW64 emulation. Throwing for x86 would break legitimate use cases.

9. netstandard2.0 support (Already addressed)

Code needs to support netstandard for VS hosting.

The csproj already targets netstandard2.0. The APIs used are compatible. This was addressed per Jonathan's feedback.

[rmarinho]

Implemented Review Fixes

I've pushed commits addressing the accepted feedback:

Changes Made

1. IDisposable for HttpClient ✅

   • Added IDisposable implementation
   • Disposes httpClient when ownsHttpClient is true
   • Added disposed flag to prevent double-dispose

2. CancellationToken in GetStringAsync ✅

   • Changed from GetStringAsync() to GetAsync(url, cancellationToken) + ReadAsStringAsync()
   • Now properly cancellable on netstandard2.0

3. Path normalization ✅

   • Added Path.GetFullPath() before Program Files StartsWith check
   • Prevents path traversal bypass (e.g., C:\Program Files\..\Users)

4. Validation failure cleanup ✅

   • Added cleanup code to delete targetPath if IsValid() fails
   • Prevents leaving corrupted installations on disk

Build passes with 1 warning (existing nullable reference in AndroidVersion.cs).