Skip to content

Move portable RID graph into runtime and clean-up #92211

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

Merged
merged 6 commits into from
Sep 19, 2023

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Sep 18, 2023

See discussion in #86391 for more details

  1. Move portable RID graph into runtime
  2. Allow updates to both the non-portable and portable RID graphs under source build.
  3. Clean-up project and remove hacks

cc @elinor-fung @akoeplinger @jkotas @am11

@ghost ghost assigned ViktorHofer Sep 18, 2023
@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 18, 2023
@ViktorHofer ViktorHofer force-pushed the MicrosoftNETCorePlatformsRID branch from b421a72 to 51e0d4e Compare September 18, 2023 09:54
1. Move portable RID graph into runtime
2. Allow updates to both the non-portable and portable RID graphs under
   source build.
3. Clean-up project and remove hacks
@ViktorHofer ViktorHofer force-pushed the MicrosoftNETCorePlatformsRID branch from 51e0d4e to 2f5eb8f Compare September 18, 2023 09:56
- `OmitRIDs`: A list of strings delimited by semi-colons that represent RIDs calculated from this RuntimeGroup that should be omitted from the RuntimeGraph. These RIDs will not be referenced nor defined.
- `OmitRIDDefinitions`: A list of strings delimited by semi-colons that represent RIDs calculated from this RuntimeGroup that should be omitted from the RuntimeGraph. These RIDs will not be defined by this RuntimeGroup, but will be referenced: useful in case some other RuntimeGroup (or runtime.json template) defines them.
- `OmitRIDReferences`: A list of strings delimited by semi-colons that represent RIDs calculated from this RuntimeGroup that should be omitted from the RuntimeGraph. These RIDs will be defined but not referenced by this RuntimeGroup.
The RID graphs are frozen and shouldn't be updated anymore. Build from source automatically adds the non-portable distro RID encoded via the `OutputRID` property into the RID graph which allows build tools to target that RID.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The RID graphs are frozen and shouldn't be updated anymore. Build from source automatically adds the non-portable distro RID encoded via the `OutputRID` property into the RID graph which allows build tools to target that RID.
The RID graphs should be only updated with new base OSes. The RID graphs shouldn't be updated with new OS flavor- and version-specific RIDs anymore. Build from source automatically adds the non-portable distro RID encoded via the `OutputRID` property into the RID graph which allows build tools to target that RID.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

@ViktorHofer ViktorHofer Sep 18, 2023

Choose a reason for hiding this comment

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

Makes sense, thanks. Do we somewhere already capture that we don't need to update the RID graph for building an unknown Unix distribution as part of source build? cc @tmds

Copy link
Member

Choose a reason for hiding this comment

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

The RID graphs shouldn't be updated with new OS flavor- and version-specific RIDs anymore.

More specifically: the build automatically takes care of adding the non-portable rid for the OS during source-build.

Copy link
Member Author

@ViktorHofer ViktorHofer Sep 19, 2023

Choose a reason for hiding this comment

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

The one difficulty that I don't think many are aware of is that the libraries TFM infrastructure reads from the RID graph (portable by default) to calculate the compatibility mapping when referencing other projects. That's what powers TFMs like net8.0-unix.

As the Microsoft.NETCore.Platforms package builds too late, we can't leverage the source built updated RID graph. Therefore we still require manual additions to the RID graph just for the sake of being able to target RIDs in our libraries.

This problem would go away if libraries would use OS runtime detection instead of at build time. Looking at the haiku PR, there aren't just a handful of these libraries. Collapsing build time platforms is general goodness as it reduces the build graph (makes evaluation, restore, build, pack, ... faster).

Copy link
Member

Choose a reason for hiding this comment

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

As the Microsoft.NETCore.Platforms package builds too late, we can't leverage the source built updated RID graph. Therefore we still require manual additions to the RID graph just for the sake of being able to target RIDs in our libraries.

At least for .NET 8, it doesn't seem necessary for the non-portable rid that gets source-built to be in the graph.

This problem would go away if libraries would use OS runtime detection instead of at build time. Looking at the haiku PR, there aren't just a handful of these libraries. Collapsing build time platforms is general goodness as it reduces the build graph (makes evaluation, restore, build, pack, ... faster).

Yea, we shouldn't create rid specific libs when they are not needed. I imagine most of them exists for rid specific assets, so they are needed.

Haiku is different from adding the non-portable rid like we're doing with source-build.
haiku a direct base of unix. It's more similar to a portable rid than to a non-portable rid.

Maybe some changes are possible to reduce the verbosity/repetition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, we shouldn't create rid specific libs when they are not needed. I imagine most of them exists for rid specific assets, so they are needed.

System.Data.Odbc is a prime example of a library that targets way too many platforms. I would imagine that all the Unix derived platforms could be collapsed into a single build by using runtime checks (which would then get optimized away when the linker is used in the consuming app).

Copy link
Member

Choose a reason for hiding this comment

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

System.Data.Odbc is a prime example of a library that targets way too many platforms. I would imagine that all the Unix derived platforms could be collapsed into a single build by using runtime checks (which would then get optimized away when the linker is used in the consuming app).

