Make host look for RID-specific assets via a known list by default #84100

elinor-fung · 2023-03-29T23:26:45Z

Host probing algorithm portion of #83246 - looks for a list of RIDs based on how the host is built:

explicit RID from host build - this may be a non-portable RID for non-portable builds of the host/runtime
non-distro/version-specific RIDs based on the host's build.

As long as an app is running on .NET 8.0 (or above), using the probing algorithm is enabled by default - regardless of its TFM. For example, if an app/component targets net7.0 (TargetFrameworkMoniker=net7.0), enables roll-forward on major version, and ends up running on .NET 8.0, the probing algorithm is still enabled by default.

As a backwards compat fallback, the runtime config property System.Host.Resolution.ReadRidGraph (or another name suggestion) can be set to true, which will go back to the existing method of reading the RID fallback graph.

Example of algorithmic probing:

linux-x64
linux
unix-x64
unix
any

or on a non-portable build of the host/runtmie on rhel.9:

rhel.9-x64
rhel.9
[same as above]

ghost · 2023-03-29T23:26:53Z

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Host probing algorithm portion of #83246 - looks for computed RID followed by known non-distro/version-specific RIDs based on the host's build.

If an app targets net8.0 or above, using the probing algorithm is enabled by default. Below net7.0 keeps reading the RID fallback graph - this is in hostpolicy, so we could also say that regardless of what you target, if you are running on 8.0 (for example via roll forward), the algorithm is the default regardless, but I think the target pivot is clearer.

As a backwards compat fallback, the runtime config property System.Host.Resolution.ReadRidGraph (or another name suggestion) can be set to true, which will go back to the existing method of reading the RID fallback graph.

For example:

ubuntu.22.04-x64
ubuntu.22.04
linux-x64
linux
unix-x64
unix
any

There's still one test case for multiple packages with/without RID assets (MostSpecificRidAssemblySelectedPerTypeMultipleAssets) that I haven't created non-graph-reading variation for yet.

Author:	elinor-fung
Assignees:	-
Labels:	`area-Host`
Milestone:	-

…eading in the fallback graph

src/native/corehost/hostpolicy/deps_format.cpp

ghost · 2023-03-31T03:18:42Z

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
- [Breaking change]: Runtime host looks for RID-specific assets via a known list instead of RID graph by default docs#35398
Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

elinor-fung · 2023-03-31T03:19:19Z

cc @richlander

richlander · 2023-03-31T03:42:02Z

System.Host.Resolution.ReadRidGraph

Works for me. I don't have anything better to offer.

vitek-karas · 2023-03-31T10:11:58Z

Just a dump of thoughts on this change (and related topics):

The automatic backward compatibility for < 8.0 means that we can never remove the RID graph from the framework itself, unless in the future we say that it's not valid to run really old apps on newest runtimes anymore.

The host property also has the added complexity that if the app needs to do some of the special magic as defined in the spec, it would need to do two things - inject the RID graph into the app and set the host property, so that it's actually looked at.

Should we maybe always read the RID graph if it's in the app itself? And only avoid reading it if it comes from the framework (with the explicit opt-in for rare cases).

The proposal says that we will add the RID graph customizations into a new section in the .deps.json. This means that the host will have to be able to merge RID graphs. For FDD apps if the opt-in property is on, it should merge the RID from the framework with the new section. For SCD it would need to merge the RID graph from the app with the new section.

There's another question about SCD apps - as designed, in theory the host property should work there as well, but that means the SDK will have to include the RID graph in the app. So we will need the SDK to be aware of the host property and react accordingly - assuming we don't want to include the RID graph by default (it would be really nice to be able to drop the RID graph in SCD by default).

src/installer/tests/HostActivation.Tests/DependencyResolution/RidAssetResolution.cs

src/native/corehost/hostpolicy/deps_format.cpp

src/native/corehost/hostpolicy/deps_format.h

richlander · 2023-03-31T15:14:50Z

If an app targets net8.0 or above
The automatic backward compatibility for < 8.0 means that we can never remove the RID graph from the framework itself, unless in the future we say that it's not valid to run really old apps on newest runtimes anymore.

Excellent point/insight. I was thinking about this yesterday when I read the PR description. I knew there was something more to discuss but I couldn't see around that corner.

Help me understand ... Is the code in question in the actual apphost or in hostfxr (or similar binaries) in the runtime layout? That's clearly an important point for any plan.

I didn't realize this. The first is from an FDD build and the second SCD.

% cat dotnetapp.deps.json | wc -l 
      22
% cat dotnetapp.deps.json | wc -l
    2911

Is there a functional reason why that is needed? The whole design point of SCD is that RID selection has already occurred.

