mac: Support ARCHS with zero or one element in the make and ninja generators.

mac: Support ARCHS with zero or one element in the make and ninja generators.

General ARCHS support (fat binaries) are still unsupported.

Review URL: http://codereview.chromium.org/9382044
Landed in gyp r1198.

[Nico, 2012-02-13 06:09:07]

Thanks for working on this! It looks like dump_syms (in breakpad/breakpad.gyp)
doesn't depend on anything, so in my patch I just forgot the linker flags.
Thanks for catching this.

I worry about the len() == 0 case though – I'd like to make sure we're matching
xcode's behavior in that case. Could you maybe write a few tests for this (one
test case: no explicit ARCHS, i.e. len 0. second test case the example I give
below. If that fails to build in xcodebuild that's fine, I just want make and
ninja to match xcodebuild's behavior). (See test/mac for a bunch of mac-specific
tests.)

http://codereview.chromium.org/9382044/diff/1/pylib/gyp/xcode_emulation.py
File pylib/gyp/xcode_emulation.py (right):

http://codereview.chromium.org/9382044/diff/1/pylib/gyp/xcode_emulation.py#ne...
pylib/gyp/xcode_emulation.py:292: pass
I believe xcodebuild might default to i386 if ARCHS is empty.

http://codereview.chromium.org/9382044/diff/1/pylib/gyp/xcode_emulation.py#ne...
pylib/gyp/xcode_emulation.py:294: cflags.append('-arch ' + archs[0])
What if you have this setup:

