Add manifest feed parsing for SDK/JDK download resolution#280
Add manifest feed parsing for SDK/JDK download resolution#280
Conversation
Suggestions from parallel workThanks for implementing the manifest feed parsing! I've been working on related tooling and have a few observations: 1. Our Manifest Package Models Include More MetadataWe include additional fields that are useful for consumers: public class ManifestPackage
{
public string Path { get; set; } // Package identifier
public string DisplayName { get; set; } // Human-readable name
public string Revision { get; set; } // Version
public string PackageType { get; set; } // cmdline-tools, platform-tools, etc.
public bool Obsolete { get; set; }
public string? LicenseId { get; set; }
public List<ManifestArchive> Archives { get; set; }
}
public class ManifestArchive
{
public Uri Url { get; set; }
public string Checksum { get; set; }
public string? ChecksumType { get; set; } // Important for SHA-256 vs SHA-1
public string HostOS { get; set; }
public string? HostArch { get; set; }
public uint HostBits { get; set; }
// Helper methods
public bool IsValidForCurrentSystem();
public ManifestArchive? GetBestArchiveForCurrentSystem();
}2. JDK-Specific Package ModelFor JDK packages, we have additional vendor info: public class JdkManifestPackage
{
public string VendorId { get; set; } // e.g., ""microsoft""
public string VendorDisplay { get; set; } // e.g., ""Microsoft""
public bool Preview { get; set; }
public int GetMajorVersion(); // Helper to extract major version
}These are used by the JDK installer to filter and select appropriate versions. Would love to collaborate on a unified approach! @copilot can you implement these changes |
|
@copilot you look ash the comment above? I tagged you to work on that. |
Implemented the enhanced models you suggested in commits 2ff0e9a and a29ff82: PackageInfo enhancements:
ArchiveInfo enhancements:
New JdkPackageInfo class:
ManifestFeed API additions:
Also extracted platform utilities to shared |
| <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets> | ||
| </PackageReference> | ||
| <PackageReference Include="Microsoft.SourceLink.GitHub" PrivateAssets="All" /> | ||
| <PackageReference Include="System.Text.Json" Version="8.0.5" /> |
There was a problem hiding this comment.
Yes, this dependency is required. The project targets netstandard2.0 which doesn't include System.Text.Json. The XamarinManifestParser uses JsonDocument and JsonElement from this package to parse JSON manifest feeds. Version 8.0.5 is used specifically because it includes patches for CVE-2024-43485 and CVE-2024-30105.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive manifest feed parsing infrastructure to enable SDK/JDK download resolution from Xamarin Android Manifest Feed and Google SDK Repository without hardcoding URLs. The implementation includes data models for packages and archives, parsers for both XML (Google) and JSON (Xamarin) formats, platform utilities, and extensive test coverage.
Changes:
- Adds
System.Text.Json8.0.5 dependency for JSON parsing with CVE patches - Implements
ManifestFeedAPI withLoadAsync(),LoadFromFile(),CacheAsync(),ResolvePackage(), and JDK-specific query methods - Creates
PackageInfo,ArchiveInfo, andJdkPackageInfomodels with platform/architecture filtering capabilities - Provides
GoogleRepositoryParserandXamarinManifestParserfor parsing both manifest formats - Includes
PlatformUtilsfor OS/architecture detection and normalization
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
Xamarin.Android.Tools.AndroidSdk.csproj |
Adds System.Text.Json 8.0.5 package reference |
ManifestFeed/ManifestFeed.cs |
Core API for loading, caching, and querying manifest feeds |
ManifestFeed/PackageInfo.cs |
Data models for packages, archives, and JDK packages with helper methods |
ManifestFeed/GoogleRepositoryParser.cs |
XML parser for Google SDK Repository format with namespace handling |
ManifestFeed/XamarinManifestParser.cs |
JSON parser for Xamarin Android Manifest Feed with flexible property mapping |
ManifestFeed/PlatformUtils.cs |
Shared utilities for OS/architecture detection and normalization |
tests/.../ManifestFeedTests.cs |
Comprehensive unit tests for all components |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Return first archive if no match (may be cross-platform) | ||
| return Archives.FirstOrDefault (a => string.IsNullOrEmpty (a.HostOs)); | ||
| } |
There was a problem hiding this comment.
The fallback logic in GetBestArchiveForCurrentSystem returns the first cross-platform archive (where HostOs is null/empty) if no OS-specific match is found. However, this could return an archive with a non-matching HostOs if there are no cross-platform archives in the list. Consider returning null instead if no valid match is found, or add a final fallback that returns the first archive only if there are truly cross-platform archives available.
| public async Task CacheAsync (string localPath, CancellationToken cancellationToken = default) | ||
| { | ||
| if (string.IsNullOrEmpty (FeedUrl)) | ||
| throw new InvalidOperationException ("Cannot cache a manifest that was not loaded from a URL."); | ||
|
|
||
| logger (TraceLevel.Info, $"Caching manifest feed to: {localPath}"); | ||
|
|
||
| try { | ||
| var directory = Path.GetDirectoryName (localPath); | ||
| if (!string.IsNullOrEmpty (directory) && !Directory.Exists (directory)) { | ||
| Directory.CreateDirectory (directory); | ||
| } | ||
|
|
||
| var response = await httpClient.GetAsync (FeedUrl, cancellationToken).ConfigureAwait (false); | ||
| response.EnsureSuccessStatusCode (); | ||
|
|
||
| var content = await response.Content.ReadAsStringAsync ().ConfigureAwait (false); | ||
| File.WriteAllText (localPath, content); | ||
|
|
||
| logger (TraceLevel.Info, "Manifest feed cached successfully."); | ||
| } catch (Exception ex) { | ||
| logger (TraceLevel.Error, $"Failed to cache manifest feed: {ex.Message}"); | ||
| throw; | ||
| } | ||
| } |
There was a problem hiding this comment.
There is no test coverage for the CacheAsync method. Given that this is a public API method with potential failure modes (file path vs URL validation, network errors, directory creation), it should have test coverage. Consider adding tests for CacheAsync including success scenarios and error cases.
| const string CommonNamespace = "http://schemas.android.com/repository/android/common/02"; | ||
| const string GenericNamespace = "http://schemas.android.com/repository/android/generic/02"; |
There was a problem hiding this comment.
The CommonNamespace and GenericNamespace constants are defined but never used. These namespace constants may be needed for parsing other elements in the Google SDK Repository format, or they should be removed if not needed. Consider removing them if they're not required, or add a comment explaining why they're declared for future use.
| const string CommonNamespace = "http://schemas.android.com/repository/android/common/02"; | |
| const string GenericNamespace = "http://schemas.android.com/repository/android/generic/02"; |
| public static JdkPackageInfo FromPackageInfo (PackageInfo package) | ||
| { | ||
| return new JdkPackageInfo { | ||
| Path = package.Path, | ||
| DisplayName = package.DisplayName, | ||
| Version = package.Version, | ||
| Archives = package.Archives, | ||
| License = package.License, | ||
| Description = package.Description, | ||
| PackageType = package.PackageType, | ||
| Obsolete = package.Obsolete | ||
| }; |
There was a problem hiding this comment.
The FromPackageInfo method performs a shallow copy of the Archives list, meaning both the original PackageInfo and the new JdkPackageInfo will share the same List instance and ArchiveInfo objects. Modifications to the Archives list in one will affect the other. Consider creating a new List and copying the ArchiveInfo instances, or document this behavior clearly if sharing is intentional.
| { | ||
| return Packages | ||
| .Where (p => p.Path != null && ( | ||
| p.Path.StartsWith ("jdk", StringComparison.OrdinalIgnoreCase) || |
There was a problem hiding this comment.
The filtering logic in GetJdkVersions and GetJdkPackages is inconsistent. GetJdkVersions uses StartsWith("jdk") which will miss packages like "microsoft-jdk-17", while GetJdkPackages uses IndexOf("jdk") which will match them. This inconsistency means the two methods will return different sets of packages for the same manifest. Consider making the filtering logic consistent between the two methods, likely by using IndexOf in both cases.
| p.Path.StartsWith ("jdk", StringComparison.OrdinalIgnoreCase) || | |
| p.Path.IndexOf ("jdk", StringComparison.OrdinalIgnoreCase) >= 0 || |
| if (string.IsNullOrEmpty (FeedUrl)) | ||
| throw new InvalidOperationException ("Cannot cache a manifest that was not loaded from a URL."); |
There was a problem hiding this comment.
The CacheAsync method allows caching even if FeedUrl is a local file path. Since LoadFromFile sets FeedUrl to the file path, calling CacheAsync on a manifest loaded from a local file would attempt to use that file path as a URL in httpClient.GetAsync, which would fail. Consider checking if FeedUrl is a URL (starts with http:// or https://) before allowing caching.
| if (string.IsNullOrEmpty (FeedUrl)) | |
| throw new InvalidOperationException ("Cannot cache a manifest that was not loaded from a URL."); | |
| if (string.IsNullOrEmpty (FeedUrl) || | |
| (!FeedUrl.StartsWith ("http://", StringComparison.OrdinalIgnoreCase) && | |
| !FeedUrl.StartsWith ("https://", StringComparison.OrdinalIgnoreCase))) | |
| throw new InvalidOperationException ("Cannot cache a manifest that was not loaded from an HTTP or HTTPS URL."); |
| if (string.IsNullOrEmpty (expectedSha1)) | ||
| return false; | ||
|
|
||
| try { | ||
| using (var sha1 = SHA1.Create ()) | ||
| using (var stream = File.OpenRead (filePath)) { | ||
| var hashBytes = sha1.ComputeHash (stream); | ||
| var actualSha1 = BitConverter.ToString (hashBytes).Replace ("-", "").ToLowerInvariant (); | ||
| return string.Equals (actualSha1, expectedSha1.ToLowerInvariant (), StringComparison.Ordinal); | ||
| } | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The VerifyChecksum method only supports SHA-1 verification (as indicated by the method name parameter and implementation), but ArchiveInfo supports both SHA-1 and SHA-256 checksums via the ChecksumType property. The method should either be enhanced to support both checksum types based on the ChecksumType, or the documentation should clearly state it only supports SHA-1. Consider adding a checksumType parameter or overload to support SHA-256 verification.
| if (string.IsNullOrEmpty (expectedSha1)) | |
| return false; | |
| try { | |
| using (var sha1 = SHA1.Create ()) | |
| using (var stream = File.OpenRead (filePath)) { | |
| var hashBytes = sha1.ComputeHash (stream); | |
| var actualSha1 = BitConverter.ToString (hashBytes).Replace ("-", "").ToLowerInvariant (); | |
| return string.Equals (actualSha1, expectedSha1.ToLowerInvariant (), StringComparison.Ordinal); | |
| } | |
| } catch { | |
| return false; | |
| } | |
| } | |
| // Preserve existing behavior: SHA-1 verification. | |
| return VerifyChecksum (filePath, expectedSha1, "SHA-1"); | |
| } | |
| /// <summary> | |
| /// Verifies that a downloaded file matches the expected checksum using the specified algorithm. | |
| /// </summary> | |
| /// <param name="filePath">Path to the downloaded file.</param> | |
| /// <param name="expectedChecksum">Expected checksum in hexadecimal format.</param> | |
| /// <param name="checksumType"> | |
| /// The checksum algorithm to use (for example, "SHA-1" or "SHA-256"). | |
| /// Comparison is case-insensitive. | |
| /// </param> | |
| /// <returns>True if the checksum matches, false otherwise.</returns> | |
| public static bool VerifyChecksum (string filePath, string expectedChecksum, string checksumType) | |
| { | |
| if (string.IsNullOrEmpty (expectedChecksum) || string.IsNullOrEmpty (checksumType)) | |
| return false; | |
| try { | |
| using (var algorithm = CreateHashAlgorithm (checksumType)) | |
| { | |
| if (algorithm == null) | |
| return false; | |
| using (var stream = File.OpenRead (filePath)) { | |
| var hashBytes = algorithm.ComputeHash (stream); | |
| var actualChecksum = BitConverter.ToString (hashBytes).Replace ("-", "").ToLowerInvariant (); | |
| var expected = expectedChecksum.Replace ("-", "").ToLowerInvariant (); | |
| return string.Equals (actualChecksum, expected, StringComparison.Ordinal); | |
| } | |
| } | |
| } catch { | |
| return false; | |
| } | |
| } | |
| static HashAlgorithm? CreateHashAlgorithm (string checksumType) | |
| { | |
| switch (checksumType.ToUpperInvariant ()) { | |
| case "SHA1": | |
| case "SHA-1": | |
| return SHA1.Create (); | |
| case "SHA256": | |
| case "SHA-256": | |
| return SHA256.Create (); | |
| default: | |
| return null; | |
| } | |
| } |
| try { | ||
| var response = await httpClient.GetAsync (feedUrl, cancellationToken).ConfigureAwait (false); | ||
| response.EnsureSuccessStatusCode (); | ||
|
|
There was a problem hiding this comment.
The ReadAsStringAsync call does not pass the cancellationToken parameter. In .NET Standard 2.0, ReadAsStringAsync does not accept a CancellationToken, but the cancellation token should still be honored by checking for cancellation before this call. Consider checking cancellationToken.ThrowIfCancellationRequested() before calling ReadAsStringAsync, or note that cancellation is only effective for the GetAsync call.
| cancellationToken.ThrowIfCancellationRequested (); |
|
|
||
| var response = await httpClient.GetAsync (FeedUrl, cancellationToken).ConfigureAwait (false); | ||
| response.EnsureSuccessStatusCode (); | ||
|
|
There was a problem hiding this comment.
The ReadAsStringAsync call does not pass the cancellationToken parameter. In .NET Standard 2.0, ReadAsStringAsync does not accept a CancellationToken. Consider checking cancellationToken.ThrowIfCancellationRequested() before calling ReadAsStringAsync to respect cancellation requests.
| cancellationToken.ThrowIfCancellationRequested (); |
| if (string.IsNullOrEmpty (os)) | ||
| return os; | ||
|
|
||
| // Normalize OS names to match manifest conventions | ||
| switch (os.ToLowerInvariant ()) { | ||
| case "windows": | ||
| case "win": | ||
| return "windows"; | ||
| case "macosx": | ||
| case "macos": | ||
| case "darwin": | ||
| case "osx": | ||
| return "macosx"; | ||
| case "linux": | ||
| return "linux"; | ||
| default: | ||
| return os.ToLowerInvariant (); | ||
| } |
There was a problem hiding this comment.
The NormalizeOsName method in ManifestFeed duplicates the same logic in PlatformUtils.NormalizeOsName. This violates the DRY principle and creates maintenance overhead. The ManifestFeed.NormalizeOsName should call PlatformUtils.NormalizeOsName instead of duplicating the logic.
| if (string.IsNullOrEmpty (os)) | |
| return os; | |
| // Normalize OS names to match manifest conventions | |
| switch (os.ToLowerInvariant ()) { | |
| case "windows": | |
| case "win": | |
| return "windows"; | |
| case "macosx": | |
| case "macos": | |
| case "darwin": | |
| case "osx": | |
| return "macosx"; | |
| case "linux": | |
| return "linux"; | |
| default: | |
| return os.ToLowerInvariant (); | |
| } | |
| return PlatformUtils.NormalizeOsName (os); |
- Add PackageInfo and ArchiveInfo data models - Implement ManifestFeed class with LoadAsync, LoadFromFile, and CacheAsync methods - Add GoogleRepositoryParser for parsing Google SDK repository XML - Add XamarinManifestParser for parsing Xamarin Android Manifest Feed JSON - Support package resolution by OS and architecture - Implement SHA-1 checksum verification - Add comprehensive unit tests (11/12 passing, 1 ignored for network access) Co-authored-by: rmarinho <1235097+rmarinho@users.noreply.github.com>
…elper methods Co-authored-by: rmarinho <1235097+rmarinho@users.noreply.github.com>
Co-authored-by: rmarinho <1235097+rmarinho@users.noreply.github.com>
Use 'is null' and 'is not null' instead of '== null' and '!= null' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rmarinho
force-pushed
the
copilot/add-manifest-feed-parsing
branch
from
c832c20 to
fda2fad
Compare
Adds infrastructure to resolve SDK/JDK download URLs and SHA-1 checksums from Xamarin Android Manifest Feed and Google SDK Repository without hardcoding. Enables manifest-driven downloads with offline/CI caching for downstream tools.
Components
PackageInfo,ArchiveInfo, andJdkPackageInforepresent packages with platform-specific archives (URL, checksum, size, OS, arch)LoadAsync(),LoadFromFile(),CacheAsync(),ResolvePackage(),GetJdkVersions(),GetJdkPackages(),GetJdkPackagesByVendor(),GetJdkPackagesByMajorVersion(), andVerifyChecksum()Enhanced Models
PackageInfo enhancements:
PackageTypeproperty for package categorization (cmdline-tools, platform-tools, etc.)ObsoleteflagGetBestArchiveForCurrentSystem()helper methodArchiveInfo enhancements:
ChecksumandChecksumTypeproperties (supporting both SHA-1 and SHA-256)HostBitsproperty (32 or 64)IsValidForCurrentSystem()helper methodNew JdkPackageInfo class:
VendorIdandVendorDisplayfor vendor identification (Microsoft, Eclipse Adoptium, Azul, Oracle, Amazon Corretto)Previewflag for early-access releasesGetMajorVersion()helper methodParseVendorFromPath()for automatic vendor detectionUsage
Dependencies
Adds
System.Text.Json8.0.5 for JSON parsing in netstandard2.0 (patched version for CVE-2024-43485, CVE-2024-30105).Downstream enablement
Prerequisite for JDK installation (#270) and SDK bootstrap (#271) features.
Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.