elinor-fung · 2023-03-31T16:40:29Z

The automatic backward compatibility for < 8.0 means that we can never remove the RID graph from the framework itself,

Indeed, great point. I do think it is reasonable to just say running on 8.0 means we don't read the RID graph by default. It just becomes a slightly broader breaking change.

if the app needs to do some of the special magic as defined in the spec, it would need to do two things - inject the RID graph into the app and set the host property, so that it's actually looked at.
Should we maybe always read the RID graph if it's in the app itself?

In my mind, we would keep the distro compat as specified by the app separate from the 'read RID graph' setting. And with putting the distro compat in a new section, we would always read it if it exists.

The proposal says that we will add the RID graph customizations into a new section in the .deps.json. This means that the host will have to be able to merge RID graphs.

I had thoughts kicking about around this that I hadn't put into words yet. The proposal currently mentions using the distro compat in combination with reading the existing RID graph, which would require merging. I see an end desire of this as not using/having the RID graph, but providing the new distro-compat mechanism to achieve the required behaviour instead. Given that, maybe the two options should be prioritized rather than combined.

For example, if you say set the property to fall back to old behaviour, that is what we do, period. Otherwise (default behaviour), if the distro compat section exists, we will use that in combination with the algorithmic probing and if the distro compat section doesn't exist, we will just do the algorithmic probing. And then, eventually, we get rid of the runtimes section entirely. Thoughts?

So we will need the SDK to be aware of the host property and react accordingly - assuming we don't want to include the RID graph by default (it would be really nice to be able to drop the RID graph in SCD by default).

To me, part of having the compat property is that it can be changed post-build/deployment. Perhaps something more incremental - this release: we don't read by default, next: don't include the RID graph by default for SCD, later (or rather optimistically, next): delete the RID graph entirely

elinor-fung · 2023-03-31T16:54:11Z

Is the code in question in the actual apphost or in hostfxr (or similar binaries) in the runtime layout?

It is in hostpolicy, so comes with the runtime - that is, same directory as coreclr. In the case of self-contained single-file, it is in the app host (singlefilehost), just like the runtime itself.

Is there a functional reason why that is needed? The whole design point of SCD is that RID selection has already occurred.

It allows the self-contained app to load portable components (for example, plugins using AssemblyDependencyResolver).

richlander · 2023-03-31T16:57:46Z

We could adopt the approach that any app that runs on net8+ global runtime gets the new behavior. That would cover the roll-forward case.

Do we need to update AssemblyDependencyResolver to have this same probing behavior? I'm guessing that the vast majority of SCD apps don't have plugins. I'd suggest no RID graph by default.

elinor-fung · 2023-04-04T18:09:08Z

Latest commit has it not checking the TFM - so algorithmic if using .NET 8 runtime.

tmds · 2023-04-06T19:57:04Z

src/native/corehost/hostpolicy/deps_format.cpp

+        RID_CURRENT_ARCH_LIST("linux")
+        RID_CURRENT_ARCH_LIST("unix")
+#else
+        RID_CURRENT_ARCH_LIST(FALLBACK_HOST_OS)


For Microsoft's portable build, FALLBACK_HOST_OS is linux.
For a source-built .NET, FALLBACK_HOST_OS matches the rid that is built for, like fedora.37.
In the source-build case, we still want there to be a fallback to linux as well.

Ah, good catch. Adding a TARGET_LINUX case should cover that, right?

#elif defined(TARGET_LINUX) RID_CURRENT_ARCH_LIST("linux") RID_CURRENT_ARCH_LIST("unix")

~~Yes, it should.~~

No, both are TARGET_LINUX

we need something like:

RID_CURRENT_ARCH_LIST(FALLBACK_HOST_OS) RID_CURRENT_ARCH_LIST("linux") RID_CURRENT_ARCH_LIST("unix")

src/native/corehost/setup.cmake

src/native/corehost/hostpolicy/deps_format.cpp

tmds · 2023-04-06T21:52:10Z

src/native/corehost/hostpolicy/deps_format.cpp

+#elif defined(TARGET_OSX)
+        RID_CURRENT_ARCH_LIST("osx")
+        RID_CURRENT_ARCH_LIST("unix")
+#elif defined(TARGET_LINUX_MUSL)


For a non-portable build on a musl-based Linux, we also need to add the target rid for probing.

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

src/native/corehost/hostpolicy/deps_format.cpp

elinor-fung · 2023-05-10T15:36:50Z

This is now using the fixed list based on how the host/runtime is built (description updated).

vitek-karas · 2023-05-12T11:01:47Z