{
  'targets': [
  {
    'target_name': 'lib',
    'type': 'static_library',
    'sources': 'my_file.cc',
    # Note: No ARCHS, which (I think) means i386
  },
  {
    'target_name': 'exe',
    'type': 'executable',
    'dependencies': [ 'lib', ],
    'sources': 'my_other_file.cc',
    'xcode_settings': {
      'ARCHS': [ 'x86_64', ],
    },
  },
}

In this case, the executable is built for 64 bit, but the library it links in is
only 32 bit. What does xcodebuild do in this case? I believe this will error out
in make /ninja with your patch, and I think xcodebuild handles this correctly
somehow (not sure though).

[ukai, 2012-02-13 07:02:12]

On 2012/02/13 06:09:07, Nico wrote:
> Thanks for working on this! It looks like dump_syms (in breakpad/breakpad.gyp)
> doesn't depend on anything, so in my patch I just forgot the linker flags.
> Thanks for catching this.
> 
> I worry about the len() == 0 case though – I'd like to make sure we're
matching
> xcode's behavior in that case. 

Hmm, I noticed that xcode refuses 'ARCHS': [], so drop len(archs) == 0 case.
xcodebuild claimed like this:
No architectures to compile for (ARCHS=, VALID_ARCHS=i386 ppc ppc64 ppc7400
ppc970 x86_64).
[BEROR]No architectures to compile for (ARCHS=, VALID_ARCHS=i386 ppc ppc64
ppc7400 ppc970 x86_64).

Anyway, is there no way to run compiler without -arch option under xcodebuild?


> Could you maybe write a few tests for this (one
> test case: no explicit ARCHS, i.e. len 0. second test case the example I give
> below. If that fails to build in xcodebuild that's fine, I just want make and
> ninja to match xcodebuild's behavior). (See test/mac for a bunch of
mac-specific
> tests.)
> 
> http://codereview.chromium.org/9382044/diff/1/pylib/gyp/xcode_emulation.py
> File pylib/gyp/xcode_emulation.py (right):
> 
>
http://codereview.chromium.org/9382044/diff/1/pylib/gyp/xcode_emulation.py#ne...
> pylib/gyp/xcode_emulation.py:292: pass
> I believe xcodebuild might default to i386 if ARCHS is empty.
> 
>
http://codereview.chromium.org/9382044/diff/1/pylib/gyp/xcode_emulation.py#ne...
> pylib/gyp/xcode_emulation.py:294: cflags.append('-arch ' + archs[0])
> What if you have this setup:
> 
> {
>   'targets': [
>   {
>     'target_name': 'lib',
>     'type': 'static_library',
>     'sources': 'my_file.cc',
>     # Note: No ARCHS, which (I think) means i386
>   },
>   {
>     'target_name': 'exe',
>     'type': 'executable',
>     'dependencies': [ 'lib', ],
>     'sources': 'my_other_file.cc',
>     'xcode_settings': {
>       'ARCHS': [ 'x86_64', ],
>     },
>   },
> }
> 
> In this case, the executable is built for 64 bit, but the library it links in
is
> only 32 bit. What does xcodebuild do in this case? I believe this will error
out
> in make /ninja with your patch, and I think xcodebuild handles this correctly
> somehow (not sure though).

[Mark Mentovai, 2012-02-13 17:07:07]

Xcode requires ARCHS (although its value may be implicit) and always invokes the
compiler with -arch.

[Nico, 2012-02-14 01:16:43]

What does xcode do for

{
  'targets': [
  {
    'target_name': 'lib',
    'type': 'static_library',
    'sources': 'my_file.cc',
    # Note: No ARCHS, which (I think) means i386
  },
  {
    'target_name': 'exe',
    'type': 'executable',
    'dependencies': [ 'lib', ],
    'sources': 'my_other_file.cc',
    'xcode_settings': {
      'ARCHS': [ 'x86_64', ],
    },
  },
}
?

Can you add a test that tests:

1. Setting ARCHS to just x86_64 and check that the generated binary is 64bit
2. Not setting ARCHS and check that the generated binary is 32bit

?

http://codereview.chromium.org/9382044/diff/4001/pylib/gyp/xcode_emulation.py
File pylib/gyp/xcode_emulation.py (right):

http://codereview.chromium.org/9382044/diff/4001/pylib/gyp/xcode_emulation.py...
pylib/gyp/xcode_emulation.py:296: cflags.append('-arch i386')
nit: It's a little shorter if written like

if len(archs) != 1:
  # TODO: Supporting fat binaries will be annoying.
  self._WarnUnimplemented('ARCHS')
  archs = ['i386']
cflags.append('-arch ' + archs[0])

[ukai, 2012-02-14 05:17:23]

On 2012/02/14 01:16:43, Nico wrote:
> What does xcode do for
> 
> {
>   'targets': [
>   {
>     'target_name': 'lib',
>     'type': 'static_library',
>     'sources': 'my_file.cc',
>     # Note: No ARCHS, which (I think) means i386
>   },
>   {
>     'target_name': 'exe',
>     'type': 'executable',
>     'dependencies': [ 'lib', ],
>     'sources': 'my_other_file.cc',
>     'xcode_settings': {
>       'ARCHS': [ 'x86_64', ],
>     },
>   },
> }
> ?

=== BUILD NATIVE TARGET lib OF PROJECT test WITH THE DEFAULT CONFIGURATION
(Default) ===
Check dependencies
CompileC build/test.build/Default/lib.build/Objects-normal/i386/my_file.o
my_file.cc normal i386 c++ com.apple.compilers.gcc.4_2
    cd /tmp/x
    setenv LANG en_US.US-ASCII
    /Developer/usr/bin/gcc-4.2 -x c++ -arch i386 -fmessage-length=0 -pipe
-Wno-trigraphs -fpascal-strings -fasm-blocks -Os -gdwarf-2
"-I/tmp/x/build/test.build/Default/lib.build/My App.hmap" -F/tmp/x/build/Default
-I/tmp/x/build/Default/include
-I/tmp/x/build/test.build/Default/lib.build/DerivedSources/i386
-I/tmp/x/build/test.build/Default/lib.build/DerivedSources -c /tmp/x/my_file.cc
-o /tmp/x/build/test.build/Default/lib.build/Objects-normal/i386/my_file.o

Libtool "build/Default/libMy App.a" normal i386
    cd /tmp/x
    /Developer/usr/bin/libtool -static -arch_only i386 -L/tmp/x/build/Default
-filelist "/tmp/x/build/test.build/Default/lib.build/Objects-normal/i386/My
App.LinkFileList" -o "/tmp/x/build/Default/libMy App.a"


=== BUILD NATIVE TARGET exe OF PROJECT test WITH THE DEFAULT CONFIGURATION
(Default) ===
Check dependencies
CompileC build/test.build/Default/exe.build/Objects-normal/x86_64/my_main_file.o
my_main_file.cc normal x86_64 c++ com.apple.compilers.gcc.4_2
    cd /tmp/x
    setenv LANG en_US.US-ASCII
    /Developer/usr/bin/gcc-4.2 -x c++ -arch x86_64 -fmessage-length=0 -pipe
-Wno-trigraphs -fpascal-strings -fasm-blocks -Os -mdynamic-no-pic
-fvisibility=hidden -fvisibility-inlines-hidden -gdwarf-2
"-I/tmp/x/build/test.build/Default/exe.build/My App.hmap" -F/tmp/x/build/Default
-I/tmp/x/build/Default/include
-I/tmp/x/build/test.build/Default/exe.build/DerivedSources/x86_64
-I/tmp/x/build/test.build/Default/exe.build/DerivedSources -c
/tmp/x/my_main_file.cc -o
/tmp/x/build/test.build/Default/exe.build/Objects-normal/x86_64/my_main_file.o

Ld "build/Default/My App" normal x86_64
    cd /tmp/x
    /Developer/usr/bin/g++-4.2 -arch x86_64 -L/tmp/x/build/Default
-F/tmp/x/build/Default -filelist
"/tmp/x/build/test.build/Default/exe.build/Objects-normal/x86_64/My
App.LinkFileList" "-lMy App" -prebind -o "/tmp/x/build/Default/My App"

ld: warning: in /tmp/x/build/Default/libMy App.a, file was built for unsupported
file format which is not the architecture being linked (x86_64)
Undefined symbols:
  "_x", referenced from:
      _main in my_main_file.o
ld: symbol(s) not found
collect2: ld returned 1 exit status

=== BUILD AGGREGATE TARGET All OF PROJECT test WITH THE DEFAULT CONFIGURATION
(Default) ===
Check dependencies
** BUILD FAILED **


The following build commands failed:
exe:
	Ld "/tmp/x/build/Default/My App" normal x86_64
(1 failure)



> 
> Can you add a test that tests:
> 
> 1. Setting ARCHS to just x86_64 and check that the generated binary is 64bit
> 2. Not setting ARCHS and check that the generated binary is 32bit
> 
> ?

Yes, Will do.

> 
> http://codereview.chromium.org/9382044/diff/4001/pylib/gyp/xcode_emulation.py
> File pylib/gyp/xcode_emulation.py (right):
> 
>
http://codereview.chromium.org/9382044/diff/4001/pylib/gyp/xcode_emulation.py...
> pylib/gyp/xcode_emulation.py:296: cflags.append('-arch i386')
> nit: It's a little shorter if written like
> 
> if len(archs) != 1:
>   # TODO: Supporting fat binaries will be annoying.
>   self._WarnUnimplemented('ARCHS')
>   archs = ['i386']
> cflags.append('-arch ' + archs[0])

[ukai, 2012-02-14 06:09:33]

http://codereview.chromium.org/9382044/diff/4001/pylib/gyp/xcode_emulation.py
File pylib/gyp/xcode_emulation.py (right):

http://codereview.chromium.org/9382044/diff/4001/pylib/gyp/xcode_emulation.py...
pylib/gyp/xcode_emulation.py:296: cflags.append('-arch i386')
On 2012/02/14 01:16:43, Nico wrote:
> nit: It's a little shorter if written like
> 
> if len(archs) != 1:
>   # TODO: Supporting fat binaries will be annoying.
>   self._WarnUnimplemented('ARCHS')
>   archs = ['i386']
> cflags.append('-arch ' + archs[0])

Done.

[Nico, 2012-02-14 06:25:46]

Lgtm.

Looks great, thanks!

Nit: the CC files need license headers.

http://codereview.chromium.org/9382044/diff/9001/test/mac/gyptest-archs.py
File test/mac/gyptest-archs.py (right):

http://codereview.chromium.org/9382044/diff/9001/test/mac/gyptest-archs.py#ne...
test/mac/gyptest-archs.py:23: test.fail_test()
Is test in scope here?

http://codereview.chromium.org/9382044/diff/9001/test/mac/gyptest-archs.py#ne...
test/mac/gyptest-archs.py:26: 
Nit: 2 space indent

[ukai, 2012-02-14 06:38:39]

http://codereview.chromium.org/9382044/diff/9001/test/mac/gyptest-archs.py
File test/mac/gyptest-archs.py (right):

http://codereview.chromium.org/9382044/diff/9001/test/mac/gyptest-archs.py#ne...
test/mac/gyptest-archs.py:23: test.fail_test()
On 2012/02/14 06:25:46, Nico wrote:
> Is test in scope here?

move L25 before def CheckFileType

http://codereview.chromium.org/9382044/diff/9001/test/mac/gyptest-archs.py#ne...
test/mac/gyptest-archs.py:26: 
On 2012/02/14 06:25:46, Nico wrote:
> Nit: 2 space indent

Done.

[Nico, 2012-02-14 06:43:24]

I'll land this tomorrow morning!

[Nico, 2012-02-14 16:35:35]

On 2012/02/14 06:43:24, Nico wrote:
> I'll land this tomorrow morning!

r1198