`SimulateSystem` improvements #13599

Rylan12 · 2022-07-23T01:22:26Z

This PR improves Homebrew::SimulateSystem by making its methods more useful in a few ways. First, it replaces the macos? and linux? methods with treat_as_macos? and treat_as_linux? methods. The difference is that these methods are true/false based on both the simulated OS and the actual OS (before it only accounted for the simulated one). For example, if I'm running on macOS and run Homebrew::SimulateSystem.os = :linux, then treat_as_linux? will be true because I want to treat my current system as a Linux system. If I'm on macOS and haven't set a simulated OS, then treat_as_linux? will be false and treat_as_macos? will be true (since I want to treat my current system as a macOS system like it actually is). These methods are designed to be a single call to determine how to treat the current system without needing to deal with the confusing combination of checks you'd need otherwise.

Also, this PR adds current_arch and current_os to return a symbol representing the current OS and arch, regardless of whether this comes from a simulation or is the actual value. For example, if I'm on ARM, current_arch will return :arm. If I then run Homebrew::SimulateSystem.arch = :intel, current_arch will now return :intel.

As always, I'm open to suggestions on the names of these methods.

I've switched the OnSystem module to use these new methods which makes them a bit cleaner. I've also extended the use of SimulateSystem to uses_from_macos and ignore_missing_libraries.

BrewTestBot · 2022-07-23T01:22:48Z

Review period will end on 2022-07-26 at 00:00:00 UTC.

odinson94666

Yeah

MikeMcQuaid

New naming makes sense to me but have one suggestion in case you prefer it.

Would love to try and move more stuff to extend/os somehow but that shouldn't block this PR and could be a cleanup effort when the project is otherwise mostly done.

MikeMcQuaid · 2022-07-25T07:48:18Z

Library/Homebrew/software_spec.rb

+    # Linux simulating macOS. Assume oldest macOS version.
+    return if Homebrew::EnvConfig.simulate_macos_on_linux? && !bounds.key?(:since)


This makes me wonder about whether this logic should be in extend/os/linux; it feels a bit weird to be introducing OS-condition logic here (and in formula.rb).

MikeMcQuaid · 2022-07-25T07:48:47Z

Library/Homebrew/extend/on_system.rb


    raise ArgumentError, "Invalid OS condition: #{os_name.inspect}" unless MacOSVersions::SYMBOLS.key?(os_name)

    if or_condition.present? && [:or_newer, :or_older].exclude?(or_condition)
      raise ArgumentError, "Invalid OS `or_*` condition: #{or_condition.inspect}"
    end

-    return false if Homebrew::SimulateSystem.linux? || (Homebrew::SimulateSystem.none? && OS.linux?)
+    return false if Homebrew::SimulateSystem.treat_as_linux?


Suggested change

return false if Homebrew::SimulateSystem.treat_as_linux?

return false if Homebrew::SimulateSystem.simulating_linux?

is another option

I think I prefer the original because if we aren't simulating Linux but are actually on it, we still need to return true. I can see the word "simulate" being misleading

Hmm, I really don't understand this then I think 😅. Why not do a OS.linux? check?

If we do this:

return false if Homebrew::SimulateSystem.os == :linux || OS.linux?

Then the condition will be true when we're simulating macOS on Linux (because Homebrew::SimulateSystem.os will be :macos (or a specific version) and OS.linux? will still be true). We only want this condition to be true when we're simulating Linux or we are on Linux and not simulating anything. So, the full check needs to be:

return false if Homebrew::SimulateSystem.os == :linux || (Homebrew::SimulateSystem.os.blank? && OS.linux?)

The Homebrew::SimulateSystem.treat_as_linux? method is just a shortcut for this longer conditional.

I feel like we want some sort of OS.linux_no_really_not_simulated?

TBH, I don't see why we don't want OS.mac? and OS.linux? to lie when we ask it to (with HOMEBREW_SIMULATE_MACOS_ON_LINUX, or even HOMEBREW_SIMULATE_LINUX_ON_MACOS). But I'll admit to not having thought about this very hard or looked for corner cases.