Working on the review, but general comment about the PR. Could you please update the PR's description to match the implemented behavior? For example the description still says that <8.0 apps would read the fallback graph, but the code will never read it by default (regardless of TFM).

vitek-karas

Looks good - thanks a lot!

src/native/corehost/hostpolicy/deps_format.h

src/installer/tests/HostActivation.Tests/DependencyResolution/RidAssetResolution.cs

vitek-karas · 2023-05-12T12:51:54Z

src/installer/tests/HostActivation.Tests/DependencyResolution/RidAssetResolution.cs

+            // When not using the RID graph, the host uses the target OS for which it was built to determine applicable
+            // RIDs that apply, so it can find both the arch-specific and the OS-only assets.
+            // When using the RID graph, the host uses it to resolve any RID-specific assets that don't exactly match
+            // the current RID, so the OS-only asset remains excluded if there are no fallbacks.
            string includedPath = null;
            string excludedPath = $"{CurrentOSAsset};{DifferentArchAsset}";


Maybe not for this change, but this test misses an interesting case.
If there's an asset for the current RID as determined by the host (so on windows it would be win10-x64) and there's no RID graph but it's instructed to read the RID graph, the asset should still resolve.
Currently the test never exercises this case, since the CurrentRid will be the current build target RID (so on windows it would be win-x64), which is currently always portable RID, and thus never the one the host determines on its own.

It might be that on source-built runtime, where the build target is non-portable, it could happen that the host determined RID and the build target RID matches, in which case the test would fail.

I looked at doing this, but it was a bit more complicated than I thought. We'd need to match the fallback RID, since the non-fallback computed RID is ignored if it doesn't exist in the RID graph. But I don't think the tests have a way of knowing the fallback RID right now.

I'll try to do this in a separate change - opened #86250

elinor-fung · 2023-05-12T17:31:01Z

For example the description still says that <8.0 apps would read the fallback graph, but the code will never read it by default (regardless of TFM).

I had tried to update for that a bit ago, but apparently just made it kind of vague. Hopefully did a better job this time.

jkotas · 2023-05-22T15:54:49Z

System.Host.Resolution.ReadRidGraph

This is not the best name. We try to match the API naming for names of config switches. We would not ever create System.Host API namespace or type. We do have System.Runtime.Loader namespace for this sort of functionality already.

A better name would be something like System.Runtime.Loader.UseRidGraph. Any chance we can still change it?

elinor-fung · 2023-05-22T17:51:44Z

Ah, good point. Put up #86598 to rename System.Runtime.Loader.UseRidGraph.

elinor-fung added the area-Host label Mar 29, 2023

ghost assigned elinor-fung Mar 29, 2023

elinor-fung force-pushed the rid-algorithmic branch from a462360 to 7be2573 Compare March 29, 2023 23:51

elinor-fung added 3 commits March 29, 2023 17:40

Make host look for RID-specific assets via a known list rather than r…

0e66ae3

…eading in the fallback graph

Fix reading includedFrameworks in RuntimeConfig test helper

cfccc73

Update tests

e2c826b

elinor-fung force-pushed the rid-algorithmic branch from 7be2573 to e2c826b Compare March 30, 2023 00:40

This was referenced Mar 30, 2023

WasmTestOnBrowser-System.* test failures in CI #83655

Closed

[wasm] interpreter timeouts when WebSocket closes unexpectedly #84101

Closed

am11 reviewed Mar 30, 2023

View reviewed changes

src/native/corehost/hostpolicy/deps_format.cpp Outdated Show resolved Hide resolved

Multi-package/asset test

602c48d

elinor-fung force-pushed the rid-algorithmic branch 3 times, most recently from d0d0c90 to c78e438 Compare March 30, 2023 22:09

Compile-time RID list

ca68832

elinor-fung force-pushed the rid-algorithmic branch from c78e438 to ca68832 Compare March 30, 2023 22:49

elinor-fung marked this pull request as ready for review March 31, 2023 03:17

elinor-fung requested a review from vitek-karas March 31, 2023 03:17

elinor-fung added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Mar 31, 2023

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Mar 31, 2023

vitek-karas reviewed Mar 31, 2023

View reviewed changes

src/installer/tests/HostActivation.Tests/DependencyResolution/RidAssetResolution.cs Show resolved Hide resolved

src/native/corehost/hostpolicy/deps_format.cpp Outdated Show resolved Hide resolved

src/native/corehost/hostpolicy/deps_format.h Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Apr 4, 2023

CI System.Text.RegularExpressions.Tests failure #84323

Closed

tmds reviewed Apr 6, 2023

View reviewed changes

am11 reviewed Apr 6, 2023

View reviewed changes

src/native/corehost/setup.cmake Outdated Show resolved Hide resolved

