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

Deprecate OS::Mac on Linux #16224

Merged
merged 6 commits into from Nov 23, 2023
Merged

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Nov 16, 2023

  • 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?

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.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a 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.

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 21, 2023

The test-bot issue should be fixed. The lsyncd one is a little more tricky.

One possible way to fix it would be what I've done in this commit.

Since the offending lines only deal with resource blocks which aren't currently included in the API, not having accurate information doesn't hurt us. I don't like it though since if resource blocks get added in the future, it wouldn't work. I'd love to get some other thoughts on the best way to handle this.

Another possibility is to simply add a rescue to catch the method-undefined error like this:

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 # rubocop:{dis,en}able so maybe is better... thoughts?

@Bo98
Copy link
Member

Bo98 commented Nov 21, 2023

We likely don't need MacOS.full_version in lsyncd anymore, so it could be a series of on_el_capitan, on_sierra, etc.

And it's debatable whether we strictly speaking we need that either. We only need one header file from xnu and the one we use isn't updated often, but is probably safer as per-major OS.

@Rylan12
Copy link
Member Author

Rylan12 commented Nov 21, 2023

We likely don't need MacOS.full_version in lsyncd anymore, so it could be a series of on_el_capitan, on_sierra, etc.

The reason I think we still need it is that the minor version affects which resource is downloaded (e.g. 13.1 it uses version 8792.61.2 and on 13.0 it uses version 8792.41.9). I don't know anything about lsyncd itself to know if it could be done differently to avoid this

@Bo98
Copy link
Member

Bo98 commented Nov 21, 2023

Technically speaking yes. Though we only provide one bottle per major OS in the end and there's never been any issues there.

@MikeMcQuaid
Copy link
Member

All CI is 🟢.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a 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.

Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
Library/Homebrew/os/linux.rb Outdated Show resolved Hide resolved
@Rylan12
Copy link
Member Author

Rylan12 commented Nov 23, 2023

I've added a rubocop to ensure that MacOS references in formulae only occur inside on_macos blocks, on_{macos_version} blocks, def install, def post_install, test blocks, and service blocks. This, in conjunction with the deprecations (now commented out) should help guide people away from MacOS.

The only tricky thing is that brew readall does not reliably catch issues here, since the deprecation warning will only appear when loading the formula on a Linux machine. To still show the deprecations when simulating Linux on macOS, I added deprecations to the macOS implementations as well that only trigger when simulating. I've added these as comments in b7532a8 and can remove if we don't want it.

@Rylan12 Rylan12 marked this pull request as ready for review November 23, 2023 02:25
@Bo98
Copy link
Member

Bo98 commented Nov 23, 2023

The only tricky thing is that brew readall does not reliably catch issues here, since the deprecation warning will only appear when loading the formula on a Linux machine.

We run that usually on Ubuntu 22.04 anyway.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a 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!

@MikeMcQuaid MikeMcQuaid merged commit e93b0fc into Homebrew:master Nov 23, 2023
34 checks passed
@Rylan12 Rylan12 deleted the remove-os-mac-on-linux branch November 23, 2023 18:50
@github-actions github-actions bot added the outdated PR was locked due to age label Dec 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate OS::Mac/MacOS usage on Linux
3 participants