镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Introduce OS.FileSystemComparer / FileSystemComparison helpers and apply at filesystem-name comparison sites #12090

Closed as not planned

Description

@rmarinho

Background

While reviewing PR dotnet/android-tools#326 (AVD skin enumeration), two independent rubber-duck reviews surfaced that the skin-dedup SortedSet was using StringComparer.OrdinalIgnoreCase unconditionally, which would silently collapse legitimately distinct skin directories (e.g. Pixel vs pixel) on case-sensitive Linux/macOS filesystems. That was fixed in dotnet/android-tools#326 with an inline OS.IsWindows ? OrdinalIgnoreCase : Ordinal ternary.

A broader scan of the codebase found the same pattern (and the same latent bug) in several other places that compare filesystem names or paths, with no shared helper to reach for. Worth consolidating before more drift accumulates.

Proposed helpers

In src/Xamarin.Android.Tools.AndroidSdk/OS.cs:

public static readonly StringComparer FileSystemComparer =
    IsWindows ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal;

public static readonly StringComparison FileSystemComparison =
    IsWindows ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;

Caveat to document inline: this is a heuristic by OS, not a true per-volume filesystem-case-sensitivity query. macOS APFS is case-insensitive by default, and Windows ReFS can be case-sensitive. The helper matches the codebase's implicit existing assumption (Windows = case-insensitive, everything else = case-sensitive) and is strictly more correct than the current state, but isn't perfect. A future iteration could query the actual filesystem (e.g. via FILE_CASE_SENSITIVE_INFORMATION on Windows, pathconf(_PC_CASE_SENSITIVE) on macOS) if it ever matters.

Sites to migrate

File Line(s) Note
src/Xamarin.Android.Tools.AndroidSdk/FileUtil.cs 193, 195 IsUnderDirectory — pre-existing bug on Linux: path.StartsWith(dir + Sep, OrdinalIgnoreCase) would say /foo/Bar/x is under /foo/bar, which is wrong.
src/Xamarin.Android.Tools.AndroidSdk/AndroidVersions.cs 40 dirs.Distinct(OrdinalIgnoreCase) on framework dir paths — same pre-existing bug on Linux.
src/Xamarin.Android.Tools.AndroidSdk/Runners/AvdManagerRunner.cs 184 Skin dedup (currently uses the inline ternary added in dotnet/android-tools#326 — replace with the helper).
src/Xamarin.Android.Tools.AndroidSdk/Runners/AvdManagerRunner.cs 66, 99, 113 AVD name lookups — debatable. AVD .ini files live on disk so are filesystem-cased, but avdmanager itself treats AVD names case-insensitively in commands. Probably keep as OrdinalIgnoreCase (tool semantics, not filesystem semantics) and add a comment explaining why.

Out of scope

  • The many other StringComparison.OrdinalIgnoreCase uses in the codebase that operate on non-filesystem strings (license IDs, AVD names in process output, "Name:" prefixes in avdmanager list output, ADB serials, etc.). Those are correctly case-insensitive regardless of platform.
  • Adding a per-volume case-sensitivity probe — defer until there's a concrete need.

Acceptance

  • New helpers added to OS.cs with an XML doc summary explaining the heuristic and its limits.
  • All four sites above migrated (or explicitly excluded with a comment).
  • No new OrdinalIgnoreCase introductions for filesystem-name comparisons elsewhere — call out in .github/copilot-instructions.md so future PRs reach for the helper.
  • Build with --no-incremental: 0 warnings, 0 errors. PublicAPI files updated for the new public surface (the helpers are public).
  • A regression test for FileUtil.IsUnderDirectory exercising the case-sensitive Linux behavior (the existing tests likely only cover Windows).

Origin

Spotted during second-pass rubber-duck review of dotnet/android-tools#326. The inline ternary added there works for the AVD skin case but doesn't help the other sites and is itself a candidate for consolidation.

Activity

  1. added
    android-toolsIssues migrated from dotnet/android-tools / relates to the xamarin-android-tools subtree
    on Jul 13, 2026
  2. jonathanpeppers commented on Nov 5, 2026

    @jonathanpeppers
    Member

    🤖 Closing this as not planned because we do not currently have a demonstrated real-world bug that warrants this change.

    The proposal came from code review rather than a reported failure, and some cited sites are stale or absent on current main. The framework-directory deduplication concern is possible on case-sensitive filesystems, but we have no concrete affected scenario.

    An OS-based comparer would also replace one assumption with another: macOS filesystems can be case-sensitive or case-insensitive, and Windows directories can be case-sensitive. It would not reliably identify filesystem behavior. The current production caller of IsUnderDirectory is ZIP extraction containment, where keeping the existing conservative comparison is preferable to broadening it without a demonstrated need.

    We can reopen this if a real-world bug is reported and make a targeted fix based on that reproduction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    android-toolsIssues migrated from dotnet/android-tools / relates to the xamarin-android-tools subtreeenhancementProposed change to current functionality.needs-triageIssues that need to be assigned.

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions