Move the 'wsl?' bool from the OS::Linux module up to the OS module#22309
Conversation
I think this makes more sense, sorry. |
@MikeMcQuaid okay, that's a bit disappointing to hear but no problem. Would you be willing to consider an
|
|
Use
No, sorry. |
@MikeMcQuaid To clarify, the solution you propose here does the exact opposite of what my example was trying to achieve. I agree it is not cumbersome and it doesn't make things harder to read. However, like I said, that statement works installs The usecase I was presenting was installing Without a "top-level", public boolean member of the # Install a specific package if we're on any machine except a WSL one
# using if
brew "foobar" if (OS.linux? && !OS::Linux.wsl?) || OS.mac?
# using unless
brew "foobaz" unless (!OS.linux? || (OS.linux? && OS::Linux.wsl?)) && !OS.mac?Semantically, if you're on the ruby purist side of the scale, and you want to do something everywhere except one place you'd want to use an I may also be 100% wrong on this, but from looking at brewfile examples and some of the code, but it seems to me like you may prefer the ruby "poetry" style of writing ruby on a personal level, and the statements above would definitely not be considered poetic anymore as soon as the parentheses are thrown in. You've made it clear you don't want to implement an alternative here which is OK, it's your software not mine so I'm not entitled to it! But I just want to point out that it seems counterintuitive and IMO (huge focus on the "my opinion" part of that acronym, I mean I didn't write the article!) not in line with the philosophy you're presenting in your article Homebrew Bundle, brew bundle and Brewfile, which is what made me decide to introduce Homebrew/Brewfiles into my previously bash-only stack. So it's just a bit disappointing personally (and selflishly) to see that it didn't end up being the tool I thought it would be for me. |
|
Ok, thanks. Can you explain a bit more why you want to differentiate WSL and Linux in this way? |
|
@MikeMcQuaid Because WSL is limited, and with a multi-machine The one that was my "immediate need" while I was setting up my Other ones that come to mind immediately: brew "synergy-core" unless OS.wsl?
brew "podman" unless OS.wsl?Is a lot more palatable than # with ifs
brew "synergy-core" if (OS.linux? && !OS::Linux.wsl?) || OS.mac?
brew "podman" if (OS.linux? && !OS::Linux.wsl?) || OS.mac?
# with unlesses
brew "synergy-core" unless (!OS.linux? || (OS.linux? && OS::Linux.wsl?)) && !OS.mac?
brew "podman" unless (!OS.linux? || (OS.linux? && OS::Linux.wsl?)) && !OS.mac?Other things that I could see being a problem (but I don't use myself) would be anything with a graphical component; I know typically those are casked/limited to macOS anyways typically but for any formulas that provide a graphical interface (I don't use many graphical programs so I'm not the right person to ask, maybe there aren't many/any?), the WSLg experience is pretty terrible still so I could see an
It's also not so much about differentiating the two but more about having a clean, safe public api to determine we are on WSL. |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Fine, let's do this, thanks.
This PR moves the makes the internal
wsl?boolean member of Homebrew'sOS::Linuxavailable directly in as a public member ofOSinos.rbalongsideOS.mac?andOS.linux?While I've modified the handful of references in the code to point to this new location, I've maintained backwards compatibility by having leaving the definition of
wsl?inOS::Linuxbut changing it to return the value ofOS.wsl?instead which should prevent any breakage for users that happened to be usingOS::Linux.wsl?in, for instance, brewfiles.Why this change?
I ran into some issues with Brewfiles, which I only have one of however, one that covers 10+ machines and depends on conditionals. The Brew, brew Bundle and Brewfile "Advanced brewfiles" section seems to encourage this usage of brewfiles, and explicitly provides examples of conditionals using
OS.mac?andOS.linux?.It's however not possible to safely check
OS::Linux.wsl?to see if we are on a WSL machine since theOS::Linuxmodule is only loaded bybrewif we are currently on a Linux machine (here), thus, if we happen to be on a Mac (or run withHOMEBREW_TEST_GENERIC_OS=true), we get:Of course, we could get around this by using compounded conditional statements/checking we are on Linux first and THEN checking for WSL which would prevent the exception being raised, but IMO that leads to a more complicated to read/understand brewfile.
Hope you'll consider this change or propose an alternative if this isn't adequate, I'm happy to adjust the PR. thanks.
mac?andlinux?and the existing test forOS::Linux.wsl?tests this change by virtual of the fact that that reference has been changed to refer to this newwsl?method. I can add an additional test however if you feel it warrants the duplication.brew lgtm(style, typechecking and tests) with your changes locally?