Add methods to retrieve appropriate archflags #21636
Conversation
|
This is so good. |
|
This reminds me that I really want to create a helper to generate (Can't use |
|
We could use that helper a few places in the core formulae to (I think; if those haven't been rewritten) |
|
Yeah, we construct it in a few places, so it would really be simpler just to do it in one place. Counting taps we construct it manually in close to 30 different formulae. |
|
Thinking about it, e.g.: def universal_archs
# Building 64-bit is a no-go on Tiger, and pretty hit or miss on Leopard.
# Don't even try unless Tigerbrew's experimental 64-bit Leopard support is enabled.
if MacOS.version <= :leopard and !MacOS.prefer_64_bit?
[arch_32_bit]
else
[arch_32_bit, arch_64_bit]
end
end |
|
I was just going to ask about ppc64 |
|
The issue isn't actually ppc64 so much as an incredibly buggy Tigerbrew has optional, off-by-default 64-bit Leopard support; Homebrew doesn't. |
|
Updated |
| @@ -165,7 +165,7 @@ class Cmd | |||
| args << '-march=native' if tool =~ /clang/ | |||
| end | |||
|
|
|||
| args += %w{-arch i386 -arch x86_64} if cccfg? 'u' | |||
| args += ENV['HOMEBREW_ARCHS'].split(',').map {|a| "-arch #{a}"} if cccfg? 'u' | |||
There was a problem hiding this comment.
Seems potentially expensive for every cc call?
There was a problem hiding this comment.
Potentially, I hadn't checked. I guess it could just be preset as HOMEBREW_ARCHFLAGS or some such.
|
Looks good to me other than comments. |
|
There's a little-known module in mach.rb that is used by the Perhaps there is some potential for reuse or consolidation here. |
|
Dead pull request ? |
It's only been three days. I've been really busy and haven't had a chance to address Mike's and Jack's comments. |
|
Well it has been three days that it was mentioned from my pull request and since nothing occurred while I waited to see if I could hard code less things (allowed arches, for one). |
|
Sorry - I've just been really, really busy moving to a new city, finding an apartment, coordinating a move, etc. Been tough to find time to work on these things. |
I'd missed that, thanks! If we teach it about additional architectures it may be able to help; this PR deals with the four generic canonical archs (i386/x86_64/ppc/ppc64), whereas that needs to understand additional specific archs that may be encountered in compiled files (e.g. ppc vs ppc7400, ppc7450, ppc970). |
|
I hope you'll enjoy your new home sweet home. Once your PR is ready you can simply go and fix that little thing in cc by yourself. Guillaume DIDIER Le 8 août 2013 à 18:11, Misty De Meo notifications@github.com a écrit :
|
|
Thinking about it, could possibly combine work between them by exposing some constants in Hardware::CPU, and having # These are defined on Hardware::CPU
INTEL_32BIT_ARCHS = [:i386, :i686].freeze
INTEL_64BIT_ARCHS = [:x86_64].freeze
PPC_32BIT_ARCHS = [:ppc, :ppc7400, :ppc7450, :ppc970].freeze
PPC_64BIT_ARCHS = [:ppc64].freeze
module ArchitectureListExtension
# Means two or more archs, doesn't imply Intel or PPC
def universal?
@universal ||= length > 1
end
def intel_universal?
@intel_universal ||= (Hardware::CPU::INTEL_32BIT_ARCHS+Hardware::CPU::INTEL_64BIT_ARCHS).any? {|a| self.include? a}
end
def ppc_universal?
@ppc_universal ||= (Hardware::CPU::PPC_32BIT_ARCHS+Hardware::CPU::PPC_64BIT_ARCHS).any? {|a| self.include? a}
end
def ppc?
@ppc ||= (PPC_32BIT_ARCHS+PPC_64BIT_ARCHS).any? {|a| self.include? a}
end
def remove_ppc!
(Hardware::CPU::PPC_32BIT_ARCHS+Hardware::CPU::PPC_64BIT_ARCHS).each {|a| self.delete a}
end
def as_arch_flags
self.collect{ |a| "-arch #{a}" }.join(' ')
end
end
# used in subversion, for example
if build.include? 'perl'
# Remove hard-coded ppc target, add appropriate ones
if build.universal?
arches = Hardware::CPU.universal_archs.as_arch_flags
elsif MacOS.version <= :leopard
arches = "-arch #{Hardware::CPU.arch_32_bit}"
else
arches = "-arch #{Hardware::CPU.arch_64_bit}"
end |
|
In this definition, |
|
Good point. Should probably be |
|
Do we use this test anywhere currently? |
|
Yes, looks like it's used in at least the boost UniversalPython test: satisfy(:build_env => false) { archs_for_command("python").universal? } |
|
Aha. So we should be clear that Boost is requesting a 32/64 build of the same arch. |
|
If it's not convenient to combine them, don't worry about it; I didn't give it any thought other than |
|
It did prompt me to notice that it's missing a bunch of archs we'll actually come across in the real world; aside from the PPC stuff we build GCC i686 for 32-bit/universal for example. The current functionality is perfectly suitable for what it's doing but it doesn't hurt to make it more robust for the future. And returning an array with the mixin already extended is def. useful, even if there's only a few places it comes up. Actually, addressing @MikeMcQuaid's earlier comments - the mixin could gain an |
|
Pushed a new commit that beefs up the ArchitectureListExtension tests. |
| end | ||
|
|
||
| def as_arch_flags | ||
| self.collect{ |a| "-arch #{a}" }.join(' ') | ||
| end | ||
|
|
||
| def as_cmake_arch_flags | ||
| self.join(';') |
There was a problem hiding this comment.
not sure here, because .join(';') is shorter than as_cmake_arch_flags
There was a problem hiding this comment.
Explicit is better than implicit.
And yet another method to document and add to example.rb.
You seem to love it so much ... I better shut up now :-)
|
@sjackman Ping re: the os/mac/hardware stuff. The arch_(32|64)_bit methods are directly applicable on Linux I assume. Should the universal stuff just return an array with only the preferred arch for Linux? |
|
Yes, for Linux an array with a single value seems reasonable to me. The standard GNU toolchain has no |
|
Does linuxbrew use superenv, or only stdenv? Superenv has a different definition of |
|
Linuxbrew uses stdenv. superenv doesn't work out-of-the-box on Linux, as it would likely require a new profile in |
|
All right, I'll leave superenv issues for you to look at later then. |
This replaces hardcoding of i386/x86_64 all over the code.
Replaced the plethora of ternaries we've used all over the place to determine whether x86_64 or i386 is called for.
Also adds some reusable constants into the global Hardware::CPU namespace, available on both OS X and Linux.
This defines the new HOMEBREW_ARCHS environment variable, which is currently only set during universal builds, so that the tool wrappers no longer need to hardcode i386/x86_64.
|
Provided stub Linux methods in 3cc3b15. Should be enough to work fine here. I think this is about ready now. |
|
Looks good |
|
Pushed! ✨ |
|
👏 |
|
Just updated #21664 with that. |
Inspired by @adamv's work in #21330.
This creates new methods to retrieve the appropriate architectures for a given processor, and uses them in stdenv, superenv, and formulae. Added in this pull request are:
MacOS.preferred_arch- Returnsarch_32_bitorarch_64_bitbased onMacOS.prefer_64_bit?. Replaces an enormous amount of ternary copypaste we've been repeating over and over.Hardware::CPU.arch_32_bit- The CPU's 32-bit arch, currentlyi386orppcHardware::CPU.arch_64_bitHardware::CPU.universal_archs- Helper that returns an array of both the 32-bit and 64-bit archs for the given CPU type, mainly to construct multiple arch argumentsI'm open to naming suggestions on
arch_(32|64)_bit. Can't start a method name with a number unfortunately.