-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add support for adding RIDs to runtime.json during build #50818
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
<Project Sdk="Microsoft.NET.SDK"> | ||
<Project Sdk="Microsoft.NET.SDK"> | ||
<!-- These are wrapper project files for packaging.--> | ||
<PropertyGroup> | ||
<TargetFrameworks>$(NetCoreAppToolCurrent);net472</TargetFrameworks> | ||
|
@@ -14,6 +14,13 @@ | |
<PackageDescription>Provides runtime information required to resolve target framework, platform, and runtime specific implementations of .NETCore packages.</PackageDescription> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup Condition="'$(DotNetBuildFromSource)' == 'true'"> | ||
<!-- When building from source, ensure the RID we're building for is part of the RID graph --> | ||
<AdditionalRuntimeIdentifiers>$(AdditionalRuntimeIdentifiers);$(OutputRID)</AdditionalRuntimeIdentifiers> | ||
<!-- Also ensure the RID we're bulding on is part of the RID graph, since this may be more specific --> | ||
<AdditionalRuntimeIdentifiers>$(AdditionalRuntimeIdentifiers);$([System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier)</AdditionalRuntimeIdentifiers> | ||
</PropertyGroup> | ||
|
||
<ItemGroup Condition="'$(TargetFramework)' == 'net472'"> | ||
<Compile Include="BuildTask.Desktop.cs" /> | ||
<Compile Include="AssemblyResolver.cs" /> | ||
|
@@ -24,11 +31,14 @@ | |
<Compile Include="Extensions.cs" /> | ||
<Compile Include="GenerateRuntimeGraph.cs" /> | ||
<Compile Include="RID.cs" /> | ||
<Compile Include="RuntimeGroupCollection.cs" /> | ||
<Compile Include="RuntimeGroup.cs" /> | ||
<Compile Include="RuntimeVersion.cs" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<Content Include="runtime.json" PackagePath="/" /> | ||
<Content Condition="'$(AdditionalRuntimeIdentifiers)' == ''" Include="runtime.json" PackagePath="/" /> | ||
<Content Condition="'$(AdditionalRuntimeIdentifiers)' != ''" Include="$(IntermediateOutputPath)runtime.json" PackagePath="/" /> | ||
<Content Include="_._" PackagePath="lib/netstandard1.0" /> | ||
</ItemGroup> | ||
|
||
|
@@ -38,5 +48,14 @@ | |
<PackageReference Include="NuGet.ProjectModel" Version="$(RefOnlyNugetProjectModelVersion)" /> | ||
</ItemGroup> | ||
|
||
<Target Name="GenerateRuntimeJson" Condition="'$(AdditionalRuntimeIdentifiers)' != ''" BeforeTargets="Pack"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this have inputs and outputs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It runs on pack and is meant to only run when properties are set (not easily tracked by inputs/outputs) so I intentionally made it always run. |
||
<MakeDir Directories="$(IntermediateOutputPath)" /> | ||
<GenerateRuntimeGraph RuntimeGroups="@(RuntimeGroupWithQualifiers)" | ||
AdditionalRuntimeIdentifiers="$(AdditionalRuntimeIdentifiers)" | ||
AdditionalRuntimeIdentifierParent="$(AdditionalRuntimeIdentifierParent)" | ||
RuntimeJson="$(IntermediateOutputPath)runtime.json" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: consider defining a property for the path to the runtime.json file. You are currently hardcoding it in two places. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will do so in a follow up PR |
||
UpdateRuntimeFiles="True" /> | ||
</Target> | ||
|
||
<Import Project="runtimeGroups.props" /> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,37 +1,44 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Diagnostics; | ||
using System.Text; | ||
|
||
namespace Microsoft.NETCore.Platforms.BuildTasks | ||
{ | ||
internal class RID | ||
public class RID | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the access modifier changed because of tests depending on the RID class now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I didn't bother setting up internals visible to since this is an msbuild task and the only consumer of it is our build and tests. |
||
{ | ||
internal const char VersionDelimiter = '.'; | ||
internal const char ArchitectureDelimiter = '-'; | ||
internal const char QualifierDelimiter = '-'; | ||
|
||
public string BaseRID { get; set; } | ||
public string VersionDelimiter { get; set; } | ||
public string Version { get; set; } | ||
public string ArchitectureDelimiter { get; set; } | ||
public bool OmitVersionDelimiter { get; set; } | ||
public RuntimeVersion Version { get; set; } | ||
public string Architecture { get; set; } | ||
public string QualifierDelimiter { get; set; } | ||
public string Qualifier { get; set; } | ||
|
||
public override string ToString() | ||
{ | ||
StringBuilder builder = new StringBuilder(BaseRID); | ||
|
||
if (HasVersion()) | ||
if (HasVersion) | ||
{ | ||
builder.Append(VersionDelimiter); | ||
if (!OmitVersionDelimiter) | ||
{ | ||
builder.Append(VersionDelimiter); | ||
} | ||
builder.Append(Version); | ||
} | ||
|
||
if (HasArchitecture()) | ||
if (HasArchitecture) | ||
{ | ||
builder.Append(ArchitectureDelimiter); | ||
builder.Append(Architecture); | ||
} | ||
|
||
if (HasQualifier()) | ||
if (HasQualifier) | ||
{ | ||
builder.Append(QualifierDelimiter); | ||
builder.Append(Qualifier); | ||
|
@@ -40,20 +47,153 @@ public override string ToString() | |
return builder.ToString(); | ||
} | ||
|
||
public bool HasVersion() | ||
private enum RIDPart : int | ||
{ | ||
Base = 0, | ||
Version, | ||
Architecture, | ||
Qualifier, | ||
Max = Qualifier | ||
} | ||
|
||
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 = 0; | ||
|
||
// 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; | ||
} | ||
} | ||
|
||
partLength = runtimeIdentifier.Length - partStart; | ||
if (partLength > 0) | ||
{ | ||
SetPart(); | ||
} | ||
|
||
string GetPart(RIDPart part) | ||
{ | ||
return parts[(int)part]; | ||
} | ||
|
||
void SetPart() | ||
{ | ||
if (partLength == 0) | ||
{ | ||
throw new ArgumentException($"Unexpected delimiter at position {partStart} in \"{runtimeIdentifier}\""); | ||
} | ||
|
||
parts[(int)parseState] = runtimeIdentifier.Substring(partStart, partLength); | ||
} | ||
|
||
string version = GetPart(RIDPart.Version); | ||
|
||
if (version == null) | ||
{ | ||
omitVersionDelimiter = false; | ||
} | ||
|
||
return new RID() | ||
{ | ||
BaseRID = GetPart(RIDPart.Base), | ||
OmitVersionDelimiter = omitVersionDelimiter, | ||
Version = version == null ? null : new RuntimeVersion(version), | ||
Architecture = GetPart(RIDPart.Architecture), | ||
Qualifier = GetPart(RIDPart.Qualifier) | ||
}; | ||
} | ||
|
||
public bool HasVersion => Version != null; | ||
|
||
public bool HasArchitecture => Architecture != null; | ||
|
||
public bool HasQualifier => Qualifier != null; | ||
|
||
public override bool Equals(object obj) | ||
{ | ||
return Version != null; | ||
return Equals(obj as RID); | ||
} | ||
|
||
public bool HasArchitecture() | ||
public bool Equals(RID obj) | ||
{ | ||
return Architecture != null; | ||
return object.ReferenceEquals(obj, this) || | ||
(obj is not null && | ||
BaseRID == obj.BaseRID && | ||
(Version == null || OmitVersionDelimiter == obj.OmitVersionDelimiter) && | ||
Version == obj.Version && | ||
Architecture == obj.Architecture && | ||
Qualifier == obj.Qualifier); | ||
|
||
} | ||
|
||
public bool HasQualifier() | ||
public override int GetHashCode() | ||
{ | ||
return Qualifier != null; | ||
#if NETFRAMEWORK | ||
return BaseRID.GetHashCode(); | ||
#else | ||
HashCode hashCode = default; | ||
hashCode.Add(BaseRID); | ||
hashCode.Add(Version); | ||
hashCode.Add(Architecture); | ||
hashCode.Add(Qualifier); | ||
return hashCode.ToHashCode(); | ||
#endif | ||
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need to add the second.
OutputRID
should be$([System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier)
that ignoresDOTNET_RUNTIME_ID
envvar.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RuntimeInformation.RuntimeIdentifier
does honor theDOTNET_RUNTIME_ID
envvar. What did you mean by ignores?I added both based on your suggestion in the issue. I think having both might better support a case where someone is building portable Linux from source (OutputRid=linux-x64) and expecting it to support a new distro (myLinux.1.0-x64)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume (but maybe this is wrong) that it may be necessary to set
DOTNET_RUNTIME_ID
when building the source-built tarball on a distribution that is not in the graph of the SDK used during the build.In this case, we'd still want the
OutputRid
to reflect/etc/os-release
. We'd be settingDOTNET_RUNTIME_ID
just to make the build work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. This is a bit of chicken and egg.
How do we even run the code to read from
/etc/os-release
to calculateOutputRID
if that code needsDOTNET_RUNTIME_ID
set in order to run? The host needs to "know" what RID it's running on so that it can make decisions about which RID-specific assets to select, and its the same mechanism that impacts this API.We could just make the user specify
OutputRID
on the command-line, but that's lame and requires the user to get it right.If we didn't want that, we'd need someway to run this bit of code from the host and use it during the build, instead of
$([System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier)
as is done today. This needs to happen outside this task/project since it's something that the build needs to consume (when producing RID-specific packages).@agocke @vitek-karas @VSadov what do you think about this: how should we best automatically calculate the "ideal" RID for our purposes of source-build on new platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought: should you even need to set
DOTNET_RUNTIME_ID
? Ideally in the case where alinux-arch
host build is running on a machine that's saysomeDistro.version-arch
it should usesomeDistro.version-arch
if that's "known", but if it isn't "known" seems like the host can just assumelinux-arch
for resolving assets (since it's running after all). Is this how the host works? Should it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this will just work, I'm not sure. Let's assume it does.
Once this is merged, we should add a test in source-build repo that verifies this use-case works.
The test can change
/etc/os-release
to something unknown and see if things still build.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The host has relatively complicated code to determine the current RID:
runtime/src/native/corehost/hostmisc/pal.unix.cpp
Line 710 in bc9f00c
This algorithm may "fail", in which case the host falls back to a basic RID - on Linux it will be simply
linux
RID.runtime/src/native/corehost/hostmisc/pal.h
Line 85 in bc9f00c
If this algorithm works, then there should be no need for
DOTNET_RUNTIME_ID
. If it fails and the fallback is not enough, or if it returns wrong value, thenDOTNET_RUNTIME_ID
is a way to override it and might be necessary. In that case though, we might want to bake the new RID into the hosts produced by the source build somehow, so that running apps using the results of such source build won't requireDOTNET_RUNTIME_ID
to be set. Not sure what the workflow here is...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, very cool. In that case I think not setting
DOTNET_RUNTIME_ID
should just work. I'll go ahead and remove the second value that's being added toAdditionalRuntimeIdentifiers
. I like it, this simplifies the the behavior here.Can you help with this part @tmds or @omajid? We probably also want to make sure that the generated runtime.json will include that new RID (to ensure it's flowing through the targets correctly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crummel and @MichaelSimons can help with this. I can take a look also, but I'm not familiar with the source-build CI setup.