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

Remove distros versions calculation #100580

Merged
merged 6 commits into from
Apr 30, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented Apr 3, 2024

Remove distro version calculation for non-portable builds. The user can specify the desired value via --outputrid option (which is already used by source-build).

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 3, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 3, 2024
@am11 am11 force-pushed the feature/rid-calculation branch 6 times, most recently from 6d27384 to fd57706 Compare April 3, 2024 12:59
@am11 am11 force-pushed the feature/rid-calculation branch from fd57706 to 172e94b Compare April 3, 2024 13:00
@am11
Copy link
Member Author

am11 commented Apr 3, 2024

@tmds, @elinor-fung, I was thinking something along these lines. It entails additional work such as:

This way, distro maintainers will be free to use their preferred convention without needing to maintain those details in dotnet infra.

@tmds
Copy link
Member

tmds commented Apr 3, 2024

For non-portable Linux build, I think we should keep the logic that determines a rid based on /etc/os-release. This calculates a sensible value in a consistent way.
Maintainers that want something different can take control by specifying --output-rid

@am11
Copy link
Member Author

am11 commented Apr 3, 2024

For non-portable Linux build, I think we should keep the logic that determines a rid based on /etc/os-release. This calculates a sensible value in a consistent way. Maintainers that want something different can take control by specifying --output-rid

That will defeat its purpose. Previous discussion was to remove these cases dotnet/arcade#14595 (comment).

I think either we should make it consistently agnostic of distro info for all platforms, or keep maintaining it (and keep accepting Add support for ___ linux PRs). I am leaning towards former.

@am11 am11 added area-Infrastructure and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 3, 2024
@tmds
Copy link
Member

tmds commented Apr 3, 2024

That will defeat its purpose. Previous discussion was to remove these cases dotnet/arcade#14595 (comment).

With that comment I meant: remove the special cases for specific Linux distros, like Oracle Linux.

I don't think we should force every distro maintainer to "choose" a rid and specify it using --output-rid when the script can calculate a sensible value to use.

@am11
Copy link
Member Author

am11 commented Apr 3, 2024

With that comment I meant: remove the special cases for specific Linux distros, like Oracle Linux.

Alright, you mean by using the same default for all distros: ${ID}.${VERSION} (and ${ID} when VERSION is empty), correct?

--output-rid

You named it --outputrid 58c3d52 😉

@am11 am11 changed the title Remove __DistroRid calculation Remove distros versions calculation Apr 3, 2024
@am11 am11 marked this pull request as ready for review April 3, 2024 15:00
@am11 am11 requested review from tmds and elinor-fung April 3, 2024 15:00
@tmds
Copy link
Member

tmds commented Apr 3, 2024

cc @dotnet/distro-maintainers

@am11
Copy link
Member Author

am11 commented Apr 3, 2024

@omajid, in case it wasn't obvious, this is dead code from VMR's perspective (TargetRid in dotnet/dotnet repo is always set which maps to --outputrid in this repo). Since .NET 8, runtime repo is pretty much agnostic of nonportable RIDs. This PR is a followup cleanup to avoid future "Add support for ___ linux" type of PRs.

@am11 am11 requested a review from jkoritzinsky April 23, 2024 19:39
@jkoritzinsky
Copy link
Member

As there's a change for tizen, cc: @dotnet/samsung.

Other than that, the fact that the VMR already provides a more comprehensive explicit feature here and that you're the maintainer for the sunOS-derived RIDs, this looks good to me.

I'll approve, but I'll wait a few days to merge in case any of our stakeholders have an issue that they want to raise.

Copy link
Member

@clamp03 clamp03 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.

@jkoritzinsky jkoritzinsky merged commit f9207e6 into dotnet:main Apr 30, 2024
145 of 152 checks passed
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
@mthalman
Copy link
Member

mthalman commented May 13, 2024

@am11 - It doesn't look like this has been applied yet. This should be applied to https://github.com/dotnet/sdk/blob/main/src/SourceBuild/content/build.sh. Note that this change has surfaced an issue in how we reference the produced output from the build: dotnet/sdk#40843. So once it's exposed as a build script parameter, we'd need to flow the desired RID into that from the build pipeline. I think this would only need to be done for Alpine as its version changes with each revision.

Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure 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.

6 participants