I also don't really have a problem with #simulating_linux? being true even if the system really is running Linux, and it's not a simulation.

Are there cases where we want to distinguish between a machine that is running Linux with another machine running macOS, but the developer wants to trick brew into thinking its running Linux?

Are there cases where we want to distinguish between a machine that is running Linux with another machine running macOS, but the developer wants to trick brew into thinking its running Linux?

One instance might be that trying to brew install anything on macOS while simulating Linux might lead to a bad time. But do we really need guardrails here for users in this position? What if they actually really wanted to brew install some Linux software on macOS (perhaps for testing purposes)?

But do we really need guardrails here for users in this position?

Probably not...? The only way for a user to activate any simulation stuff is through HOMEBREW_SIMULATE_MACOS_ON_LINUX, and I think it seems pretty clear that using that variable may cause problems if you don't know what you're doing. If we get weird bug reports where people are trying to set that unnecessarily, we can add a warning or undocumented it or something.

In which case it seems to me that we should be fine with #simulating_linux?, no? Because we don't really need to distinguish between a real Linux machine and one only pretending to be Linux.

TBH, I don't see why we don't want OS.mac? and OS.linux? to lie when we ask it to (with HOMEBREW_SIMULATE_MACOS_ON_LINUX, or even HOMEBREW_SIMULATE_LINUX_ON_MACOS). But I'll admit to not having thought about this very hard or looked for corner cases.

I don't think they necessarily should but the argument for them doing so is that it's useful to be able to e.g. generate information for macOS on Linux machines and, less so but still useful, to do Linux on macOS.

Are there cases where we want to distinguish between a machine that is running Linux with another machine running macOS, but the developer wants to trick brew into thinking its running Linux?

I think the differentiation from me is "behaviour like we need to on Linux based on the underlying system" vs. "behaviour that differs on macOS on Linux and we want to generate data (e.g. dependency trees) on one OS from the other".

One instance might be that trying to brew install anything on macOS while simulating Linux might lead to a bad time.

Yeh, specifically this: we shouldn't allow installing in this case. We can either e.g. prevent installation or opt-in to the commands/actions that we want to allow (e.g. brew deps, brew info, etc.)

In which case it seems to me that we should be fine with #simulating_linux?, no?

I'm fine with behaviour, it's just the naming that's weird to me. simulating_or_running_on_linux? is a bit of a mouthful but more accurately describes what's going on. Alternatively, SimulateSystem.linux? is generic enough that it doesn't imply that you're not on Linux right now.

Rylan12 · 2022-07-25T15:35:58Z

Would love to try and move more stuff to extend/os somehow but that shouldn't block this PR and could be a cleanup effort when the project is otherwise mostly done.

Currently, I've been moving things out of extend/os since we can't really de-require and re-require these files during runtime when we are simulating other OSes. So the undef and re-def system doesn't work great. Any suggestions for how to handle that while keeping the logic separate? If so, I agree that feels cleaner. I may be able to play with it more eventually, but I agree it seems like a cleanup thing that we can do a little farther down the road

MikeMcQuaid · 2022-07-25T15:38:50Z

Any suggestions for how to handle that while keeping the logic separate?

Maybe relying on global instances of classes/modules rather than class/module-level functions? It's relatively easy to e.g. create a new global instance.

Alternatively, and this would likely be a huge pain, allow passing in an OS object or something to a Formula which is used for all its querying.

Rylan12 · 2022-07-25T15:50:11Z

Maybe relying on global instances of classes/modules rather than class/module-level functions? It's relatively easy to e.g. create a new global instance.

Can you give an example of what you mean by this?

MikeMcQuaid · 2022-07-25T16:02:05Z

Maybe relying on global instances of classes/modules rather than class/module-level functions? It's relatively easy to e.g. create a new global instance.

Can you give an example of what you mean by this?

e.g. instead of checking OS.linux? we have some global Homebrew.os = OS.new and then do Homebrew.os.linux? and can redo Homebrew.os = OS.new at a later point.

Rylan12 · 2022-07-25T16:16:47Z

