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

Move HOMEBREW_SIMULATE_MACOS_ON_LINUX handling to SimulateSystem #13616

Merged
merged 3 commits into from
Aug 5, 2022
Merged

Move HOMEBREW_SIMULATE_MACOS_ON_LINUX handling to SimulateSystem #13616

merged 3 commits into from
Aug 5, 2022

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Jul 28, 2022

Follow-up to #13599

This PR moves the handling of HOMEBREW_SIMULATE_MACOS_ON_LINUX into SimulateSystem. Since the env var is accomplishing the same thing as simulating using the SimulateSystem methods, it makes sense to handle it in the same place instead of needing to have separate conditionals for the two possible ways of simulating. When HOMEBREW_SIMULATE_MACOS_ON_LINUX is set, we assume that we're simulating a generic macOS version, so Homebrew::SimulateSystem.os returns :macos. I've also set it up so that simulations set up using SimulateSystem methods take precedence over the environment variable, but Homebrew::SimulateSystem.clear will not clear the simulation since the env var is still set.

In this PR, if the env var is set while actually on macOS, it is essentially a no-op. Homebrew::SimulateSystem.os will be nil and Homebrew::SimulateSystem.current_os will return the symbol for whatever version of macOS is actually being used. Should setting this variable on macOS use the generic :macos instead? I don't think I see much value in this, but it's a possibility.

@BrewTestBot
Copy link
Member

Review period will end on 2022-07-29 at 19:10:55 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 28, 2022
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.

Love this cleanup!

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

Review period ended.

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! One optional comment.

@Rylan12 Rylan12 merged commit d4ddfb8 into Homebrew:master Aug 5, 2022
@Rylan12 Rylan12 deleted the simulate-macos-on-linux branch August 5, 2022 14:23
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2022
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.

3 participants