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

Cross-platform work #168

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Apr 29, 2016

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.

@retokromer retokromer referenced this pull request Apr 30, 2016

Closed

vim v7.4.1799 #1103

end

def receipt_path(bottle_file)
Utils.popen_read("/usr/bin/tar", "-tzf", bottle_file, "*/*/INSTALL_RECEIPT.json").chomp

This comment has been minimized.

@rwhogg

rwhogg Apr 30, 2016

Contributor

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

This comment has been minimized.

@sjackman

sjackman Apr 30, 2016

Contributor

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.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 30, 2016

Member

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

@rwhogg

This comment has been minimized.

Copy link
Contributor

rwhogg commented Apr 30, 2016

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

@apjanke

This comment has been minimized.

Copy link
Contributor

apjanke commented Apr 30, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 30, 2016

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.

#: * `config`:
#: Show Homebrew and system configuration useful for debugging. If you file
#: a bug report, you will likely be asked for this information if you do not
#: provide it.

This comment has been minimized.

@UniqMartin

UniqMartin Apr 30, 2016

Contributor

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 …

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 1, 2016

Member

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

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Apr 30, 2016

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

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 1, 2016

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 1, 2016

@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

This comment has been minimized.

Copy link
Contributor

UniqMartin commented May 1, 2016

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 MikeMcQuaid force-pushed the MikeMcQuaid:linux-osx branch from 0edbbd2 to db7dc2e May 6, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 6, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 6, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 6, 2016

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

@@ -0,0 +1,5 @@
require "utils/bottles"

This comment has been minimized.

@UniqMartin

UniqMartin May 6, 2016

Contributor

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.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 6, 2016

Member

Yeh, feels fine to just remove this redundant require.

This comment has been minimized.

@UniqMartin

UniqMartin May 6, 2016

Contributor

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

This comment has been minimized.

@MikeMcQuaid
@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented May 6, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 6, 2016

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

This comment has been minimized.

Copy link
Contributor

UniqMartin commented May 6, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 7, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 7, 2016

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

This comment has been minimized.

Copy link
Contributor

UniqMartin commented May 7, 2016

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.

@@ -0,0 +1,191 @@
require "hardware"

This comment has been minimized.

@UniqMartin

UniqMartin May 7, 2016

Contributor

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.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 8, 2016

Member

Added by mistake, not needed.

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented May 7, 2016

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

This comment has been minimized.

Copy link
Contributor

apjanke commented May 7, 2016

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.

filename = file.basename.to_s
return unless f.bottle && filename.match(Pathname::BOTTLE_EXTNAME_RX)

bottle_ext = filename[bottle_native_regex, 1]

This comment has been minimized.

@UniqMartin

UniqMartin May 7, 2016

Contributor

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

return unless f.bottle && filename.match(Pathname::BOTTLE_EXTNAME_RX)

bottle_ext = filename[bottle_native_regex, 1]
bottle_url_ext = f.bottle.url[bottle_native_regex, 1]

This comment has been minimized.

@UniqMartin

UniqMartin May 7, 2016

Contributor

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

end

def native_regex
/(\.#{tag}\.bottle\.(\d+\.)?tar\.gz)$/o

This comment has been minimized.

@UniqMartin

UniqMartin May 7, 2016

Contributor

tag should be Regex.escaped here.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 8, 2016

Member

Although Regexp 😉

This comment has been minimized.

@UniqMartin

UniqMartin May 8, 2016

Contributor

That will always be a point of confusion for me, as different languages tend to abbreviate this differently. I guess I was most influenced by the recent demand for replacing /PATTERN/ with /REGEX/ (note the missing “P”) in the output of brew help. 😉

f.puts hardware if hardware
f.puts "GCC-4.0: build #{gcc_40}" if gcc_40
f.puts "GCC-4.2: build #{gcc_42}" if gcc_42
f.puts "LLVM-GCC: build #{llvm}" if llvm

This comment has been minimized.

@UniqMartin

UniqMartin May 7, 2016

Contributor

GCC 4.0, GCC 4.2, and LLVM-GCC all look pretty Mac-specific to me. But can probably be fixed in a follow-up PR.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid May 8, 2016

Member

The current cross-platform work is to have code that works on Linux and OS X without any custom Homebrew/brew changes. Stuff around e.g. gcc_40 is a stage of porting that is still a while off.

This comment has been minimized.

@UniqMartin

UniqMartin May 8, 2016

Contributor

Yes, I guessed so and am totally fine with that. (Hence my last sentence above.)

Add support for testing generic OS.
If the environment variable HOMEBREW_TEST_GENERIC_OS is set ensure that
neither Mac nor Linux-specific code is loaded. This allows easier
testing of cross-platform code on OS X and will make it easier to port
Homebrew to platforms other than OS X and Linux.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:linux-osx branch from db7dc2e to 254360e May 8, 2016

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:linux-osx branch from 254360e to 62c16ea May 8, 2016

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:linux-osx branch May 8, 2016

@sjackman

This comment has been minimized.

Copy link
Contributor

sjackman commented May 8, 2016

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 8, 2016

@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

This comment has been minimized.

Copy link
Contributor

sjackman commented May 8, 2016

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 8, 2016

@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.

UniqMartin added a commit that referenced this pull request May 8, 2016

tab: fix bad default_compiler reference
Partially addresses #219. Related to changes introduced in #168.

UniqMartin added a commit that referenced this pull request May 8, 2016

utils/bottles: fix Regexp escaping for Ruby 1.8
Make sure `Regexp.escape` gets a string as Ruby 1.8 is unable to convert
the symbol to a string automatically. Related to changes from #168.

UniqMartin added a commit that referenced this pull request May 8, 2016

Fix more bad dump_verbose_config references
Follow-up to c7edf9a and related to
changes from #168.
@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 10, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 10, 2016

@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

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 10, 2016

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

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 10, 2016

@xu-cheng Yep, that's intentional.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 10, 2016

Yep, that's intentional.

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 10, 2016

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

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented May 10, 2016

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

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented May 10, 2016

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

This comment has been minimized.

Copy link
Contributor

UniqMartin commented May 10, 2016

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

👍

gregory-nisbet added a commit to gregory-nisbet/brew that referenced this pull request May 13, 2016

Make config command cross-platform.
Closes Homebrew#168.

Signed-off-by: Mike McQuaid <mike@mikemcquaid.com>

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.