e.g. instead of checking OS.linux? we have some global Homebrew.os = OS.new and then do Homebrew.os.linux? and can redo Homebrew.os = OS.new at a later point.

I'm not sure this solves the problem, though. Currently, in extend/os/software_spec we have this:

brew/Library/Homebrew/extend/os/software_spec.rb

Lines 5 to 9 in 32fd237

    
           if OS.mac? || Homebrew::EnvConfig.simulate_macos_on_linux? 
        
             require "extend/os/mac/software_spec" 
        
           elsif OS.linux? 
        
             require "extend/os/linux/software_spec" 
        
           end

Your suggestion as I understand it would change this file to something like this:

if Homebrew.os.mac?
  require "extend/os/mac/software_spec"
elsif Homebrew.os.linux?
  require "extend/os/linux/software_spec"
end

However, we still will only run the code in this file once (whenever we first run require "extend/os/software_spec"). Even if we change Homebrew.os later on, we still won't re-require this file so we'll never change which version of the SoftwareSpec module is the one that's required at the moment.

MikeMcQuaid · 2022-07-25T16:19:37Z

However, we still will only run the code in this file once (whenever we first run require "extend/os/software_spec"). Even if we change Homebrew.os later on, we still won't re-require this file so we'll never change which version of the SoftwareSpec module is the one that's required at the moment.

Yeh, I'm suggesting that anything like this that needs to vary behaviour based on OS needs to be done in a way that can be changed at runtime 😬. I'm not sure how practical that is at this point.

BrewTestBot · 2022-07-26T00:06:05Z

Review period ended.

Rylan12 · 2022-07-27T13:39:54Z

It's been a few days and there hasn't been any more maintainer feedback, so if it's okay with you, @MikeMcQuaid, I'd like to merge this for now. There are definitely more improvements that can be made, but I think they're lower priority (and will require pretty substantial code changes) so I'd rather push those to the future.

carlocab

Thanks @Rylan12!

MikeMcQuaid

Sorry, at a conference so a bit slower getting around to reviews but: let's hold off on merging this for now still. Apologies!

MikeMcQuaid · 2022-07-28T09:01:13Z

Library/Homebrew/software_spec.rb


-    @uses_from_macos_elements << spec
+    if OS.mac?


I'm not sure I understand why we're moving OS.mac? checks away from extend/os? Apologies that this is probably due to my feedback.

No worries! Let me know if this answers your question. If not, I think I can make a simple example to help.

