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

Microsoft.NETCore.Platforms: support adding rids with '-' in the base part. #84413

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

tmds
Copy link
Member

@tmds tmds commented Apr 6, 2023

Currently when trying to add a rid like 'linux-musl-x64' the rid is not understood to be base = 'linux-musl', arch = 'x64'.

Instead the parser considers a potential optional qualifier. This causes the rid to be parsed as base = 'linux', arch = 'musl', and qualifier = 'x64'.

We know the rids being added won't have a qualifier. If we take this into account while parsing, we can parse the rid correctly.

@ericstj ptal.

cc @ayakael

… part.

Currently when trying to add a rid like 'linux-musl-x64'
the rid is not understood to be base = 'linux-musl', arch = 'x64'.

Instead the parser considers a potential optional qualifier.
This causes the rid to be parsed as base = 'linux', arch = 'musl',
and qualifier = 'x64'.

We know the rids being added won't have a qualifier. If we take
this into account while parsing, we can parse the rid correctly.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 6, 2023
@ghost
Copy link

ghost commented Apr 6, 2023

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

Issue Details

Currently when trying to add a rid like 'linux-musl-x64' the rid is not understood to be base = 'linux-musl', arch = 'x64'.

Instead the parser considers a potential optional qualifier. This causes the rid to be parsed as base = 'linux', arch = 'musl', and qualifier = 'x64'.

We know the rids being added won't have a qualifier. If we take this into account while parsing, we can parse the rid correctly.

@ericstj ptal.

cc @ayakael

Author: tmds
Assignees: -
Labels:

area-Infrastructure-libraries, community-contribution

Milestone: -

@tmds tmds requested a review from ericstj April 6, 2023 11:31
@tmds
Copy link
Member Author

tmds commented Apr 6, 2023

Interesting to know: os-release allows - in the base part:

ID=
A lower-case string (no spaces or other characters outside of 0–9, a–z, ".", "_" and "-") identifying the operating system

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM modulo a nit. Thank you for the fix!

…ection.cs

Co-authored-by: Eric StJohn <ericstj@microsoft.com>
@tmds
Copy link
Member Author

tmds commented Apr 6, 2023

Thank you too for helping to get it fixed quickly @ericstj.

@ericstj
Copy link
Member

ericstj commented Apr 6, 2023

Build failures are reported and unrelated to this PR.

@ericstj ericstj merged commit 238e3d3 into dotnet:main Apr 6, 2023
ayakael added a commit to ayakael/runtime that referenced this pull request Apr 7, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2023
@carlossanlop
Copy link
Member

carlossanlop commented May 15, 2023

Do we need a 7.0 backport?
Edit: Confirmed with Eric that yes, we need one.

@carlossanlop
Copy link
Member

/backport to release/7.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/4985461853

@github-actions github-actions bot unlocked this conversation May 15, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries 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.

3 participants