Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
omajid opened this issue Mar 30, 2023 · 30 comments
Open

Comments

@omajid
Copy link
Member

omajid commented Mar 30, 2023

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 30, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 30, 2023
@omajid
Copy link
Member Author

omajid commented Mar 30, 2023

@omajid omajid changed the title GenerateRuntimeGraph fails when building rutime in source-build on Alpine GenerateRuntimeGraph fails when building runtime in source-build on Alpine Mar 30, 2023
@omajid omajid changed the title GenerateRuntimeGraph fails when building runtime in source-build on Alpine GenerateRuntimeGraph fails when building runtime in source-build mode on Alpine Mar 30, 2023
@tmds
Copy link
Member

tmds commented Mar 30, 2023

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.

@ayakael
Copy link
Contributor

ayakael commented Mar 30, 2023

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 #73525 as TargetRid would never be passed to __DistroRid. I'm looking into this right now to propose a better fix.

@tmds
Copy link
Member

tmds commented Mar 30, 2023

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
Copy link
Contributor

ayakael commented Mar 30, 2023

tmds

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

@omajid
Copy link
Member Author

omajid commented Mar 30, 2023

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?

@tmds
Copy link
Member

tmds commented Mar 30, 2023

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

@ayakael
Copy link
Contributor

ayakael commented Mar 30, 2023

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
Copy link
Member

tmds commented Mar 30, 2023

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 #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.

@vcsjones vcsjones added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 30, 2023
@ghost
Copy link

ghost commented Mar 30, 2023

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 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
Copy link
Member

tmds commented Apr 4, 2023

@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
Copy link
Member Author

omajid commented Apr 4, 2023

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
Copy link
Member

tmds commented Apr 4, 2023

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:

<_portableOS Condition="'$(_runtimeOS)' == 'linux-musl' or $(_runtimeOS.StartsWith('alpine'))">linux-musl</_portableOS>

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

<!-- RuntimeOS is the build host rid OS. -->
<InnerBuildArgs>$(InnerBuildArgs) /p:RuntimeOS=$(RuntimeOS)</InnerBuildArgs>

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
Copy link
Contributor

ayakael commented Apr 5, 2023

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:

throw new InvalidOperationException($"AdditionalRuntimeIdentifier {rid} was specified, which could not be found in any existing {nameof(RuntimeGroup)}, and no {nameof(parent)} was specified.");
, ${rid} equals x64 when AdditionalRuntimeId is set to linux-musl-x64. Maybe something gnarly occurs in the parsing logics here?
public static RID Parse(string runtimeIdentifier)
{
string[] parts = new string[(int)RIDPart.Max + 1];
bool omitVersionDelimiter = true;
RIDPart parseState = RIDPart.Base;
int partStart = 0, partLength;
// qualifier is indistinguishable from arch so we cannot distinguish it for parsing purposes
Debug.Assert(ArchitectureDelimiter == QualifierDelimiter);
for (int i = 0; i < runtimeIdentifier.Length; i++)
{
char current = runtimeIdentifier[i];
partLength = i - partStart;
switch (parseState)
{
case RIDPart.Base:
// treat any number as the start of the version
if (current == VersionDelimiter || (current >= '0' && current <= '9'))
{
SetPart();
partStart = i;
if (current == VersionDelimiter)
{
omitVersionDelimiter = false;
partStart = i + 1;
}
parseState = RIDPart.Version;
}
// version might be omitted
else if (current == ArchitectureDelimiter)
{
// ensure there's no version later in the string
if (runtimeIdentifier.IndexOf(VersionDelimiter, i) != -1)
{
break;
}
SetPart();
partStart = i + 1; // skip delimiter
parseState = RIDPart.Architecture;
}
break;
case RIDPart.Version:
if (current == ArchitectureDelimiter)
{
SetPart();
partStart = i + 1; // skip delimiter
parseState = RIDPart.Architecture;
}
break;
case RIDPart.Architecture:
if (current == QualifierDelimiter)
{
SetPart();
partStart = i + 1; // skip delimiter
parseState = RIDPart.Qualifier;
}
break;
default:
break;
}
}

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
Copy link
Contributor

ayakael commented Apr 5, 2023

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
Copy link
Member

tmds commented Apr 5, 2023

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
Copy link
Contributor

ayakael commented Apr 5, 2023

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
Copy link
Contributor

ayakael commented Apr 5, 2023

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
Copy link
Member

tmds commented Apr 5, 2023

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:

else if (current == ArchitectureDelimiter)
{
// ensure there's no version later in the string
if (runtimeIdentifier.IndexOf(VersionDelimiter, i) != -1)
{

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
Copy link
Contributor

ayakael commented Apr 5, 2023

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
Copy link
Member

tmds commented Apr 5, 2023

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:

yield return new object[] { "foo-bar-arm", new RID() { BaseRID = "foo", Architecture = "bar", Qualifier = "arm" } }; // demonstrates ambiguity, avoid using `-` in base
yield return new object[] { "linux-musl-x64", new RID() { BaseRID = "linux", Architecture = "musl", Qualifier = "x64" } }; // yes, we already have ambiguous RIDs

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
Copy link
Member

ericstj commented Apr 6, 2023

linux-musl-x64 should already be part of the RID graph. It's defined in runtimeGroups:

<RuntimeGroup Include="linux-musl">
<Parent>linux</Parent>
<Architectures>x64;x86;arm;armv6;armel;arm64;s390x;ppc64le</Architectures>
</RuntimeGroup>

I would expect this to be pre-loaded into the collection:
RuntimeGroupCollection runtimeGroupCollection = new RuntimeGroupCollection(runtimeGroups);
foreach (string additionalRuntimeIdentifier in AdditionalRuntimeIdentifiers)
{
runtimeGroupCollection.AddRuntimeIdentifier(additionalRuntimeIdentifier, AdditionalRuntimeIdentifierParent);
}

Later on, I'd expect this fast path to be hit:
// Do nothing if we already know about the RID
if (knownRIDs.Contains(rid))
{
return;
}

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.
HashCode hashCode = default;
hashCode.Add(BaseRID);
hashCode.Add(Version);
hashCode.Add(Architecture);
hashCode.Add(Qualifier);

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
Copy link
Member

tmds commented Apr 6, 2023

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
Copy link
Member

ericstj commented Apr 6, 2023

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
Copy link
Member

tmds commented Apr 6, 2023

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
Copy link
Member

tmds commented Apr 6, 2023

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
Copy link
Member

ericstj commented Apr 6, 2023

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
Copy link
Member

tmds commented Apr 6, 2023

The parsing problem is fixed by #84413.

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

@ayakael
Copy link
Contributor

ayakael commented Apr 6, 2023

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

@tmds
Copy link
Member

tmds commented Apr 6, 2023

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.

@agocke agocke added this to the Future milestone Jul 8, 2024
@agocke agocke removed the untriaged New issue has not been triaged by the area owner label Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

6 participants