Cross-platform work

[MikeMcQuaid]

Start working towards supporting Linux in Homebrew/brew for 1.0.0.

As a starting point (and to make development easier) I've added an environment variable for using only cross-platform, non-OS X code so I can start testing locally. I've only ported a few files/classes so far but I wanted to get feedback on the approach before I work on this further.

[rwhogg]

Some Linux distros don't have tar installed in /usr/bin, but rather in /bin.

[sjackman]

I prefer to use tar and other utilities from PATH. Or alternatively, prefer the brewed utility over the host's utility, which could be found in either /usr/bin/ or /bin. Either PATH could be modified to search these three directories, or we could add our own path searching wrapper function.

[MikeMcQuaid]

This is something we can consider in future but for now this is just a copy-paste from the existing code.

[rwhogg]

This approach looks really good to me, and I'm really happy to see something like this happen.

[apjanke]

Generally 👍 on this approach.

As far as design: I would make the DevelopmentTools stuff instance methods instead of class methods, and use a singleton/factory method that gives the appropriate OS-specific instance. (And maybe have class methods in DevelopmentTools that delegate to that instance.) That way some stuff could be inherited, and you could instantiate objects for multiple platforms in a single process for A/B testing. And I find the class aliasing (like DevelopmentTools ||= GenericDevelopmentTools) to be kind of advanced and confusing. But maybe that's just my Java background talking; I don't know if this is an established Ruby idiom.

[MikeMcQuaid]

Interested on @xu-cheng's and @UniqMartin's thoughts here.

As far as design: I would make the DevelopmentTools stuff instance methods instead of class methods, and use a singleton/factory method that gives the appropriate OS-specific instance. (And maybe have class methods in DevelopmentTools that delegate to that instance.) That way some stuff could be inherited, and you could instantiate objects for multiple platforms in a single process for A/B testing. And I find the class aliasing (like DevelopmentTools ||= GenericDevelopmentTools) to be kind of advanced and confusing. But maybe that's just my Java background talking; I don't know if this is an established Ruby idiom.

It's mostly the way I did things when doing C++ cross-platform work. I don't think there's an established Ruby cross-platform porting idiom. I generally dislike singletons but I guess if it was a e.g. global constant it would end up having a similar effect to a class. I don't have strong feelings here; would love to hear other maintainer thoughts.

[UniqMartin]

I guess it's not the time yet to comment on individual parts of the code, but I can't help but notice that this work in progress is still a bit messy. Namely:

• This file os/mac/cmd/config.rb clearly shouldn't be here.
• There's at least one instance where two privates immediately follow each other.
• Maybe more? It's hard to spot this stuff when things are moved around and refactored in the same commit, causing Git to lose the connection between files (at least when viewed on GitHub).

Sorry for this pedantry …

[MikeMcQuaid]

That's not pedantry; it's still a bit messy, I agree. This is why I tried to keep this PR a bit smaller; reviewing individual commits should help but once I've nailed the approach it'll be easier to split future things into individual PRs.

[UniqMartin]

Interested on @xu-cheng's and @UniqMartin's thoughts here.

I don't really have a lot of experience with doing this kind of stuff in Ruby. I'm not a big fan of the currently proposed constant (module, class) aliasing and neither seems Ruby 1.8. Singletons and inheritance could work, but doesn't sound like typical Ruby code, even though it's probably more friendly for testing.

Here's what I think is idiomatic, relatively clean, and requires comparatively few changes (example):

# In 'hardware.rb':
module Hardware
  module CPU # Called GenericCPU in current proposal.
    # Platform-agnostic parts of the implementation.
  end
end

require "os/hardware"

# In 'os/hardware.rb':
if OS.mac?
  require "os/mac/hardware"
  Hardware::CPU.extend(OS::Mac::Hardware::CPU)
elsif OS.linux?
  require "os/linux/hardware"
  Hardware::CPU.extend(OS::Linux::Hardware::CPU)
end

# In 'os/mac/hardware.rb':
module OS
  module Mac
    module Hardware
      module CPU
        # Mac-specific pieces of the implementation.
      end
    end
  end
end

# In 'os/linux/hardware.rb':
module OS
  module Linux
    module Hardware
      module CPU
        # Linux-specific pieces of the implementation.
      end
    end
  end
end

The same pattern could also be applied in all other cases where things need to be separated. I'm not sure (to stay with the example) if os/hardware.rb needs to exist at all of whether its contents should be simply appended to hardware.rb.

[xu-cheng]

I agree with @UniqMartin's proposed style. I think we should make it similar with how we handle Stdenv and Superenv

[MikeMcQuaid]

@UniqMartin I like that approach. I've generally disliked the Ruby module inclusion approach because it's not obvious where classes live but if we're strict about it and use this only for OS-specific stuff that seems fine. As an aside, I personally really wish we'd have only a single class or module per file and the filename indicate what it contains; after all my time working on Homebrew I can never remember what the filename is for various classes and I'm sure I'm not the only one.

I wonder whether it would be awful for Hardware::CPU.extend(OS::Mac::Hardware::CPU) to live in os/mac/hardware.rb itself just to reduce the boilerplate required in os/hardware.rb.

I'm not sure (to stay with the example) if os/hardware.rb needs to exist at all of whether its contents should be simply appended to hardware.rb.

I'm not sure either. My main goal was to get to a point where there's basically no e.g. OS.mac? checks outside of os

[UniqMartin]

As an aside, I personally really wish we'd have only a single class or module per file and the filename indicate what it contains; after all my time working on Homebrew I can never remember what the filename is for various classes and I'm sure I'm not the only one.

It's certainly nice to have, but I don't see this as a major issue. I'm using a programmer's text editor that isn't at all optimized for Ruby, but it still allows me to jump to arbitrary identifiers in the project and deals perfectly fine with multiple files re-opening the same class, e.g. Pathname that we're extending in five different files. Thus if I know the identifier, I don't need to know in which file it lives.

I wonder whether it would be awful for Hardware::CPU.extend(OS::Mac::Hardware::CPU) to live in os/mac/hardware.rb itself just to reduce the boilerplate required in os/hardware.rb.

If we retain os/hardware.rb, it feels like the more natural place for that. So yes, awful. 😉

I'm not sure (to stay with the example) if os/hardware.rb needs to exist at all of whether its contents should be simply appended to hardware.rb.

I'm not sure either. My main goal was to get to a point where there's basically no e.g. OS.mac? checks outside of os.

Fair point.

[MikeMcQuaid]

This has been rebased and is using the extend/os (i.e. reopening classes) approach after people found the inheritance approach messy and the extend Module approach doesn't work adequately for class method overriding.

[MikeMcQuaid]

As the rebasing is pretty hideous here I'm hoping to merge this pretty shortly if CI is 💚 and there's no complaints. We can always iterate on the approach.

[MikeMcQuaid]

(merging this will also allow future PRs to be a lot smaller now the basic groundwork is in).

[UniqMartin]

Feels odd that this requires utils/bottles which in turn requires this file. This would be a cyclic dependency if require wasn't more sophisticated. Is this really intended? If I'm not mistaken, this has the effect that requireing either of those will bring in all of Utils::Bottles, but I feel like this file here should never be required outside of utils/bottles.rb. (I hope this wasn't confusing.)

If you agree, a bold way to enforce this instead of silently doing the supposedly right thing would be to fail in this line if Utils::Bottles hasn't been defined yet.

[MikeMcQuaid]

Yeh, feels fine to just remove this redundant require.

[UniqMartin]

This also applies to a few similar files currently residing in extend/os/.

[MikeMcQuaid]

Yep.

[UniqMartin]

I've only reviewed a small part of the updated PR, but what I noticed is the (in my opinion) odd placement of files that I hope we can address before merging this. (Everything below relative to Library/Homebrew.)

Major: So far we've used extend/ exclusively to extend classes provided by Ruby. Re-purposing this directory for a hierarchy of platform-specific code feels wrong to me and the original placement in os/ resonated much more with me. To pick a specific example, could we change things such that what previously was bottles.rb would be split into the following files?

• utils/bottles.rb – Platform-agnostic code, ends with loading os/utils/bottles.rb.
• os/utils/bottles.rb – Loads one of os/<platform>/utils/bottles.rb.
• os/mac/utils/bottles.rb – Contains OS-X-specific parts of Utils::Bottles.

Minor: bottles.rb apparently was split into extend/os/bottles.rb, extend/os/mac/utils/bottles.rb, and utils/bottles.rb. Notice how the first item lacks a utils path component. That's probably unintended.

---

Any suggestion for reviewing this and seeing a useful diff of the code that has been moved around, so it actually becomes possible to spot problems that might have snuck in while shuffling around the code? (I suppose the test coverage for the touched code isn't close to 100%.)

[MikeMcQuaid]

So far we've used extend/ exclusively to extend classes provided by Ruby. Re-purposing this directory for a hierarchy of platform-specific code feels wrong to me and the original placement in os/ resonated much more with me.

I don't agree with this, personally. It feels weird to reopen classes in a bunch of different places and so far almost everything inside e.g os/mac has been stuff that's in the OS::Mac namespace. I'm open to a third way beyond extend/os but I don't think the argument can be made that it's more consistent in os/mac

bottles.rb apparently was split into extend/os/bottles.rb, extend/os/mac/utils/bottles.rb, and utils/bottles.rb. Notice how the first item lacks a utils path component. That's probably unintended.

👍

Any suggestion for reviewing this and seeing a useful diff of the code that has been moved around, so it actually becomes possible to spot problems that might have snuck in while shuffling around the code? (I suppose the test coverage for the touched code isn't close to 100%.)

Not really, unfortunately. It's not so much just moving as moving and splitting logic between files (hence why rename detection doesn't kick in). I guess you could maybe set the rename detection low enough and try it locally?

[UniqMartin]

I don't agree with this, personally. It feels weird to reopen classes in a bunch of different places and so far almost everything inside e.g os/mac has been stuff that's in the OS::Mac namespace. I'm open to a third way beyond extend/os but I don't think the argument can be made that it's more consistent in os/mac

I guess that's a matter of perception. For me, extend meant (and still means) extensions to classes and modules not originally provided by Homebrew, effectively meaning stuff provided by Ruby itself. I can totally understand how you re-interpreted that as “the directory where we reopen and extend existing classes/modules, no matter where they came from” (if I'm interpreting your thoughts correctly).

And yes, my argument was more against extend/os/ and less about moving everything to os/ though that was the first thing that came to mind. Maybe we can avoid confusion with the OS and OS::Mac namespace if we move the platform-specific stuff to platform/ or some other currently unused directory? (I'm somewhat sorry for raising this and delaying the merge, but shuffling around files in a follow-up PR isn't fun either.)

Not really, unfortunately. It's not so much just moving as moving and splitting logic between files (hence why rename detection doesn't kick in). I guess you could maybe set the rename detection low enough and try it locally?

I'll try and see what I can come up with for making the review easier (I'm already doing it locally). Just was hopeful you already know a simple and ready-to-use solution you've used yourself.

[MikeMcQuaid]

Maybe we can avoid confusion with the OS and OS::Mac namespace if we move the platform-specific stuff to platform/ or some other currently unused directory?

@UniqMartin I think that's going to add to the confusion; what's the difference between an OS and Platform? It sounds like we don't have a clear solution here so I don't think it should block this PR being merged.

but shuffling around files in a follow-up PR isn't fun either.

