Skip to content

Conversation

@MarkpageBxl
Copy link
Contributor

@MarkpageBxl MarkpageBxl commented Aug 18, 2022

Fixes #74159
Add support for Rocky Linux RIDs on the 6.0 branch.

@ghost ghost added area-Infrastructure-libraries community-contribution Indicates that the PR has been added by a community member labels Aug 18, 2022
@ghost
Copy link

ghost commented Aug 18, 2022

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

Issue Details

Add support for Rocky Linux RIDs on the 6.0 branch.

Author: mhlindstr
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@MarkpageBxl MarkpageBxl force-pushed the release/6.0 branch 2 times, most recently from 0f506eb to dd3bcdd Compare August 18, 2022 18:41
@teo-tsirpanis teo-tsirpanis added this to the 6.0.x milestone Aug 18, 2022
@ViktorHofer
Copy link
Member

cc @carlossanlop

Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

The changes seem to be roughly the same as in main, aside from the double brackets, and the package authoring changes that I don't understand.

@wfurt do you approve of this change as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems both single or double square brackets are allowed to evaluate expressions. I mention it because the main version only had single square brackets.

Source: http://mywiki.wooledge.org/BashFAQ/031

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I didn't even notice that single brackets where used on main, I only wished to add rocky as a valid alternative to the existing test. It doesn't have any functional impact here, as double brackets only give access to the extended Bash test syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems double brackets are more correct. https://stackoverflow.com/a/2188369/8816314

If we have to do any corrections, we would have to add them to main, not here. But I think single brackets work too, if I'm understanding those two sources correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Single brackets are compatible with /bin/sh (e.g. standard POSIX tests), whereas double brackets are a bash extension. I assume it has since been decided to not use bash extensions on main, but I would leave it as it is on servicing.

@carlossanlop carlossanlop added the needs-author-action An issue or pull request that requires more info or actions from the author. label Sep 7, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Sep 8, 2022
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Package authoring suggestion addressed. LGTM.

@carlossanlop carlossanlop merged commit 2cfbeab into dotnet:release/6.0 Sep 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 8, 2022
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.

6 participants