I took a quick look. It seems the split is mostly for finding the platform specific name of the Odbc32 library. Perhaps we can do something with NativeLibrary instead. @ViktorHofer may be you can create an issue to investigate it further.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. ODBC is tracked via #53900 but there are probably also inbox libraries that would benefit from a build platform simplification.

@ViktorHofer
Copy link
Member Author

@dsplaisted @agocke @elinor-fung can you please review the changes? Is that we want?

@tmds do we actually want to update both RID graphs for source build or would the portable one be enough?

@tmds
Copy link
Member

tmds commented Sep 19, 2023

@tmds do we actually want to update both RID graphs for source build or would the portable one be enough?

We don't need to update the non-portable graph, as the plan is to remove it, right? (#90000).

@ViktorHofer
Copy link
Member Author

We don't need to update the non-portable graph, as the plan is to remove it, right?

I'm not sure about that anymore as

depends on it. cc @vitek-karas who made that change

@vitek-karas
Copy link
Member

It's true that we will need to keep the non-portable RID graph as it was in .NET 8 around, as it needs to be included in the Microsoft.NETCore.App framework. My change was the most expedient way to fix a code integration problem.

I must admit I don't know where SDK gets the RID graph from when it generates a self-contained app. Wherever that is, we probably need to keep it there for now.

@tmds
Copy link
Member

tmds commented Sep 19, 2023

as it needs to be included in the Microsoft.NETCore.App framework

This doesn't cause something to get included/excluded. It's a choice between the legacy graph and the new portable graph.

If you need to set this, then something is broken when using the new portable graph.

Do you know what opting into the legacy graph is supposed to fix?

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Sep 19, 2023

I must admit I don't know where SDK gets the RID graph from when it generates a self-contained app. Wherever that is, we probably need to keep it there for now.

The SDK gets it from runtime via the Microsoft.NETCore.Platforms non-shipping transport package (the project file that is changed in this PR).

@vitek-karas
Copy link
Member

If you need to set this, then something is broken when using the new portable graph.

Do you know what opting into the legacy graph is supposed to fix?

Even .NET 8+ apps can opt into the "old" behavior where the host reads the RID graph from .deps.json (instead of relying on the computed fallback list). If the app is self-contained, then SDK needs to write the RID graph into the .deps.json of the app (and Viktor just replied as to where that RID graph comes from). If the app is framework dependent, then the RID graph needs to come from the core framework - which is Microsoft.NETCore.App. So Microsoft.NETCore.App.deps.json needs to contain the full RID graph (the "old" one) even in .NET 8+. By default host will ignore it, but if the app opts into the backward compat behavior host will read it (just like it does in 7).

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Sep 19, 2023

Even .NET 8+ apps can opt into the "old" behavior where the host reads the RID graph from

It sounded like we are considering removing that switch in .NET 9 or future. I assume that the need for keeping the non portable RID graph alive would then go away?

@vitek-karas
Copy link
Member

It sounded like we are considering removing that switch in .NET 9 or future. I assume that the need for keeping the non portable RID graph alive would then go away?

That's a good point. I must admit don't know if we already made a plan to deprecate the old RID graph. @elinor-fung would know best.

@tmds
Copy link
Member

tmds commented Sep 19, 2023

That's a good point. I must admit don't know if we already made a plan to deprecate the old RID graph. @elinor-fung would know best.

#90000 was created for removing the legacy rid graph.

@vitek-karas
Copy link
Member

Looks like I'm out of the loop then. If we want to remove the RID graph from the framework I would expect test failures in the host tests (that was the reason I added the change to the framework build).

@elinor-fung
Copy link
Member

elinor-fung commented Sep 19, 2023

It sounded like we are considering removing that switch in .NET 9 or future. I assume that the need for keeping the non portable RID graph alive would then go away?

We don't have a specific release set for removing the switch. I think .NET 9 is a bit too soon though - my thought had been having at least a release of warning that the switch would go away. But yes, once the switch is removed, the non-portable graph could go way.

@tmds
Copy link
Member

tmds commented Sep 19, 2023

If the legacy graph is maintained for .NET 9, then we should to update it as part of source-build too, to maintain the legacy behavior.

@ViktorHofer
Copy link
Member Author

If the legacy graph is maintained for .NET 9, then we should to update it as part of source-build too, to maintain the legacy behavior.

Agreed. Perfect, that's already the current state of the PR. Just waiting for some reviews.

@ViktorHofer ViktorHofer added area-Infrastructure-libraries and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 19, 2023
@ghost
Copy link

ghost commented Sep 19, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

See discussion in #86391 for more details

  1. Move portable RID graph into runtime
  2. Allow updates to both the non-portable and portable RID graphs under source build.
  3. Clean-up project and remove hacks

cc @elinor-fung @akoeplinger @jkotas @am11

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: -

@ViktorHofer ViktorHofer merged commit 3b9b4fd into main Sep 19, 2023
@ViktorHofer ViktorHofer deleted the MicrosoftNETCorePlatformsRID branch September 19, 2023 21:29
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Sep 19, 2023

@dsplaisted can you please follow-up on deleting the non portable RID graph from the sdk repo (probably including the task) and consuming it from the Microsoft.NETCore.Platforms transport package?

edit tracked by dotnet/sdk#35750

@ghost ghost locked as resolved and limited conversation to collaborators Oct 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants