-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Deprecate OS::Mac
on Linux
#16224
Deprecate OS::Mac
on Linux
#16224
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! This approach makes a lot of sense to me. As failing CI indicates: there's at least one formula and brew test-bot
that will need updated before this can be merged.
Also, good to keep everything here for now for testing but may want to merge this with the odeprecated
commented out once this is all 🟢 so it can get some testing before we do the actual deprecation before a new major or minor release.
The test-bot issue should be fixed. The One possible way to fix it would be what I've done in this commit. Since the offending lines only deal with Another possibility is to simply add a begin
macos_version = MacOS.full_version.major_minor # Ignore bugfix/security updates
on_catalina :or_older do
macos_version = MacOS.full_version
end
rescue NameError
macos_version = nil
end This avoids the |
We likely don't need And it's debatable whether we strictly speaking we need that either. We only need one header file from |
The reason I think we still need it is that the minor version affects which resource is downloaded (e.g. |
Technically speaking yes. Though we only provide one bottle per major OS in the end and there's never been any issues there. |
All CI is 🟢. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good to merge after these.
I've added a rubocop to ensure that The only tricky thing is that |
We run that usually on Ubuntu 22.04 anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here, thanks again @Rylan12!
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Closes #15343
This PR deprecates the
OS::Mac
methods that are currently defined on Linux. I've gone through each instance that I can find where one of the methods was exposed to Linux code and refactored it. I'm marking this as a draft for now to gauge whether this is the right approach, and I will do more thorough testing.