At some point when we start running a Homebrew command, we require "formula" which in turn runs require "software_spec". This defines a uses_from_macos method that always passes its arguments on to depends_on (this is the Linux implementation of uses_from_macos.

Then, at the bottom of this file, we call require "extend/os/software_spec" which executes this file and, depending on the system configuration at that moment, requires either the macOS or Linux extension files:

If it requires the macOS extension, the uses_from_macos method is re-defined as the method that checks the since condition to see whether to pass to depends_on or not.

If it requires the Linux extension, there are no changes to uses_from_macos (so it retains its original functionality).

The problem arises when we want to simulate Linux while on macOS or vice versa. If we switch from one to the other, we don't actually re-execute any of the files listed above because their contents have already been executed and their modules/methods have already been defined. So, we are stuck with the implementation for the original system configuration (i.e. whatever it was when we first ran require "formula". So, if we simulate Linux on macOS and re-load a formula, we'll still execute the Linux version of uses_from_macos (i.e. always add the dependency) which is incorrect for macOS.

Similarly, the ignore_missing_libraries method is an immediate error by default, and is re-defined without the error on Linux. So, if we start on macOS and then switch to Linux, we still execute the macOS version of ignore_missing_libraries which throws an error and halts execution.

Thanks, that makes sense. What I don't understand is why OS.mac? is queried here rather than a SimulateSystem method?

Ah, because with the newest version OS.mac? reflects the simulation as well as the reality (just to try it out). Based on discussion above I think we'd rather go the other route where OS.mac? is only when we're actually on a mac and SimulateSystem.simulating_or_running_on_linux? is preferable

Ah, yes. Agreed on changing based on the above and if this is changed to a SimulateSystem check: that's fine with me 👍🏻

If we're already not distinguishing between simulating or not, it seems a bit superfluous to distinguish between OS.linux? and SimulateSystem.simulating_or_running_on_linux?, though.

It's not a super strong opinion, but I feel it'll be simpler to write future code if all we cared about is OS.foo? instead of considering also that we might want a code path to be run while simulating so we also need SimulateSystem.simulating_or_running_on_linux?.

This concern is at least partly (perhaps largely) mitigated by our preference to use os/mac or os/linux directories, admittedly.

If we're already not distinguishing between simulating or not

I was planning on reverting this change so that OS.mac? etc only reflect the actual system and is not affected by simulations. Does that remove your concern?

but I feel it'll be simpler to write future code if all we cared about is OS.foo?

That's my point, though: we don't want to do this. I've worked pretty hard to avoid scattering OS.mac? and OS.linux? all over the codebase in favour of the (IMO much more maintainable) extend/os model.

As @Rylan12 has addressed above: we have to do this a bit differently with simulation given that we need to do this without e.g. require.

Okay, I've pushed some changes, let me know if this is what you both were expecting.

Now, OS.mac? is only true when we're actually on macOS. Homebrew::SimulateSystem.simulating_or_running_on_macos? is true if we're actually on macOS or are simulating macOS.

MikeMcQuaid

This is great! Happy with how we've ended up here. Great work @Rylan12!

carlocab

Works for me. Thanks for your work @Rylan12!

Rylan12 added 3 commits July 23, 2022 03:08

Simplify SimulateSystem usage

fa384b0

Use SimulateSystem for ignore_missing_libraries

34a1bc6

Use SimulateSystem for uses_from_macos deps

91cf9f2

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 23, 2022

odinson94666 reviewed Jul 23, 2022

View reviewed changes

MikeMcQuaid approved these changes Jul 25, 2022

View reviewed changes

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jul 26, 2022

BrewTestBot approved these changes Jul 26, 2022

View reviewed changes

carlocab approved these changes Jul 27, 2022

View reviewed changes

MikeMcQuaid reviewed Jul 28, 2022

View reviewed changes

Rename treat_as_*? to simulating_or_running_on_*?

4cc0e85

MikeMcQuaid approved these changes Jul 28, 2022

View reviewed changes

carlocab approved these changes Jul 28, 2022

View reviewed changes

Rylan12 merged commit ef929a7 into Homebrew:master Jul 28, 2022

Rylan12 deleted the cleanup-simulate-system branch July 28, 2022 14:05

Rylan12 mentioned this pull request Jul 28, 2022

Move HOMEBREW_SIMULATE_MACOS_ON_LINUX handling to SimulateSystem #13616

Merged

github-actions bot added the outdated PR was locked due to age label Aug 28, 2022

github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2022

		# Linux simulating macOS. Assume oldest macOS version.
		return if Homebrew::EnvConfig.simulate_macos_on_linux? && !bounds.key?(:since)

	return false if Homebrew::SimulateSystem.treat_as_linux?
	return false if Homebrew::SimulateSystem.simulating_linux?

SimulateSystem improvements #13599

SimulateSystem improvements #13599

Conversation

Rylan12 commented Jul 23, 2022 • edited Loading

BrewTestBot commented Jul 23, 2022

odinson94666 left a comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rylan12 commented Jul 25, 2022

MikeMcQuaid commented Jul 25, 2022

Rylan12 commented Jul 25, 2022

MikeMcQuaid commented Jul 25, 2022

Rylan12 commented Jul 25, 2022

MikeMcQuaid commented Jul 25, 2022

BrewTestBot commented Jul 26, 2022

Rylan12 commented Jul 27, 2022

carlocab left a comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlocab Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

carlocab left a comment

Choose a reason for hiding this comment

`SimulateSystem` improvements #13599

`SimulateSystem` improvements #13599

Rylan12 commented Jul 23, 2022 •

edited

Loading

carlocab Jul 28, 2022 •

edited

Loading