am11 reviewed Apr 6, 2023

View reviewed changes

src/native/corehost/hostpolicy/deps_format.cpp Show resolved Hide resolved

tmds reviewed Apr 6, 2023

View reviewed changes

Apply suggestions from code review

8851ebe

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>

elinor-fung commented Apr 10, 2023

View reviewed changes

src/native/corehost/hostpolicy/deps_format.cpp Outdated Show resolved Hide resolved

Update src/native/corehost/hostpolicy/deps_format.cpp

9f3d527

elinor-fung commented Apr 10, 2023

View reviewed changes

src/native/corehost/hostpolicy/deps_format.cpp Outdated Show resolved Hide resolved

elinor-fung added 2 commits April 10, 2023 13:11

Update src/native/corehost/hostpolicy/deps_format.cpp

80ce3f2

Use build-time list only

e48c8e7

elinor-fung force-pushed the rid-algorithmic branch from ad24e7c to e48c8e7 Compare May 9, 2023 17:29

build-analysis bot mentioned this pull request May 9, 2023

Could not find chrome snapshot folder for Linux_x64 #85949

Closed

elinor-fung requested review from tmds and vitek-karas May 10, 2023 15:37

vitek-karas approved these changes May 12, 2023

View reviewed changes

Renames and comments

41e753d

This was referenced May 15, 2023

Add test for successfully matching current RID when reading an empty RID graph #86250

Closed

[Breaking change]: Runtime host looks for RID-specific assets via a known list instead of RID graph by default dotnet/docs#35398

Closed

elinor-fung removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label May 18, 2023

elinor-fung merged commit b7cc2e5 into dotnet:main May 18, 2023

elinor-fung deleted the rid-algorithmic branch May 18, 2023 22:14

elinor-fung mentioned this pull request May 22, 2023

Rename setting for using RID graph to System.Runtime.Loader.UseRidGraph #86598

Merged

ghost locked as resolved and limited conversation to collaborators Jun 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make host look for RID-specific assets via a known list by default #84100

Make host look for RID-specific assets via a known list by default #84100

elinor-fung commented Mar 29, 2023 •

edited

Loading

ghost commented Mar 29, 2023

ghost commented Mar 31, 2023 •

edited by elinor-fung

Loading

elinor-fung commented Mar 31, 2023

richlander commented Mar 31, 2023

vitek-karas commented Mar 31, 2023

richlander commented Mar 31, 2023

elinor-fung commented Mar 31, 2023

elinor-fung commented Mar 31, 2023

richlander commented Mar 31, 2023

elinor-fung commented Apr 4, 2023

tmds Apr 6, 2023

elinor-fung Apr 6, 2023

tmds Apr 6, 2023 •

edited

Loading

tmds Apr 6, 2023 •

edited

Loading

elinor-fung commented May 10, 2023

vitek-karas commented May 12, 2023

vitek-karas left a comment

vitek-karas May 12, 2023

elinor-fung May 15, 2023

elinor-fung commented May 12, 2023

jkotas commented May 22, 2023 •

edited

Loading

elinor-fung commented May 22, 2023

Make host look for RID-specific assets via a known list by default #84100

Make host look for RID-specific assets via a known list by default #84100

Conversation

elinor-fung commented Mar 29, 2023 • edited Loading

ghost commented Mar 29, 2023

ghost commented Mar 31, 2023 • edited by elinor-fung Loading

elinor-fung commented Mar 31, 2023

richlander commented Mar 31, 2023

vitek-karas commented Mar 31, 2023

richlander commented Mar 31, 2023

elinor-fung commented Mar 31, 2023

elinor-fung commented Mar 31, 2023

richlander commented Mar 31, 2023

elinor-fung commented Apr 4, 2023

tmds Apr 6, 2023

Choose a reason for hiding this comment

elinor-fung Apr 6, 2023

Choose a reason for hiding this comment

tmds Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

tmds Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

elinor-fung commented May 10, 2023

vitek-karas commented May 12, 2023

vitek-karas left a comment

Choose a reason for hiding this comment

vitek-karas May 12, 2023

Choose a reason for hiding this comment

elinor-fung May 15, 2023

Choose a reason for hiding this comment

elinor-fung commented May 12, 2023

jkotas commented May 22, 2023 • edited Loading

elinor-fung commented May 22, 2023

elinor-fung commented Mar 29, 2023 •

edited

Loading

ghost commented Mar 31, 2023 •

edited by elinor-fung

Loading

tmds Apr 6, 2023 •

edited

Loading

tmds Apr 6, 2023 •

edited

Loading

jkotas commented May 22, 2023 •

edited

Loading