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

Add support for adding RIDs to runtime.json during build #50818

Merged
merged 4 commits into from
Apr 13, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,24 @@ public ITaskItem[] RuntimeGroups
set;
}

/// <summary>
/// Additional runtime identifiers to add to the graph.
/// </summary>
public string[] AdditionalRuntimeIdentifiers
{
get;
set;
}

/// <summary>
/// Parent RID to use for any unknown AdditionalRuntimeIdentifer.
/// </summary>
public string AdditionalRuntimeIdentifierParent
{
get;
set;
}

/// <summary>
/// Optional source Runtime.json to use as a starting point when merging additional RuntimeGroups
/// </summary>
Expand Down Expand Up @@ -134,7 +152,11 @@ public override bool Execute()
runtimeGraph = new RuntimeGraph();
}

foreach (var runtimeGroup in RuntimeGroups.NullAsEmpty().Select(i => new RuntimeGroup(i)))
List<RuntimeGroup> runtimeGroups = RuntimeGroups.NullAsEmpty().Select(i => new RuntimeGroup(i)).ToList();

AddRuntimeIdentifiers(runtimeGroups);

foreach (var runtimeGroup in runtimeGroups)
{
runtimeGraph = SafeMerge(runtimeGraph, runtimeGroup);
}
Expand Down Expand Up @@ -291,6 +313,21 @@ private void ValidateImports(RuntimeGraph runtimeGraph, IDictionary<string, stri
}
}

private void AddRuntimeIdentifiers(ICollection<RuntimeGroup> runtimeGroups)
{
if (AdditionalRuntimeIdentifiers == null || AdditionalRuntimeIdentifiers.Length == 0)
{
return;
}

RuntimeGroupCollection runtimeGroupCollection = new RuntimeGroupCollection(runtimeGroups);

foreach (string additionalRuntimeIdentifier in AdditionalRuntimeIdentifiers)
{
runtimeGroupCollection.AddRuntimeIdentifier(additionalRuntimeIdentifier, AdditionalRuntimeIdentifierParent);
}
}

private static IDictionary<string, IEnumerable<string>> GetCompatibilityMap(RuntimeGraph graph)
{
Dictionary<string, IEnumerable<string>> compatibilityMap = new Dictionary<string, IEnumerable<string>>();
Expand Down
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>
Expand All @@ -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>
Copy link
Member

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 ignores DOTNET_RUNTIME_ID envvar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$([System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier) that ignores DOTNET_RUNTIME_ID envvar

RuntimeInformation.RuntimeIdentifier does honor the DOTNET_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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did you mean by ignores?

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 setting DOTNET_RUNTIME_ID just to make the build work.

Copy link
Member Author

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 calculate OutputRID if that code needs DOTNET_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.

Copy link
Member Author

@ericstj ericstj Apr 8, 2021

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 a linux-arch host build is running on a machine that's say someDistro.version-arch it should use someDistro.version-arch if that's "known", but if it isn't "known" seems like the host can just assume linux-arch for resolving assets (since it's running after all). Is this how the host works? Should it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should you even need to set DOTNET_RUNTIME_ID?

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.

Copy link
Member

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:

pal::string_t pal::get_current_os_rid_platform()
(there are special versions of this for certain distros, same source file).
This algorithm may "fail", in which case the host falls back to a basic RID - on Linux it will be simply linux RID.
#define FALLBACK_HOST_RID _X("linux")

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, then DOTNET_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 require DOTNET_RUNTIME_ID to be set. Not sure what the workflow here is...

Copy link
Member Author

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 to AdditionalRuntimeIdentifiers. I like it, this simplifies the the behavior here.

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.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help with this part @tmds or @omajid?

@crummel and @MichaelSimons can help with this. I can take a look also, but I'm not familiar with the source-build CI setup.

</PropertyGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'net472'">
<Compile Include="BuildTask.Desktop.cs" />
<Compile Include="AssemblyResolver.cs" />
Expand All @@ -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>

Expand All @@ -38,5 +48,14 @@
<PackageReference Include="NuGet.ProjectModel" Version="$(RefOnlyNugetProjectModelVersion)" />
</ItemGroup>

<Target Name="GenerateRuntimeJson" Condition="'$(AdditionalRuntimeIdentifiers)' != ''" BeforeTargets="Pack">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have inputs and outputs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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>
172 changes: 156 additions & 16 deletions src/libraries/Microsoft.NETCore.Platforms/src/RID.cs
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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Expand All @@ -40,20 +47,153 @@ public override string ToString()
return builder.ToString();
}

public bool HasVersion()
private enum RIDPart : int
{
Base = 0,
Version,
Architcture,
ericstj marked this conversation as resolved.
Show resolved Hide resolved
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 (-1 != runtimeIdentifier.IndexOf(VersionDelimiter, i))
ericstj marked this conversation as resolved.
Show resolved Hide resolved
{
break;
}
SetPart();
partStart = i + 1; // skip delimiter
parseState = RIDPart.Architcture;
}
break;
case RIDPart.Version:
if (current == ArchitectureDelimiter)
{
SetPart();
partStart = i + 1; // skip delimiter
parseState = RIDPart.Architcture;
}
break;
case RIDPart.Architcture:
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}");
ericstj marked this conversation as resolved.
Show resolved Hide resolved
}

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.Architcture),
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 null) &&
ericstj marked this conversation as resolved.
Show resolved Hide resolved
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
}
}

}
Loading