Bump external/xamarin-android-tools and fix NRT warnings#11247
Draft
jonathanpeppers wants to merge 6 commits intomainfrom
Draft
Bump external/xamarin-android-tools and fix NRT warnings#11247jonathanpeppers wants to merge 6 commits intomainfrom
jonathanpeppers wants to merge 6 commits intomainfrom
Conversation
Bump the submodule from 2fd1240 to ed6aab1 which enables nullable reference types in BaseTasks. Fix all resulting CS8600/CS8602/CS8604 warnings in callers without using the null-forgiving operator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the external/xamarin-android-tools dependency to a commit that enables nullable reference types (NRT) in BaseTasks, and then adjusts the dotnet/android codebase to address the resulting NRT warnings in build tasks/utilities.
Changes:
- Bump
external/xamarin-android-tools(enables NRT in BaseTasks) and update consumers accordingly. - Add null-handling patterns (null checks/throws, null-coalescing for logging args, null-conditional debug logging) across MSBuild tasks and utilities.
- Tighten some internal build-pipeline assumptions by failing fast when required registered task objects are missing.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Utilities/RtxtParser.cs | Make debug logging tolerant of nullable logger under NRT. |
| src/Xamarin.Android.Build.Tasks/Utilities/FileResourceParser.cs | Adjust debug logging calls for nullable Log under NRT. |
| src/Xamarin.Android.Build.Tasks/Tasks/RewriteMarshalMethods.cs | Add null check for required registered NativeCodeGenState. |
| src/Xamarin.Android.Build.Tasks/Tasks/ResolveAndroidTooling.cs | Coalesce nullable strings when used as message args for coded errors. |
| src/Xamarin.Android.Build.Tasks/Tasks/Legacy/ValidateJavaVersion.cs | Coalesce nullable BuildTools version when used as message arg. |
| src/Xamarin.Android.Build.Tasks/Tasks/Legacy/ResolveAndroidTooling.cs | Coalesce nullable version strings when used as warning args. |
| src/Xamarin.Android.Build.Tasks/Tasks/GetJavaPlatformJar.cs | NRT-driven adjustments around manifest logging / message args. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateTypeMappings.cs | Add null check for required registered NativeCodeGenState. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateMainAndroidManifest.cs | Add null checks for required registered objects; avoid null output path on cleanup. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateLayoutBindings.cs | Make registered widget collection nullable and guard its use. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateAdditionalProviderSources.cs | Add null check for required registered NativeCodeGenStateCollection. |
| src/Xamarin.Android.Build.Tasks/Tasks/ConvertCustomView.cs | Coalesce nullable resource directory metadata when logging coded diagnostics. |
| src/Xamarin.Android.Build.Tasks/Tasks/CalculateLayoutCodeBehind.cs | Remove unnecessary null-conditional on non-null widgets. |
| src/Xamarin.Android.Build.Tasks/Tasks/BuildAppBundle.cs | Change debug log formatting while addressing NRT warnings (but see review comment). |
| src/Xamarin.Android.Build.Tasks/Tasks/AndroidWarning.cs | Coalesce nullable resource string / format args under NRT. |
| src/Xamarin.Android.Build.Tasks/Tasks/AndroidError.cs | Coalesce nullable resource string / format args under NRT. |
| src/Xamarin.Android.Build.Tasks/Tasks/AndroidDotnetToolTask.cs | Treat cached mono path as nullable under NRT. |
| src/Xamarin.Android.Build.Tasks/Tasks/Aapt2Compile.cs | Ensure non-null enumerable passed to async processing under NRT. |
Comments suppressed due to low confidence (2)
src/Xamarin.Android.Build.Tasks/Tasks/GenerateLayoutBindings.cs:213
- After the
widgets is null || widgets.Count == 0guard,widgetsis known to be non-null, but later the code still uses the null-forgiving operator (widgets!) when callingGenerateSource. Since this PR aims to fix NRT warnings without!, please refactor to avoid the null-forgiving operator here (e.g., by passing the non-nullwidgetsvariable directly after the guard).
string collectionKey;
if (!GetRequiredMetadata (item, CalculateLayoutCodeBehind.WidgetCollectionKeyMetadata, out collectionKey))
return;
ICollection<LayoutWidget>? widgets = BuildEngine4.GetRegisteredTaskObjectAssemblyLocal<ICollection<LayoutWidget>> (ProjectSpecificTaskObjectKey (collectionKey), RegisteredTaskObjectLifetime.Build);
if (widgets is null || widgets.Count == 0) {
string inputPaths = String.Join ("; ", resourceItems.Select (i => i.ItemSpec));
LogCodedWarning ("XA4222", Properties.Resources.XA4222, inputPaths);
return;
}
src/Xamarin.Android.Build.Tasks/Utilities/FileResourceParser.cs:76
ResourceParser.Logis nullable, but this method now mixesLog?.LogDebugMessage(...)with later unconditionalLog.LogDebugMessage(...)calls in the same method body. IfLogis required for this parser, consider validatingLogis non-null once at the start ofParse()(and then usingLog.consistently) instead of sprinkling null-conditional calls (which can also leave CS8602 warnings on the remainingLog.usages).
public IList<R> Parse (string resourceDirectory, IEnumerable<string> additionalResourceDirectories, IEnumerable<string> aarLibraries, Dictionary<string, string> resourceMap)
{
Log?.LogDebugMessage ($"Parsing Directory {resourceDirectory}");
publicXml = LoadPublicXml ();
var result = new List<R> ();
Dictionary<string, ICollection<R>> resources = new Dictionary<string, ICollection<R>> ();
foreach (var knownType in RtxtParser.knownTypes) {
if (knownType == "styleable") {
resources.Add (knownType, new List<R> ());
continue;
}
resources.Add (knownType, new SortedSet<R> (new RComparer ()));
}
foreach (var dir in Directory.EnumerateDirectories (resourceDirectory, "*", SearchOption.TopDirectoryOnly)) {
foreach (var file in Directory.EnumerateFiles (dir, "*.*", SearchOption.AllDirectories)) {
ProcessResourceFile (file, resources);
}
}
foreach (var dir in additionalResourceDirectories ?? []) {
Log.LogDebugMessage ($"Processing Directory {dir}");
if (Directory.Exists (dir)) {
foreach (var file in Directory.EnumerateFiles (dir, "*.*", SearchOption.AllDirectories)) {
ProcessResourceFile (file, resources);
}
} else {
Log.LogDebugMessage ($"Skipping non-existent directory: {dir}");
}
Use `required` on ResourceParser.Log so it must be set via object initializer, eliminating the need for Log?. Pass log and map as parameters to RtxtParser.ProcessRtxtFile instead of storing them in nullable fields, removing the null-conditional calls and the runtime null check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The `required` keyword needs CompilerFeatureRequiredAttribute which is only available in net7.0+, not netstandard2.0. Use a constructor parameter for ResourceParser.Log instead, and chain it through FileResourceParser, JavaResourceParser, and ManagedResourceParser. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move Parser field initialization into the constructor that takes TaskLoggingHelper, removing the old parameterless constructor. Also remove Log! usages now that Log is guaranteed non-null. Add missing using System in RewriteMarshalMethods. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bumps external/xamarin-android-tools from
2fd1240toed6aab1, which enables nullable reference types inBaseTasks.Fixes all resulting CS8600/CS8602/CS8604 NRT warnings across 18 files without using
!(null-forgiving operator).Key changes in xamarin-android-tools
ed6aab1[BaseTasks] Enable nullable reference types ([AndroidClientHandler] Prevent orphaned thread in DoProcessRequest #344)4fd9ae0[Tools.AndroidSdk] Remove unusedkey_namevariables (use ant version 1.9.8 as 1.9.7 is no longer available at ibiblio #345)3bfeba6[copilot] Add/reviewGitHub agentic workflow (Add support for Round Icon #346)NRT fix patterns used
value ?? ""for nullable strings passed toparams object[]GetRegisteredTaskObjectAssemblyLocal/UnregisterTaskObjectAssemblyLocalresults that must existLog?.LogDebugMessage(...)for nullableTaskLoggingHelper?.: e.g.min_sdk?.Value->min_sdk.Valueinsideif (min_sdk != null)blocksstring? mono = ...for nullable return typesSupersedes #11243