-
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
Move portable RID graph into runtime and clean-up #92211
Merged
Merged
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
2f5eb8f
Move portable RID graph into runtime and clean-up
ViktorHofer bf9d5ea
Update README and delete test
ViktorHofer 571dc09
Fix RID graph update when the key already exists
ViktorHofer 277f617
Update src/libraries/Microsoft.NETCore.Platforms/readme.md
ViktorHofer a58113c
Merge branch 'main' into MicrosoftNETCorePlatformsRID
ViktorHofer cbc3677
Update src/libraries/Microsoft.NETCore.Platforms/readme.md
ViktorHofer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
39 changes: 0 additions & 39 deletions
39
src/libraries/Microsoft.NETCore.Platforms/Microsoft.NETCore.Platforms.sln
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
dotnet/designs#260 (comment)
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.
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
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.
More specifically: the build automatically takes care of adding the non-portable rid for the OS during source-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 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).
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.
At least for .NET 8, it doesn't seem necessary for the non-portable rid that gets source-built to be in the graph.
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 ofunix
. It's more similar to a portable rid than to a non-portable rid.Maybe some changes are possible to reduce the verbosity/repetition.
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.
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).
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 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.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.
Exactly. ODBC is tracked via #53900 but there are probably also inbox libraries that would benefit from a build platform simplification.