It's not "fun" but it does mean people (e.g. me) can continue to do cross-platform work while the discussion happens about where these files should go. If they require no modifications except filenames then the rename detection will be good and the diff will be minimal. From that perspective: I think we should merge this as-is for now as this cross-platform work is a blocker for a 1.0.0 release.

[MikeMcQuaid]

I think we should merge this as-is for now as this cross-platform work is a blocker for a 1.0.0 release.

And also: there's enough stuff being touched here to mean that there's regular and irritating merge conflicts to resolve.

[UniqMartin]

And also: there's enough stuff being touched here to mean that there's regular and irritating merge conflicts to resolve.

I appreciate this is more than just annoying. I was just hopeful someone else would join our conversation after you rebased and shuffled a lot of stuff around, maybe coming up with better and more convincing ideas than mine. As it stands, I wasn't able to convince you that extend/ is not the right place nor was I able to come with a satisfying alternative.

I haven't had the time to review this thoroughly (and won't be able to do that in the next few days), but I trust you that you were careful when moving things around. Thus, if it were up to me, feel free to merge.

[UniqMartin]

What is the purpose of this file? I was of the impression that the OS-X-specific code of cmd/config.rb is now in Library/Homebrew/extend/os/mac/system_config.rb.

[MikeMcQuaid]

Added by mistake, not needed.

[sjackman]

Sorry that I haven't found the time to review this PR yet. Regardless of when it's merged, I'll find some time next week to go over it.

[apjanke]

I noticed the conversation but am not Ruby-ed up enough to have an opinion beyond what I said earlier. I've read through the code and it seems cogent, so 👍 to merge by me.

[UniqMartin]

bottle_native_regex has been renamed to native_regex (or Utils::Bottles::native_regex). See a few lines below.

[sjackman]

I will say that I agree with Martin that regardless of how the class composition is implemented (inheritance vs reopening the class) I think the code would better live in os rather than extend/os, and that extend is reserved for extending standard Ruby classes. My two cents, and not a fiercely-held opinion.

[sjackman]

Thanks, Mike! I see a lot of merge conflicts in my future. 😂

[MikeMcQuaid]

@sjackman In the short-term: perhaps. In the long-term: you should have none 😉. The only thing I can see that's going to perhaps cause you grief is that you'll need to stop using e.g. MacOS in formulae.

[sjackman]

So long as the portable stuff is extracted out into a common shared file, that's fine by me.

[MikeMcQuaid]

@sjackman Yeh, the goal is that everything outside extend/os/mac or os/mac (or wherever these end up) will work on non-OS X platforms.

[xu-cheng]

This may accidentally break some formulae. e.g.

$ brew info octave
Error: undefined method `clang_version' for OS::Mac:Module
Please report this bug:
    https://git.io/brew-troubleshooting

We could certainly fix it by altering formula. But I'm not sure whether this should be treated as a API break? If I didn't mistake, we probably want to avoid DevelopmentTool used in formula DSL?

[MikeMcQuaid]

@xu-cheng brew update; that case is fixed for me. I agree that we should discourage some of these methods from being used in the formula DSL (ideally programmatically).

[xu-cheng]

brew update; that case is fixed for me.

Unfortunately, it is not fixed if run homebrew in no compat mode HOMEBREW_NO_COMPAT=1 brew info octave

[MikeMcQuaid]

@xu-cheng Yep, that's intentional.

[xu-cheng]

Yep, that's intentional.

Why is it intentional? Homebrew under no compat mode should be able to work just like normal mode.

[MikeMcQuaid]

@xu-cheng It's not a public API and it's not not a tap I maintain.

[xu-cheng]

OR do you mean we should change octave formula to use DevelopmentTools?

[MikeMcQuaid]

OR do you mean we should change octave formula to use DevelopmentTools?

I don't think it should use either really; we should decide what we want to support as a public API which currently is only http://www.rubydoc.info/github/Homebrew/brew/Formula and the classes linked from it.

[UniqMartin]

I don't think it should use either really; […]

👍