Repository navigation
[AndroidSdk] Replace uname and remove the unused manifest file writer - #13457
simonrozsival wants to merge 7 commits into
Conversation
* OS.IsRunningOnMac: use OperatingSystem (net) / RuntimeInformation (netstandard2.0) platform checks instead of P/Invoking uname(3). All Apple platforms are matched, as uname reports "Darwin" on each. * FileUtil.SystemRename: use a LibraryImport'd rename(2) on .NET, keeping exact rename(2) and errno semantics; netstandard2.0 keeps the DllImport. File.Move(overwrite) was rejected: it falls back to non-atomic copy+delete on EXDEV, rejects directory sources, and throws UnauthorizedAccessException instead of errno IOExceptions. * Remove unused Xamarin.Android.Build.Tasks Utilities/FileUtil.cs and its rename P/Invoke. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
uname() reports the "Darwin" sysname on watchOS too, so the managed
replacement for the uname P/Invoke must treat watchOS as Mac on both the
.NET (OperatingSystem.IsWatchOS) and netstandard2.0
(OSPlatform.Create ("WATCHOS")) paths.
Add a host test comparing OS.IsMac/IsLinux/IsWindows with the original
uname()-based classification.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The runtime platform checks do not establish uname equivalence on unsupported platforms such as watchOS. Correct the comment without changing the detection logic. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The platform detection implementation now relies on runtime platform checks instead of calling uname(3). Remove the host-only comparison, which required a separate uname P/Invoke in the test suite. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
AndroidAppManifest.WriteToFile remains public and uses the native rename helper. Cover non-ASCII paths and both new and existing destinations so the source-generated and netstandard marshalling paths preserve this behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context: 6b8fb8c AndroidAppManifest.WriteToFile() is the only production caller of FileUtil.SystemRename(), and no in-tree callers use the file-writing method. The manifest type is still used by build tasks, so retain it while removing the file writer and its native rename implementation. This removes a shipped public API. Consumers can use Write(XmlWriter) and manage file creation and replacement themselves. Record the removal in the public API baselines and remove tests for the deleted operations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Removing the shipped public AndroidAppManifest.WriteToFile(string) API is binary-breaking and requires maintainer confirmation of downstream compatibility and release-policy acceptance.
No open issues 🎉
What changed in this PR
This PR simplifies Android SDK tooling by replacing native uname detection with managed platform checks and removing unused manifest-writing and rename code.
Changes:
- Uses runtime platform checks for Darwin classification, with watchOS equivalence explicitly unverified.
- Intentionally removes the shipped
AndroidAppManifest.WriteToFile(string)API;Write(XmlWriter)remains, with file creation and replacement left to callers. - Removes unused rename helpers, the duplicate build-task utility, and their tests. The chmod fallback and Windows interop remain unchanged.
| File | Description |
|---|---|
| tests/Xamarin.Android.Tools.AndroidSdk-Tests/SystemRenameTests.cs | Deletes tests for the removed rename helper. |
| tests/Xamarin.Android.Tools.AndroidSdk-Tests/FileUtilTests.cs | Removes rename-specific cases and an unused import. |
| src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt | Records the manifest writer API removal for netstandard2.0. |
| src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/net11.0/PublicAPI.Unshipped.txt | Records the manifest writer API removal for net11.0. |
| src/Xamarin.Android.Tools.AndroidSdk/PublicAPI/net10.0/PublicAPI.Unshipped.txt | Records the manifest writer API removal for net10.0. |
| src/Xamarin.Android.Tools.AndroidSdk/OS.cs | Replaces uname interop with managed Apple-platform checks. |
| src/Xamarin.Android.Tools.AndroidSdk/FileUtil.cs | Removes temporary-name and rename helpers, including native interop. |
| src/Xamarin.Android.Tools.AndroidSdk/AndroidAppManifest.cs | Removes WriteToFile, retaining Write(XmlWriter). |
| src/Xamarin.Android.Build.Tasks/Utilities/FileUtil.cs | Deletes the unused duplicate utility. |
Review effort: Balanced
|
OS classification microbenchmark — final candidate On macOS arm64 with the .NET 11 runtime, 40 counterbalanced cold-process pairs compared main
These measure |
|
/review |
|
✅ Android PR Reviewer completed successfully!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes — compatibility decision outstanding
Reviewed all nine changed files, their surrounding source, callers, and AndroidSdk target-framework configuration. 0 errors, 1 warning, 1 suggestion, posted inline. The warning asks for explicit downstream-compatibility/release acceptance of the intentional shipped WriteToFile(string) removal; absent that acceptance, preserve a compatibility entry point. The suggestion adds independent platform-classification coverage for both target-framework paths.
The managed Darwin checks remove the unmanaged allocation and uname call while retaining the existing Windows/non-Darwin classification on supported desktop hosts. No in-repository callers of the removed writer or rename helpers remain. The API-removal records are consistent across all three existing framework baselines, and the chmod fallback and Windows interop are unchanged. The description clearly distinguishes the API break and watchOS limitation; no additional correctness defect was identified in the remaining implementation.
CI: All 41 GitHub check runs report success, including the aggregate dotnet-android check for Azure build 1673052. Direct Azure build/timeline verification was blocked by CLI startup and authentication errors, so Azure status was not independently confirmed. No local builds or tests were run for this read-only review.
Generated by Android PR Reviewer for #13457 · copilot · gpt61 · 31.3 AIC · ⌖ 7.63 AIC · ⊞ 19.5K
Comment /review to run again
| @@ -1,4 +1,5 @@ | |||
| #nullable enable | |||
| *REMOVED*Xamarin.Android.Tools.AndroidAppManifest.WriteToFile(string! fileName) -> void | |||
There was a problem hiding this comment.
🤖 AndroidAppManifest.WriteToFile(string) will fail with MissingMethodException after upgrading. The description correctly acknowledges this, but Write(XmlWriter) is neither a binary-compatible replacement nor an equivalent file-replacement API. Unless the break is approved for this release, retain an obsolete compatibility entry point and preserve its replacement behavior. This applies to the net10.0 and net11.0 removals as well.
{Rule: Shared tooling API-shape changes require downstream binary-compatibility review.}
There was a problem hiding this comment.
The API removal remains intentional and was explicitly requested by the PR author after confirming there are no in-repository callers. A compatibility entry point would retain the file-writing/rename behavior this change is removing. I cannot independently confirm downstream compatibility or release-policy acceptance, so I am leaving this thread open for maintainer review of that release decision.
Auto-replied by the GitHub Copilot app
Add independent runtime-platform assertions for the OS classification and native library format. Run the test with both the netstandard2.0 and modern .NET AndroidSdk assemblies. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
OS.IsMaccurrently callsunameto identify Darwin. Use managed runtime platform checks instead, while retaining the existing Windows and non-Darwin classification. Remove the unused duplicate build-tasksFileUtiland the AndroidSdk rename helper: its only production caller isAndroidAppManifest.WriteToFile, which has no in-repository callers.Compatibility: This intentionally removes the shipped public
AndroidAppManifest.WriteToFile(string)API.Write(XmlWriter)remains available, but callers must manage file creation and replacement themselves; it does not by itself preserve the removed method's replacement semantics. The chmod fallback and Windows path/registry interop are unchanged.Platform limitation: The checks cover the Apple platforms recognized by the runtime. They include watchOS identifiers, but watchOS equivalence with
unameremains unverified: the .NET 10/11 runtime sources omitWATCHOSfrom their platform-name list, and runtime main documentsOperatingSystem.IsWatchOS()as always false. This affects both the modern and netstandard paths.Validation (macOS arm64; baseline
6b8fb8c3742b8ea4aa5f9f92e70a601a28f83360):make leeroy MSBUILD_ARGS=-p:IncludeAndroidJavadoc=true— passed using a session-owned toolchain. An initialmake leeroyfailed because the workload pack expected aMono.Android.xmlthat the default build had not generated../dotnet-local.sh test bin/TestDebug/net11.0/Xamarin.Android.Build.Tests.dll --filter 'FullyQualifiedName~RemoveDirTests|FullyQualifiedName~BuildTest2.NonAndroidNativeLibrariesDoNotProduceWarnings' -v minimal— 7 passed, 4 skipped.dotnet build src/Xamarin.Android.Tools.AndroidSdk/Xamarin.Android.Tools.AndroidSdk.csproj -c Release -p:AndroidToolsDisableMultiTargeting=false -p:DotNetTargetFrameworkVersion=11.0 -v minimal— passed for net11.0 and netstandard2.0; repeating with-p:DotNetTargetFrameworkVersion=10.0passed for net10.0 and netstandard2.0.dotnet test tests/Xamarin.Android.Tools.AndroidSdk-Tests/Xamarin.Android.Tools.AndroidSdk-Tests.csproj -p:AndroidToolsDisableMultiTargeting=false -p:DotNetTargetFrameworkVersion=11.0 -v minimal— 536 passed, 8 skipped.-p:AndroidToolsDisableMultiTargeting=true— 533 passed, 8 skipped, 3 failed. The threeProcessUtilsTestsargument-list failures also reproduce with the saved main-baseline assembly. With--filter 'FullyQualifiedName!~CreateProcessStartInfo_SingleArg&FullyQualifiedName!~CreateProcessStartInfo_MultipleArgs&FullyQualifiedName!~CreateProcessStartInfo_ArgWithSpaces', 529 passed and 8 skipped.WriteToFileand the rename helpers are absent andWrite(XmlWriter)remains present in the final net11.0 and netstandard2.0 assemblies. Both baseline and candidate classified macOS as Darwin under arm64 and x64/Rosetta.-p:DotNetTargetFrameworkVersion=12.0together with multitargeting failed because main has noPublicAPI/net12.0files; the full SDK build above succeeded with the project's default AndroidSdk targeting.Performance: Final-candidate timing was deferred because the shared host was saturated. Earlier baseline comparisons used an intermediate candidate, so they are not used to claim a speedup for this final PR.
Overlap: #12988 changes AndroidSdk targeting and the chmod fallback and edits these files. #13375 adds safety annotations to the removed interop declarations. Neither is a prerequisite, but both will need coordination/rebasing.