dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.24k stars 4.73k forks source link

GenerateRuntimeGraph fails when building runtime in source-build mode on Alpine #84127

Open omajid opened 1 year ago

omajid commented 1 year ago

Description

The dotnet/runtime build fails when running on Alpine in source-build mode with a portable configuration:

  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: The "GenerateRuntimeGraph" task failed unexpectedly. [TargetFramework=net8.0]
  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: System.InvalidOperationException: AdditionalRuntimeIdentifier x64 was specified, which could not be found in any existing RuntimeGroup, and no parent was specified. [TargetFramework=net8.0]
  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018:    at Microsoft.NETCore.Platforms.BuildTasks.RuntimeGroupCollection.AddRuntimeIdentifier(RID rid, String parent) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs:line 116 [TargetFramework=net8.0]
  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018:    at Microsoft.NETCore.Platforms.BuildTasks.RuntimeGroupCollection.AddRuntimeIdentifier(RID rid, String parent) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs:line 140 [TargetFramework=net8.0]
  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018:    at Microsoft.NETCore.Platforms.BuildTasks.RuntimeGroupCollection.AddRuntimeIdentifier(RID rid, String parent) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs:line 140 [TargetFramework=net8.0]
  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018:    at Microsoft.NETCore.Platforms.BuildTasks.RuntimeGroupCollection.AddRuntimeIdentifier(String runtimeIdentifier, String parent) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs:line 39 [TargetFramework=net8.0]
  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018:    at Microsoft.NETCore.Platforms.BuildTasks.GenerateRuntimeGraph.AddRuntimeIdentifiers(ICollection`1 runtimeGroups) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/GenerateRuntimeGraph.cs:line 327 [TargetFramework=net8.0]
  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018:    at Microsoft.NETCore.Platforms.BuildTasks.GenerateRuntimeGraph.Execute() in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/GenerateRuntimeGraph.cs:line 157 [TargetFramework=net8.0]
  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() [TargetFramework=net8.0]
  /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) [TargetFramework=net8.0]

The problem is most visible with .NET 6, because dotnet/runtime is built in portable mode as part of the end-to-end source-build process.

With .NET 7 and .NET 8, source-build only builds dotnet/runtime in non-portable mode, so this problem doesn't appear there out of the box. It only appears when building dotnet/runtime by itself, in a configuration that's not used by source-build.

Reproduction Steps

Create an alpine container using a Dockerfile

# Dockerfile suitable for building .NET on Alpine

FROM alpine:3.17

RUN apk update && \
    apk add \
    bash \
    binutils \
    clang \
    cmake \
    git \
    gcc \
    icu-dev \
    krb5-dev \
    llvm \
    libstdc++ \
    linux-headers \
    lttng-ust-dev \
    make \
    musl-dev \
    openssl-dev \
    python3 \
    zlib-dev \

Then git clone https://github.com/dotnet/runtime in the container and try and build runtime in source-build mode in the appropriate branch:

main:

./build.sh -p:TargetRid=linux-musl-x64 -c Release --restore --build --pack /p:ArcadeBuildFromSource=true -bl

release/7.0 and release/6.0

./build.sh -c Release --restore --build --pack /p:ArcadeBuildFromSource=true -bl

Expected behavior

Build works

Actual behavior

Build fails

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

omajid commented 1 year ago

cc @ayakael @crummel @mthalman @MichaelSimons @tmds

tmds commented 1 year ago

This is probably due to https://github.com/dotnet/runtime/pull/77508 being merged in 6.0 for runtime, but not the changes in installer: https://github.com/dotnet/installer/pull/14816.

The fix is to revert https://github.com/dotnet/runtime/pull/77508 and maybe some other PRs that got merged for eliminating the portable build with 6.0.

ayakael commented 1 year ago

This is probably due to #77508 being merged in 6.0 for runtime, but not the changes in installer: dotnet/installer#14816.

The fix is to revert #77508 and maybe some other PRs that got merged for eliminating the portable build with 6.0.

Reverting this particular one would break a workaround we use for https://github.com/dotnet/runtime/issues/73525 as TargetRid would never be passed to __DistroRid. I'm looking into this right now to propose a better fix.

tmds commented 1 year ago

Reverting this particular one would break a workaround we use for #73525 as TargetRid would never be passed to __DistroRid

The fix for this can backported.

ayakael commented 1 year ago

tmds

Could you point to the fix for this so I can backport?

omajid commented 1 year ago

This is probably due to https://github.com/dotnet/runtime/pull/77508 being merged in 6.0 for runtime, but not the changes in installer: https://github.com/dotnet/installer/pull/14816.

I am not sure I follow. I ran into this issue when building dotnet/runtime standalone. How is dotnet/installer involved?

tmds commented 1 year ago

I am not sure I follow. I ran into this issue when building dotnet/runtime standalone.

SourceBuild.props no longer worked standalone with the changes for the non-portable build. That is fixed on main.

If you want to make it work standalone on 6.0 as-is, you can add /p:BaseOS=.

Could you point to the fix for this so I can backport?

I think that would be https://github.com/dotnet/runtime/pull/80901 and https://github.com/dotnet/runtime/pull/81497

ayakael commented 1 year ago

This is probably due to #77508 being merged in 6.0 for runtime, but not the changes in installer: dotnet/installer#14816.

I am not sure I follow. I ran into this issue when building dotnet/runtime standalone. How is dotnet/installer involved?

Applying the following diff yields the same behaviour, as well:

diff --git a/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj b/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
index 84d4bb88604..9fc8774c083 100644
--- a/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
+++ b/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
@@ -18,7 +18,7 @@
     <PackageDescription>Provides runtime information required to resolve target framework, platform, and runtime specific implementations of .NETCore packages.</PackageDescription>

     <!-- When building from source, ensure the RID we're building for is part of the RID graph -->
-    <AdditionalRuntimeIdentifiers Condition="'$(DotNetBuildFromSource)' == 'true'">$(AdditionalRuntimeIdentifiers);$(OutputRID)</AdditionalRuntimeIdentifiers>
+    <AdditionalRuntimeIdentifiers Condition="'$(DotNetBuildFromSource)' == 'true'">linux-musl-x64</AdditionalRuntimeIdentifiers>
     <ServicingVersion>8</ServicingVersion>
     <GeneratePackageOnBuild>true</GeneratePacm kageOnBuild>
   </PropertyGroup>

Thus, this seems to have to do something with how GenerateRuntimeGraph parses AdditionalRuntimeIdentifiers. If it had to do with how OutputRid was set (or not, in the case of lack of the installer backport), this would've fixed the error.

Also, re-reading omajid's tests, they also tested against main. So, there's someting wrong with the code regardless of the backport.

I am not sure I follow. I ran into this issue when building dotnet/runtime standalone.

SourceBuild.props no longer worked standalone with the changes for the non-portable build. That is fixed on main.

If you want to make it work standalone on 6.0 as-is, you can add /p:BaseOS=.

Could you point to the fix for this so I can backport?

I think that would be #80901 and #81497

Awesome, thank you for pointing that out :)

tmds commented 1 year ago

Thus, this seems to have to do something with how GenerateRuntimeGraph parses AdditionalRuntimeIdentifiers. If it had to do with how OutputRid was set (or not, in the case of lack of the installer backport), this would've fixed the error.

There were other PRs backported to 6.0 for eliminating portable build, like https://github.com/dotnet/runtime/pull/77510.

I think we should start by reverting those, and seeing if there is still an issue to be solved.

Also, re-reading omajid's tests, they also tested against main. So, there's someting wrong with the code regardless of the backport.

afaik everything works well on main.

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/runtime-infrastructure See info in area-owners.md if you want to be subscribed.

Issue Details
### Description The dotnet/runtime build fails when running on Alpine in source-build mode with a portable configuration: ``` /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: The "GenerateRuntimeGraph" task failed unexpectedly. [TargetFramework=net8.0] /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: System.InvalidOperationException: AdditionalRuntimeIdentifier x64 was specified, which could not be found in any existing RuntimeGroup, and no parent was specified. [TargetFramework=net8.0] /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: at Microsoft.NETCore.Platforms.BuildTasks.RuntimeGroupCollection.AddRuntimeIdentifier(RID rid, String parent) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs:line 116 [TargetFramework=net8.0] /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: at Microsoft.NETCore.Platforms.BuildTasks.RuntimeGroupCollection.AddRuntimeIdentifier(RID rid, String parent) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs:line 140 [TargetFramework=net8.0] /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: at Microsoft.NETCore.Platforms.BuildTasks.RuntimeGroupCollection.AddRuntimeIdentifier(RID rid, String parent) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs:line 140 [TargetFramework=net8.0] /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: at Microsoft.NETCore.Platforms.BuildTasks.RuntimeGroupCollection.AddRuntimeIdentifier(String runtimeIdentifier, String parent) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs:line 39 [TargetFramework=net8.0] /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: at Microsoft.NETCore.Platforms.BuildTasks.GenerateRuntimeGraph.AddRuntimeIdentifiers(ICollection`1 runtimeGroups) in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/GenerateRuntimeGraph.cs:line 327 [TargetFramework=net8.0] /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: at Microsoft.NETCore.Platforms.BuildTasks.GenerateRuntimeGraph.Execute() in /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/GenerateRuntimeGraph.cs:line 157 [TargetFramework=net8.0] /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() [TargetFramework=net8.0] /runtime/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(63,5): error MSB4018: at Microsoft.Build.BackEnd.TaskBuilder.ExecuteInstantiatedTask(ITaskExecutionHost taskExecutionHost, TaskLoggingContext taskLoggingContext, TaskHost taskHost, ItemBucket bucket, TaskExecutionMode howToExecuteTask) [TargetFramework=net8.0] ``` The problem is most visible with .NET 6, because dotnet/runtime is built in portable mode as part of the end-to-end source-build process. With .NET 7 and .NET 8, source-build only builds dotnet/runtime in non-portable mode, so this problem doesn't appear there out of the box. It only appears when building dotnet/runtime by itself, in a configuration that's not used by source-build. ### Reproduction Steps Create an alpine container using a `Dockerfile` ```Dockerfile # Dockerfile suitable for building .NET on Alpine FROM alpine:3.17 RUN apk update && \ apk add \ bash \ binutils \ clang \ cmake \ git \ gcc \ icu-dev \ krb5-dev \ llvm \ libstdc++ \ linux-headers \ lttng-ust-dev \ make \ musl-dev \ openssl-dev \ python3 \ zlib-dev \ ``` Then `git clone https://github.com/dotnet/runtime` in the container and try and build runtime in source-build mode in the appropriate branch: main: ./build.sh -p:TargetRid=linux-musl-x64 -c Release --restore --build --pack /p:ArcadeBuildFromSource=true -bl release/7.0 and release/6.0 ./build.sh -c Release --restore --build --pack /p:ArcadeBuildFromSource=true -bl ### Expected behavior Build works ### Actual behavior Build fails ### Regression? _No response_ ### Known Workarounds _No response_ ### Configuration _No response_ ### Other information _No response_
Author: omajid
Assignees: -
Labels: `area-Infrastructure`, `untriaged`
Milestone: -
tmds commented 1 year ago

@omajid @ayakael is there an issue on main?

Are you looking into fixing the issue with 6.0? If you want, I can look into it.

omajid commented 1 year ago

is there an issue on main?

On main, this command fails when running on Alpine.

./build.sh -p:TargetRid=linux-musl-x64 -c Release --restore --build --pack /p:ArcadeBuildFromSource=true -bl

My goal was to build runtime in portable mode. Is it supposed to work with just TargetRid specified like this?

tmds commented 1 year ago

Is it supposed to work with just TargetRid specified like this?

TargetRid names the output of the build. You can set it to anything you want.

I'm not sure if you actually need to set something because there is Alpine handling here:

https://github.com/dotnet/runtime/blob/e579ccb60acb044453f2cf1907482b165658488e/Directory.Build.props#L198

If it's needed, you need to set /p:RuntimeOS=linux-musl:

https://github.com/dotnet/runtime/blob/e579ccb60acb044453f2cf1907482b165658488e/eng/SourceBuild.props#L34-L35

My goal was to build runtime in portable mode

I'm not sure what you want to build. source-build (/p:ArcadeBuildFromSource=true) implies non-portable.

ayakael commented 1 year ago

I can reproduce this on .NET 7.0.104 and on .NET 8.0.0-preview.2. This referenced jobs build runtime in portable mode while setting DotNetBuildFromSource=true.

I still suspect there's an issue with the code. When error code occurs by: https://github.com/dotnet/runtime/blob/e13f0dc1e0327b5d0cd0602b55ee257ef554f0a1/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs#L116, ${rid} equals x64 when AdditionalRuntimeId is set to linux-musl-x64. Maybe something gnarly occurs in the parsing logics here?https://github.com/dotnet/runtime/blob/e13f0dc1e0327b5d0cd0602b55ee257ef554f0a1/src/libraries/Microsoft.NETCore.Platforms/src/RID.cs#L59-L122

Note: I needed to force OutputRid to linux-musl-x64 on dotnet8, else the build would target alpine.3.17-x64. I suspect the parsing logics fizz out due to m̀ultiple dashes. I'm C# stupid, so can't help more other than vague intuitions.

ayakael commented 1 year ago

To test this hypothesis, I attempted a build with OutputRid set as linuxmusl-x64. Of course, it failed as no parent as defined. But it failed with this error: error MSB4018: System.InvalidOperationException: AdditionalRuntimeIdentifier linuxmusl-x64 was specified, which could not be found in any existing RuntimeGroup, and no parent was specified. [TargetFramework=net8.0] OutputRid was well parsed. When it is linux-musl-x64, it parses as x64.

tmds commented 1 year ago

I can reproduce this on .NET 7.0.104 and on .NET 8.0.0-preview.2.

What command are you running? on what branch of what repo? I don't see it in the logs.

ayakael commented 1 year ago

Tag v7.0.4 and v8.0.0-preview.2.23128.3 of runtime with the following command:

export DotNetBuildFromSource=true

./build.sh \
    -c Release \
    -bl \
    -clang \
    -arch x64 \
    /p:OutputRid=linux-musl-x64 \
    /p:NoPgoOptimize=true \
    /p:EnableNgenOptimization=false
ayakael commented 1 year ago

I extracted the RID parsing code, and passed linux-musl-x64 through it:

https://dotnetfiddle.net/3ej5qT

As you can see, rid.BaseRID and rid.Architecture comes out as linux (expected linux-musl) and musl (expected x64), respectively. The code just doesn't have a case when a BaseRID has a - character, unless there's a version number tracked using . .

A quick fix is to skip adding OutputRid to AdditionalRuntimeIdentifiers when RuntimeOS is equal to linux-musl:

diff --git a/src/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj b/src/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platfor
ms.csproj
index 1df893388..6a1591035 100644
--- a/src/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
+++ b/src/runtime/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj
@@ -23,7 +23,9 @@
     <_generateRuntimeGraphTargetFramework Condition="'$(MSBuildRuntimeType)' != 'core'">net472</_generateRuntimeGraphTargetFramework>
     <_generateRuntimeGraphTask>$([MSBuild]::NormalizePath('$(BaseOutputPath)', $(Configuration), '$(_generateRuntimeGraphTargetFramework)', '$(AssemblyName).dll'))</_generateRuntimeGraphTask>
     <!-- When building from source, ensure the RID we're building for is part of the RID graph -->
-    <AdditionalRuntimeIdentifiers Condition="'$(DotNetBuildFromSource)' == 'true'">$(AdditionalRuntimeIdentifiers);$(OutputRID)</AdditionalRuntimeIdentifiers>
+  <!-- Skips 'linux-musl' and 'linux-bionic' as 'RID.Parse' doesn't know how to parse 'BaseRID' containing a '-' character 
+       See https://github.com/dotnet/runtime/issues/84127 -->
+    <AdditionalRuntimeIdentifiers Condition="'$(DotNetBuildFromSource)' == 'true' AND ( '$(RuntimeOS)' != 'linux-musl' OR '$(RuntimeOS)' != 'linux-bionic' )">$(AdditionalRuntimeIdentifiers);$(OutputRid)</AdditionalRuntimeIdentifiers>
   </PropertyGroup>

   <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">

An actual fix is adding in a case for parsing BaseRID that contains a - characters. You would probably have to add logics to src/libraries/Microsoft.NETCore.Platforms/src/RID.cs to be able to parse supported architectures, as to extract that from RIDs that don't contain a version string.

tmds commented 1 year ago

as 'RID.Parse' doesn't know how to parse 'BaseRID' containing a '-' character

Aha, good you found the root cause!

Looking at the code here:

https://github.com/dotnet/runtime/blob/042a3509a3f24c6c8da0b0d48eeb45854c8665fc/src/libraries/Microsoft.NETCore.Platforms/src/RID.cs#L91-L95

We should add something like the following (at line 93), which causes the first - of linux-musl-x64 to be considered part of the RIDPart.Base.

// ensure there's no architeture later in the string
if (runtimeIdentifier.IndexOf(ArchitectureDelimiter, i + 1) != -1)
{
    break;
}
ayakael commented 1 year ago
if (runtimeIdentifier.IndexOf(ArchitectureDelimiter, i + 1) != -1)

Nice, that did it! I wish I could read C#! Want me to get a PR going for this?

tmds commented 1 year ago

Nice, that did it! I wish I could read C#! Want me to get a PR going for this?

I was going to suggest adding something to the tests, and looking at them I see:

https://github.com/dotnet/runtime/blob/042a3509a3f24c6c8da0b0d48eeb45854c8665fc/src/libraries/Microsoft.NETCore.Platforms/tests/RidTests.cs#L22-L23

So, it's a known ambiguity with the parser.

The parsing takes into account an optional qualifier that may exist at the end of the rid, which is also separated by a -. A full rid can look like: [os].[version]-[architecture]-[additional qualifiers]. (see https://learn.microsoft.com/en-us/dotnet/core/rid-catalog).

I'm not sure what the desired solution is.

One option would be to make the parser aware of the architectures. We know the dashes before the architecture should be considered part of the os.

@ericstj @ViktorHofer what do you think?

ericstj commented 1 year ago

linux-musl-x64 should already be part of the RID graph. It's defined in runtimeGroups: https://github.com/dotnet/runtime/blob/58d1ecb3e50ead5cd48752bbaedf6dc23ccd2523/src/libraries/Microsoft.NETCore.Platforms/src/runtimeGroups.props#L12-L15 I would expect this to be pre-loaded into the collection: https://github.com/dotnet/runtime/blob/58d1ecb3e50ead5cd48752bbaedf6dc23ccd2523/src/libraries/Microsoft.NETCore.Platforms/src/GenerateRuntimeGraph.cs#L323-L328 Later on, I'd expect this fast path to be hit: https://github.com/dotnet/runtime/blob/58d1ecb3e50ead5cd48752bbaedf6dc23ccd2523/src/libraries/Microsoft.NETCore.Platforms/src/RuntimeGroupCollection.cs#L44-L48 However it's not hit, since we hit the ambiguous case where linux-musl-x64 was parsed differently, so it's getting a different hashcode even though its string representation is identical. https://github.com/dotnet/runtime/blob/58d1ecb3e50ead5cd48752bbaedf6dc23ccd2523/src/libraries/Microsoft.NETCore.Platforms/src/RID.cs#L190-L194

One fix would be to use a comparer for this hashset that treated RIDs that were the same string representation as equal. There's really not a case where we'd define structured RIDs from the runtime groups with this ambiguity - it's only comping from string parsing in which case we want to ignore that ambiguity.

if (runtimeIdentifier.IndexOf(ArchitectureDelimiter, i + 1) != -1) Nice, that did it! I wish I could read C#! Want me to get a PR going for this?

That's actually ignoring Qualifier. The same check works for Version because the version delimiter can only occur once. I think one option would be to make the Parse option have a flag to ignore Qualifiers and then plumb this through for AdditionalRuntimeIdentifier. The use of qualifiers was something we did early on (-aot could be used to provide a different asset for .NET Native vs CoreCLR) but then later abandoned due to RID graph explosion. I think dropping it from the AdditionalRuntimeIdentifier scenario is reasonable.

tmds commented 1 year ago

That's actually ignoring Qualifier.

I had come to that conclusion as well.

@ericstj I had also suggested another option:

One option would be to make the parser aware of the architectures. We know the dashes before the architecture should be considered part of the os.

Any thoughts about that?

I think one option would be to make the Parse option have a flag to ignore Qualifiers and then plumb this through for AdditionalRuntimeIdentifier.

Shall I go ahead and implement this?

ericstj commented 1 year ago

I'd prefer to not further complicate the parsing algorithm with lookahead searches for known architectures. If anything I'd prefer we simplify it.

My order of preference would be:

  1. Change the RuntimeGroupCollection knownRIDs comparer to use string value of RID, or just switch to HashSet.
  2. Parse(string, noQualifier = true)
  3. Add knowledge of existing architectures
tmds commented 1 year ago

Later on, I'd expect this fast path to be hit:

I assume the path doesn't get hit because the knownRIDs didn't get parsed but constructed. So it looks like: BaseRID = "linux-musl", Architecture = "x64", Qualifier = "". And then fails equality against BaseRID = "linux", Architecture = "musl", Qualifier = "x64".

Because we know the additional rids won't have a qualifier, my preferred option would be:

Parse(string, noQualifier = true)

Otherwise we still have issue when an additional runtime identifier has - in its os and is not yet known.

tmds commented 1 year ago

Tag v7.0.4 and v8.0.0-preview.2.23128.3 of runtime with the following command:

@ayakael because source-build uses a non-portable rid, it won't encounter this parse issue. To make this command work, you can probably unblock yourself by explicitly adding /p:AdditionalRuntimeIdentifiers= to it.

ericstj commented 1 year ago

I'm good with that. I'd even consider deprecating the qualifier completely if we thought we could remove the -aot RIDs, not sure about that breaking change though.

I'd also be OK with doing both 1&2. It's not correct to allow two different RIDs with the same string representation but different meaning into the graph. Things will break later on when writing the graph in this case.

tmds commented 1 year ago

The parsing problem is fixed by https://github.com/dotnet/runtime/pull/84413.

@omajid @ayakael are we good to close this issue?

ayakael commented 1 year ago

Yes, I'd like to backport this to release/6.0 to unblock the testing CI if that's okay.

tmds commented 1 year ago

I think the most important CI, is the one for main. I'm not sure the 6.0 will catch many issues at this point. Or if it will catch them in time, so you can act on them before the release date.

You can make the request on the PR. The maintainers will decide.