Add support for adding RIDs to runtime.json during build #50818
Conversation
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue DetailsFixes #48507 cc @omajid This replaces dotnet/arcade#7141 and #50157 The most interesting thing to look at are the tests for GenerateRuntimeGraph, those show how RID additions are handled. It took a bit of refinement until I could come up with something that I think was most consistent with what would happen if we actually made source changes to include the RIDs.
|
ghost
added this to In Progress
in 
ericstj
changed the title
| @@ -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.
Should this have inputs and outputs?
There was a problem hiding this comment.
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.
src/libraries/Microsoft.NETCore.Platforms/tests/GenerateRuntimeGraphTests.cs
Show resolved
Hide resolved
| /// <param name="runtimeFilePrefix">a unique prefix to use for the generated </param> | ||
| private void AssertRuntimeGraphAdditions(string[] additionalRIDs, RuntimeDescription[] expectedAdditions, string additionalRIDParent = null, [CallerMemberName] string runtimeFilePrefix = null) | ||
| { | ||
| string runtimeFile = runtimeFilePrefix + ".runtime.json"; |
There was a problem hiding this comment.
Should these files be created on a temp directory and cleaned up after the test class is disposed?
We have: https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/TestUtilities/System/IO/FileCleanupTestBase.cs to achieve that.
There was a problem hiding this comment.
Test working directory is a temp directory. I don’t think cleaning them up is desirable, they are useful for debugging.
There was a problem hiding this comment.
Ok, as long as the working dir is a temp directory, I'm fine with it not being cleaned up.
There was a problem hiding this comment.
I'm calling it temp in the context of it's the test output directory. It will be cleaned up by normal dev activities. I intentionally don't want to hide these files in some random location as they are useful for diffing what the result of the test is. I see these as related to TestResults output which is placed in the same directory and treated similarly by the test runner.
ericstj
force-pushed
the
platfromsAdditionalRIDs
branch
from
ad04d00
to
024e2b9
Compare
| <!-- 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> |
There was a problem hiding this comment.
I'm not sure we need to add the second.
OutputRID should be $([System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier) that ignores DOTNET_RUNTIME_ID envvar.
There was a problem hiding this comment.
$([System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier)that ignoresDOTNET_RUNTIME_IDenvvar
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The host has relatively complicated code to determine the current RID:
(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.
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...
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@crummel and @MichaelSimons can help with this. I can take a look also, but I'm not familiar with the source-build CI setup.
The pack task itself doesn't do anything but depend on a sequence of tasks. As a result running a target `BeforeTargets="Pack"` actually runs after the package is created, since dependencies run before BeforeTargets. Sequence the target before GenerateNuspec instead.
ericstj
force-pushed
the
platfromsAdditionalRIDs
branch
from
7b9a372
to
3246f2f
Compare
| @@ -12,6 +12,9 @@ | |||
| <SuppressDependenciesWhenPacking>true</SuppressDependenciesWhenPacking> | |||
| <NoWarn>$(NoWarn);NU5128</NoWarn> <!-- No Dependencies--> | |||
| <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> | |||
There was a problem hiding this comment.
It's good we're appending the OutputRID at the end here. The AdditionalRuntimeIdentifiers will be added in order, so we want them to be ordered from least to most specific.
There was a problem hiding this comment.
That shouldn’t matter since the only way to establish a parent RID is through the parent parameter. New RIDs are added recursively following the rules in the runtime groups and parent. When written the graph is sorted.
In other words if you are imagining a case where order matters then you probably don’t need to specify that RID since it will be discovered recursively.
|
@ViktorHofer or @safern can I get a review here? |
| using System.Text; | ||
|
|
||
| namespace Microsoft.NETCore.Platforms.BuildTasks | ||
| { | ||
| internal class RID | ||
| public class RID |
There was a problem hiding this comment.
I guess the access modifier changed because of tests depending on the RID class now?
There was a problem hiding this comment.
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.
| <GenerateRuntimeGraph RuntimeGroups="@(RuntimeGroupWithQualifiers)" | ||
| AdditionalRuntimeIdentifiers="$(AdditionalRuntimeIdentifiers)" | ||
| AdditionalRuntimeIdentifierParent="$(AdditionalRuntimeIdentifierParent)" | ||
| RuntimeJson="$(IntermediateOutputPath)runtime.json" |
There was a problem hiding this comment.
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.
will do so in a follow up PR
|
LGTM! Thanks for addressing the questions/comments I had. |
Fixes #48507
cc @omajid
This replaces dotnet/arcade#7141 and #50157
The most interesting thing to look at are the tests for GenerateRuntimeGraph, those show how RID additions are handled. It took a bit of refinement until I could come up with something that I think was most consistent with what would happen if we actually made source changes to include the RIDs.