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.

Sign up for GitHub

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

Jump to bottom

audit_glibc: Permit glibc 2.27, 2.31, or 2.35 and fix the error message #13618

Merged
merged 3 commits into from Jul 29, 2022
Merged

audit_glibc: Permit glibc 2.27, 2.31, or 2.35 and fix the error message #13618

merged 3 commits into from Jul 29, 2022

Conversation

sjackman
Copy link
Member

@sjackman sjackman commented Jul 29, 2022 •
edited

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The glibc version must be 2.23, as this is the version used by our CI on Linux. Glibc is for users who have a system Glibc with a lower version, which allows them to use our Linux bottles, which were compiled against system Glibc on CI.

@sjackman
audit_glibc: Fix the error message
291eacd 
"The glibc version must be 2.35" should have read
"The glibc version must be 2.23".
@BrewTestBot
Copy link
Member

Review period will end on 2022-08-01 at 16:31:27 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 29, 2022
@request-info
Copy link

request-info bot commented Jul 29, 2022

Please provide a better issue/pull request title and/or description!

@request-info request-info bot added the needs response Needs a response from the issue/PR author label Jul 29, 2022
@sjackman sjackman mentioned this pull request Jul 29, 2022
Closed
16 tasks
@sjackman
Copy link
Member Author

codecov/project — 68.68% (-5.39%) compared to 43a3470
This codecov report does not seem accurate.

Please provide a better issue/pull request title and/or description!
I like my title and/or description. 🤷‍♂️

@sjackman sjackman changed the title audit_glibc: Fix the error message audit_glibc: Permit glibc 2.35 and fix the error message Jul 29, 2022
@sjackman
Copy link
Member Author

sjackman commented Jul 29, 2022 •
edited

@carlocab I've updated this PR to allow glibc version 2.35 since you've approved this PR, which was previously only a bug fix. Once the bottling distribution is updated, we can change OS::CI_GLIBC_VERSION to 2.35 and remove this hack.

@sjackman
Copy link
Member Author 

Review period will end on 2022-08-01 at 16:31:27 UTC.
Error: Review period has not ended yet.

May I skip the review period for this PR?

carlocab
carlocab approved these changes Jul 29, 2022
View reviewed changes
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Works for me, but I'm not sure we've reached a consensus on glibc 2.35 yet.

@carlocab carlocab added the critical Critical change which should be shipped as soon as possible. label Jul 29, 2022
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 29, 2022
@carlocab
Copy link
Member

Marking as critical to unblock testing of Homebrew/homebrew-core#106837.

@sjackman
Copy link
Member Author

sjackman commented Jul 29, 2022 •
edited

Works for me, but I'm not sure we've reached a consensus on glibc 2.35 yet.

Yes, I agree, but it'll help keep the ball rolling to test the glibc 2.35 PR on CI. I'm happy to change the version of Glibc in this PR and the Glibc PR if we decide to go with a different version.

@sjackman sjackman changed the title audit_glibc: Permit glibc 2.35 and fix the error message audit_glibc: Permit glibc 2.27, 2.31, or 2.35 and fix the error message Jul 29, 2022
@sjackman
Copy link
Member Author

Works for me, but I'm not sure we've reached a consensus on glibc 2.35 yet.

Yes, I agree, but it'll help keep the ball rolling to test the glibc 2.35 PR on CI. I'm happy to change the version of Glibc in this PR and the Glibc PR if we decide to go with a different version.

I expanded the list of permitted Glibc versions to include those shipped by all three versions of Ubuntu LTS under consideration.

@sjackman sjackman merged commit 4f02c5d into Homebrew:master Jul 29, 2022
@sjackman sjackman deleted the sj/audit_glibc branch July 29, 2022 21:58
MikeMcQuaid
MikeMcQuaid reviewed Aug 1, 2022
View reviewed changes
Library/Homebrew/formula_auditor.rb
return unless @core_tap
return if formula.name != "glibc"
return if [OS::CI_GLIBC_VERSION, "2.27", "2.31", "2.35"].include?(formula.version.to_s)
Copy link
Member

Choose a reason for hiding this comment

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

Can this get a follow-up Ruby comment explaining why these particular versions?

Library/Homebrew/formula_auditor.rb
return if version == OS::CI_GLIBC_VERSION

problem "The glibc version must be #{version}, as this is the version used by our CI on Linux. " \
problem "The glibc version must be #{OS::CI_GLIBC_VERSION}, as this is the version used by our CI on Linux. " \
Copy link
Member

Choose a reason for hiding this comment

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

This message doesn't seem to match the return if logic above?

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid left review comments

@BrewTestBot BrewTestBot BrewTestBot approved these changes

@carlocab carlocab carlocab approved these changes

@iMichka iMichka Awaiting requested review from iMichka

@danielnachun danielnachun Awaiting requested review from danielnachun

Assignees
No one assigned
Labels
critical Critical change which should be shipped as soon as possible. needs response Needs a response from the issue/PR author outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sjackman @BrewTestBot @carlocab @MikeMcQuaid