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

[CrossGen2] Build for any RID if building from source #54223

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

MichaelSimons
Copy link
Member

@MichaelSimons
Copy link
Member Author

Friendly ping, please review when convenient.

@agocke
Copy link
Member

agocke commented Jun 17, 2021

Should this check be placed somewhere else? Or is it only relevant for crossgen2?

@MichaelSimons
Copy link
Member Author

Should this check be placed somewhere else? Or is it only relevant for crossgen2?

I can't answer that. I am going to have to rely on you and the other repo owners/expert to provide the guidance here. As the patch comment indicates this change is necessary to support building crossgen2 for non-portable rids. Perhaps @dagood, the original patch author, can comment/give some more context.

@dagood
Copy link
Member

dagood commented Jun 18, 2021

There's no general rule being applied here AFAIK. (Not one that can be automatically applied through Arcade, anyway.) It just fixes an assumption the crossgen2 project is making about RID support.

The reason this hard-coded RID list exists in crossgen2 in the first place is that crossgen2 doesn't (didn't?) support all RIDs that Microsoft builds. Microsoft only builds portable builds (linux), so a hard-coded list is reasonable in that situation. A list doesn't work for source-build, which needs to build arbitrary non-portable RIDs like centos.8-x64 and banana.42-x64 with no unexpected feature gaps vs. portable RIDs.

Instead of applying this patch directly and turning the list on and off depending on whether we're in source-build, the logic could be improved! It could have a list of "good" portable RIDs, and check if the current RID is a descendant of any of the "good" RIDs in the RID graph. Then, there would be no need for source-build-specific behavior, because banana.42-x64 derives from linux-x64. But... I don't know how hard that is to implement. (Does MSBuild have a "RID X is derived from RID Y" method yet, like the "TFM X is supported by Y" one it got recently? Can we inject the knowledge that banana.42-x64 derives from linux-x64 into the build?)

Something that makes this situation easier is source-build support is very narrow compared to the Microsoft build. Every supported target runtime is compatible with linux-x64, linux-arm64, or osx-x64, I believe. (Although this will expand.)

@dagood
Copy link
Member

dagood commented Jun 18, 2021

FWIW, I didn't go through all of that when I wrote the commit. The logic was there way back when I helped implement the crossgen2 pack initially and it supported many fewer runtimes: https://github.com/dotnet/runtime/blob/d8232a917551e343290572947547e14f47350c0c/src/installer/pkg/projects/netcoreapp/pkg/crossgenRIDs.props

It's possible the list of runtimes should be removed entirely. I'm not actually sure which RIDs are being excluded by it at this point.

@ViktorHofer
Copy link
Member

cc @ericstj for Davis's RID specific question above.

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@ericstj
Copy link
Member

ericstj commented Jun 18, 2021

NuGet implements RID mapping types, the definitions of compatible RIDs exist in runtime.json. I'm not aware of a property function to determine RID compatibility, but a task to do so wouldn't be much. You could look at the MS.NETCore.Platforms project for some inspiration. Also, be aware we did some work to make source-build + RIDs a bit easier.

@MichaelSimons
Copy link
Member Author

How would the reviewers like to proceed? I see two options:

  1. Merge this PR and log an issue to improve this logic to see if the current RID is a descendant of any of the "good" RIDs.
  2. Implement the suggested improvement in this PR.

Either way I would like to see the runtime team take ownership of the improvement. Thoughts?

@ViktorHofer
Copy link
Member

Merge this PR and log an issue to improve this logic to see if the current RID is a descendant of any of the "good" RIDs.

I don't own that component but I think that should be fine and unblocks source build meanwhile.

@ViktorHofer
Copy link
Member

Merging this in as I believe we reached consensus. @MichaelSimons can you please make sure to log an issue for the RID selection?

@ViktorHofer ViktorHofer merged commit 4f3364e into dotnet:main Jun 21, 2021
@MichaelSimons MichaelSimons deleted the ArPow-PatchRemoval-19 branch June 21, 2021 17:50
@MichaelSimons
Copy link
Member Author

I logged #54513 to track improving the hard coded RID list to better support building for arbitrary RIDs.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 21, 2021
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