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

corehost: derive fallback rid from output rid. #82163

Merged
merged 8 commits into from
Feb 22, 2023

Conversation

tmds
Copy link
Member

@tmds tmds commented Feb 15, 2023

Fixes #81654.

This is a start. It handles non-portable non-Windows.

I wonder if we need a separate path to handle portable, or if this should work as-is.

I need some help with updating the Windows scripts. I can figure some things out by copy-pasting, but I don't know how to strip the architecture from the __OutputRid.

@vitek-karas @elinor-fung @ViktorHofer @am11 ptal.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 15, 2023
@ghost
Copy link

ghost commented Feb 15, 2023

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

Issue Details

Fixes #81654.

This is a start. It handles non-portable non-Windows.

I wonder if we need a separate path to handle portable, or if this should work as-is.

I need some help with updating the Windows scripts. I can figure some things out by copy-pasting, but I don't know how to strip the architecture from the __OutputRid.

@vitek-karas @elinor-fung @ViktorHofer @am11 ptal.

Author: tmds
Assignees: -
Labels:

area-Host, community-contribution

Milestone: -

src/coreclr/build-runtime.sh Outdated Show resolved Hide resolved
@am11
Copy link
Member

am11 commented Feb 15, 2023

I need some help with updating the Windows scripts. I can figure some things out by copy-pasting, but I don't know how to strip the architecture from the __OutputRid.

:: batch script - test.cmd

set __OutputRid=win10-arm64

for /f "delims=-" %%i in ("%__OutputRid%") do set __FallbackOS=%%i

echo %__FallbackOS%

outputs: win10

src/coreclr/build-runtime.cmd Outdated Show resolved Hide resolved
src/native/corehost/build.cmd Outdated Show resolved Hide resolved
@tmds
Copy link
Member Author

tmds commented Feb 16, 2023

CI is looking good for these changes.

Portable and non-portable share the same logic.

Also, the Windows has the same logic as the Unix script.
The Windows script could be simplified to: set __FallbackOS=win10 so it matches the FALLBACK_HOST_RID that was in the host for Windows.
I don't know what the preference is.

@vitek-karas @elinor-fung this is up for review.

@tmds
Copy link
Member Author

tmds commented Feb 21, 2023

I've addressed the feedback, and CI looks happy.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Thank you!

@elinor-fung elinor-fung merged commit ec2859d into dotnet:main Feb 22, 2023
@tmds
Copy link
Member Author

tmds commented Feb 22, 2023

And thank you and @am11 for reviewing!

@ghost ghost locked as resolved and limited conversation to collaborators Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Host community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fallback host rid is broken on non-portable builds
3 participants