Issue 10704039: Add iOS support to common.gypi

Issue 10704039: Add iOS support to common.gypi (Closed)

Created:
8 years, 5 months ago by stuartmorgan

Modified:
8 years, 5 months ago

Reviewers:
Mark Mentovai

CC:
chromium-reviews

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Add iOS support to common.gypi There are basically three categories of changes here: 1) Most OS==mac becomes OS==mac or OS==ios 2) Some Xcode-related settings move into OS==mac blocks since Xcode is no longer Mac-only 3) Addition of iOS-specific settings (SDK, compile flags, etc) BUG=None TEST=None Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145008

Patch Set 1 #

Patch Set 2 : Fixing branch diffing #

Total comments: 16

Patch Set 3 : Address review comments #

Patch Set 4 : Remove PREBINDING (which had been accidentally kept around in the fork) #

Created: 8 years, 5 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+163 lines, -45 lines)			Patch
	M	build/build_config.h	View	1 2	4 chunks	+13 lines, -2 lines	0 comments	Download
	M	build/common.gypi	View	1 2 3	22 chunks	+150 lines, -43 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

Mark Mentovai

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/build_config.h File build/build_config.h (right): https://chromiumcodereview.appspot.com/10704039/diff/2001/build/build_config.h#newcode20 build/build_config.h:20: #if defined(MAC_TGT_IOS) Who defines this? https://chromiumcodereview.appspot.com/10704039/diff/2001/build/build_config.h#newcode106 build/build_config.h:106: #define WCHAR_T_IS_UNSIGNED ...

8 years, 5 months ago (2012-06-29 13:31:25 UTC) #2

stuartmorgan

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/build_config.h File build/build_config.h (right): https://chromiumcodereview.appspot.com/10704039/diff/2001/build/build_config.h#newcode106 build/build_config.h:106: #define WCHAR_T_IS_UNSIGNED 1 On 2012/06/29 13:31:25, Mark Mentovai wrote: ...

8 years, 5 months ago (2012-06-29 14:58:25 UTC) #3

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/build_config.h
File build/build_config.h (right):

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/build_config....
build/build_config.h:106: #define WCHAR_T_IS_UNSIGNED 1
On 2012/06/29 13:31:25, Mark Mentovai wrote:
> I don’t think this belong in this section to begin with. This section is about
> CPU specifications. The type of wchar_t is at best a factor of the OS and CPU
> together, and correctly belongs with the other wchar_t stuff below.

Done

> I assume that this should be defined for Android.

I have no idea; I've done it as !IOS to preserve behavior since I don't want to
accidentally break a platform I'm not familiar with.

> And the WCHAR_T_IS_UNSIGNED=0 definition in the MIPS group that follows is
> almost certainly unnecessary.

Left a drive-by on https://chromiumcodereview.appspot.com/10448043/ but for now
I'm moving it as-is to preserve since I don't actually know or have any way to
test.

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/common.gypi#n...
build/common.gypi:318: # every single project file for not using the "current"
SDK.
On 2012/06/29 13:31:25, Mark Mentovai wrote:
> But this is also fragile because “current”&nbsp;might not always be 5.0,
right?

We can take another look at this when we switch to the iOS 6 SDK. In practice,
it's probably fine since in reality we generally always want to develop against
the current SDK (unlike OS X, doing that is strongly encouraged *and* seems to
reliably work in combination with an older deployment target).

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/common.gypi#n...
build/common.gypi:1263: # See
http://gcc.gnu.org/onlinedocs/gcc-4.4.2/gcc/Optimize-Options.html
On 2012/06/29 13:31:25, Mark Mentovai wrote:
> See how the Windows comment is duplicated inside each case? That’s not a bad
> idea, because it prevents this comment from becoming orphaned when someone
> decides to refactor this section.

Done.

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/common.gypi#n...
build/common.gypi:2878: 'MAC_TGT_IOS=1',
On 2012/06/29 13:31:25, Mark Mentovai wrote:
> I think this is a little hairy and I’m not convinced that AvailabilityMacros
is
> so awful.

Done.

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/common.gypi#n...
build/common.gypi:2880: # If building for iOS, it means Xcode 4.2+.  So no gcc
is available, so
On 2012/06/29 13:31:25, Mark Mentovai wrote:
> Don’t start sentences with so!
> 
> This comment seems kind of outdated since the Mac build uses nothing but clang
> anyway.

Yeah, it predates the switch. I'll just remove the comment.

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/common.gypi#n...
build/common.gypi:3229: 'TARGETED_DEVICE_FAMILY': '1,2',
On 2012/06/29 13:31:25, Mark Mentovai wrote:
> What’s this do? A comment could help.

The comment above is actually about this line; I'll reword the comment to make
that clearer.

> Should this be something that could be influenced by some variable?

https://chromiumcodereview.appspot.com/10704039/diff/2001/build/common.gypi#n...
build/common.gypi:3232: # Just build armv7 since we're 4.3+ and that is the only
chip set we
On 2012/06/29 13:31:25, Mark Mentovai wrote:
> Reword without saying “we.”

Done.