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

Add missing OL releases to RID graph #81115

Closed
wants to merge 3 commits into from

Conversation

Mr-Tao
Copy link

@Mr-Tao Mr-Tao commented Jan 24, 2023

In PR, I’ve tried to address comments that #66783 received.

I’m very much in favour of adding only major versions to the RID graph, as @omajid suggested in #66783 (comment). In Oracle Linux, we have been adding minor versions for Dotnet versions up to 7.9 and 8.7 so far.

Since all minor versions should be compatible, dropping all minor versions from the graph may be possible. But I see that this didn’t happen with rhel minor versions already present in the graph, which lets me think about whether there are potential repercussions from dropping them. 🤔 I don’t know for sure, so I have kept minor versions.

But since we are now adding OL, we need the parent RHEL versions (is this a strict requirement? Can anyone confirm) #66783 (comment)

I tried generating the graph without adding respective rhel RIDs. The task failed as expected, complaining about missing parents.

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

ghost commented Jan 24, 2023

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

Issue Details

In PR, I’ve tried to address comments that #66783 received.

I’m very much in favour of adding only major versions to the RID graph, as @omajid suggested in #66783 (comment). In Oracle Linux, we have been adding minor versions for Dotnet versions up to 7.9 and 8.7 so far.

Since all minor versions should be compatible, dropping all minor versions from the graph may be possible. But I see that this didn’t happen with rhel minor versions already present in the graph, which lets me think about whether there are potential repercussions from dropping them. 🤔 I don’t know for sure, so I have kept minor versions.

But since we are now adding OL, we need the parent RHEL versions (is this a strict requirement? Can anyone confirm) #66783 (comment)

I tried generating the graph without adding respective rhel RIDs. The task failed as expected, complaining about missing parents.

Author: Mr-Tao
Assignees: -
Labels:

area-Host

Milestone: -

@omajid
Copy link
Member

omajid commented Jan 24, 2023

I am happy there are no minor-version RIDs for OL 9 🎉

Since all minor versions should be compatible, dropping all minor versions from the graph may be possible. But I see that this didn’t happen with rhel minor versions already present in the graph, which lets me think about whether there are potential repercussions from dropping them. thinking I don’t know for sure, so I have kept minor versions.

I think it's a compatibility concern. If a user has used the RID in the past and we drop the RID in the graph, that might break applications, or at prevent the build/publish commands that were previously working from continuing to work.

I don't know if these concerns still apply when adding it to repo here, because those RIDs weren't present previously so they wouldn't be treated as "removed"/"breaking" if they just not added.

So maybe we can leave out the minor version RIDs from this PR? Will this really impact customers given the RID graph on nuget.org didn't have these versions already?

Anyway, I wont block the addition of RHEL minor version IDs here. I don't see a harm.

@richlander richlander mentioned this pull request Mar 10, 2023
@agocke
Copy link
Member

agocke commented Mar 13, 2023

While we're contemplating deprecating the whole RID graph (#83246), I'm going to close this for now. We can re-open if we end up not going in that direction

@Mr-Tao
Copy link
Author

Mr-Tao commented Mar 15, 2023

Thanks for taking the time to review the PR, @agocke. I like the idea of getting rid of RIDs very much. We are already following Red Hat by not introducing any more RIDs for minor versions of Oracle Linux 9. At the same time we still need to maintain the set of RIDs added on OL8 and OL7 to be able to build dotnet 3, 6 and 7.

By including this change the Oracle Linux support would get on par with support for RHEL, Rocky and others. The change also does not add any complexity to the process of deprecation of RIDs when and if the time comes.

@ViktorHofer
Copy link
Member

ViktorHofer commented Mar 20, 2023

@richlander until we execute on the RID plan changes, should we keep accepting these changes or close them?

@tmds
Copy link
Member

tmds commented Mar 21, 2023

We are already following Red Hat by not introducing any more RIDs for minor versions of Oracle Linux 9.

You can't omit these minor versions until you update .NET to ignore the minor.

That means adding code here:

pal::string_t normalize_linux_rid(pal::string_t rid)
{
pal::string_t rhelPrefix(_X("rhel."));
pal::string_t alpinePrefix(_X("alpine."));
pal::string_t rockyPrefix(_X("rocky."));

(and similar for .NET 6).

@ViktorHofer
Copy link
Member

ping @richlander regarding my above question

@carlossanlop
Copy link
Member

Reopening this PR to continue the conversation. I closed the release/6.0 PR version of this change, since we would first need to merge the main version before taking a servicing version: #83681

Ping @richlander

@Mr-Tao
Copy link
Author

Mr-Tao commented May 1, 2023

You can't omit these minor versions until you update .NET to ignore the minor.

That means adding code here:

pal::string_t normalize_linux_rid(pal::string_t rid)
{
pal::string_t rhelPrefix(_X("rhel."));
pal::string_t alpinePrefix(_X("alpine."));
pal::string_t rockyPrefix(_X("rocky."));

(and similar for .NET 6).

Thanks for pointing this out, @tmds. Do you think that there’s anything else that needs to be added besides this change? https://github.com/dotnet/runtime/pull/81115/files#diff-b2bd8511e88327d8f146bc460e79182cc6826e2b90039dffc4f5f89c5467d12a

</RuntimeGroup>
<RuntimeGroup Include="rhel">
<Parent>linux</Parent>
<Architectures>x64;arm64</Architectures>
<Versions>8;8.0;8.1</Versions>
<Versions>8;8.0;8.1;8.2;8.3;8.4;8.5;8.6;8.7</Versions>
Copy link
Member

Choose a reason for hiding this comment

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

With the addition of changes to pal.unix.cpp, are these still necessary? What happens if you don't add all the new 8 RIDs to ol and rhel?

Copy link
Author

Choose a reason for hiding this comment

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

I suppose that dotnet would build fine. If this is safe, why does RedHat still keep minors for 8? Could this be an issue for customer applications? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

If this is safe, why does Red Hat still keep minors for 8

It's mostly due to backwards compatibility. We added them to the RID graph for 8 before we finished all the fixes (eg, the pal.unix.cpp stuff), and now we can't remove them.

No one should be depending on the minor versions. It makes everyone's life harder.

@MichaelSimons
Copy link
Member

@agocke, @carlossanlop, @richlander - What is the plan for requests like this to update the RID graph? I know there is the proposed RID plan. What is the timeline for implementing it? This issue is affecting folks wanting to source-build .NET.

@agocke
Copy link
Member

agocke commented Jun 26, 2023

RID changes are in and breaking change is filed: https://learn.microsoft.com/en-us/dotnet/core/compatibility/deployment/8.0/rid-asset-list

RID graph is now frozen.

@agocke agocke closed this Jun 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 27, 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.

7 participants