mac: Let mac_sdk default to the oldest available SDK that's at least 10.6

mac: Let mac_sdk default to the oldest available SDK that's at least 10.6

Having to manually set mac_sdk was a FAQ when Xcode 4 was released. Now
we're using the 10.6 SDK by default, but Xcode 4.4 ships only with the
10.7 and 10.8 SDKs, so it'll likely become a FAQ again. Automatically
set mac_sdk to something sensible.

(People who explicitly say "mac_sdk=10.6" will have to stop doing so
when they switch to Xcode 4.4.)

BUG=121162
TEST=Build without mac_sdk in GYP_DEFINES. Something useful happens
with Xcode 3.2.6, Xcode 4, Xcode 4.4.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148817

[Mark Mentovai, 2012-07-27 03:31:30]

Cool idea.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/common.gypi#n...
build/common.gypi:646: 'mac_sdk%': '<!(python mac/find_sdk.py)',
I wouldn’t want this to impact official builds. Could you make it conditional?

Also, I don’t remember if this will be executed only once the way this is
written, or if we should do pymod_do_main.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk.py
File build/mac/find_sdk.py (right):

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:15: job = subprocess.Popen(['xcode-select',
'-print-path'],
Didn’t we find that xcode-select wouldn’t be initialized to anything in some
cases? When that happens, do we want to fall back to anything?
/Applications/Xcode.app/Contents/Developer for 4.4+, and /Developer for pre-4.4?

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:20: print out
Wouldn’t you especially want to print err (to stderr) in this case?

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:29: sdks = [re.findall('MacOSX(10\.\d+)\.sdk', s) for s in
os.listdir(sdk_dir)]
Do you want to anchor the RE at the front and back with ^ and $? I think it’s a
good idea.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:29: sdks = [re.findall('MacOSX(10\.\d+)\.sdk', s) for s in
os.listdir(sdk_dir)]
Something in os.listdir(sdk_dir) that doesn’t match the pattern will cause there
to be an empty ([]) element in the sdks list, right? But that’s OK, because the
next line will get rid of those entries, right?

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:31: sdks = [s for s in sdks if s >= '10.6']  # ['10.5',
'10.6'] => ['10.6]
Missing a ' on ['10.6].

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:31: sdks = [s for s in sdks if s >= '10.6']  # ['10.5',
'10.6'] => ['10.6]
This isn’t really the greatest version comparison.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:34: print sdks[0]
Is there any guarantee of sortedness of SDKs? I don’t think so.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:38: if sys.platform == 'darwin':
If you’re going to be conditionalizing when you call this script, you might want
to only call it on Mac, and have it raise an exception when not on Darwin.

[Nico, 2012-07-27 18:01:05]

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/common.gypi#n...
build/common.gypi:646: 'mac_sdk%': '<!(python mac/find_sdk.py)',
On 2012/07/27 03:31:30, Mark Mentovai wrote:
> I wouldn’t want this to impact official builds. Could you make it conditional?

Explicitly setting mac_sdk still overrides the output of the script, this just
sets the default value. (Also, bots all have a 10.6 SDK, so it'll select 10.6 on
bots. If we're paranoid about bots and don't set mac_sdk in the bot configs yet,
we should set it in the bot configs.)

> Also, I don’t remember if this will be executed only once the way this is
> written, or if we should do pymod_do_main.

It doesn't impact gyp_chromium time in a measurable way at least (29.3s with and
without this)

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk.py
File build/mac/find_sdk.py (right):

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:20: print out
On 2012/07/27 03:31:30, Mark Mentovai wrote:
> Wouldn’t you especially want to print err (to stderr) in this case?

Done.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:29: sdks = [re.findall('MacOSX(10\.\d+)\.sdk', s) for s in
os.listdir(sdk_dir)]
On 2012/07/27 03:31:30, Mark Mentovai wrote:
> Do you want to anchor the RE at the front and back with ^ and $? I think it’s
a
> good idea.

Done.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:29: sdks = [re.findall('MacOSX(10\.\d+)\.sdk', s) for s in
os.listdir(sdk_dir)]
On 2012/07/27 03:31:30, Mark Mentovai wrote:
> Something in os.listdir(sdk_dir) that doesn’t match the pattern will cause
there
> to be an empty ([]) element in the sdks list, right? But that’s OK, because
the
> next line will get rid of those entries, right?

Yes.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:31: sdks = [s for s in sdks if s >= '10.6']  # ['10.5',
'10.6'] => ['10.6]
On 2012/07/27 03:31:30, Mark Mentovai wrote:
> This isn’t really the greatest version comparison.

Done.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:31: sdks = [s for s in sdks if s >= '10.6']  # ['10.5',
'10.6'] => ['10.6]
On 2012/07/27 03:31:30, Mark Mentovai wrote:
> Missing a ' on ['10.6].

Done.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:34: print sdks[0]
On 2012/07/27 03:31:30, Mark Mentovai wrote:
> Is there any guarantee of sortedness of SDKs? I don’t think so.

Done.

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/mac/find_sdk....
build/mac/find_sdk.py:38: if sys.platform == 'darwin':
On 2012/07/27 03:31:30, Mark Mentovai wrote:
> If you’re going to be conditionalizing when you call this script, you might
want
> to only call it on Mac, and have it raise an exception when not on Darwin.

I think it's easier this way.

[Mark Mentovai, 2012-07-27 18:18:35]

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10824055/diff/4002/build/common.gypi#n...
build/common.gypi:646: 'mac_sdk%': '<!(python mac/find_sdk.py)',
Nico wrote:
> Explicitly setting mac_sdk still overrides the output of the script, this just
> sets the default value. (Also, bots all have a 10.6 SDK, so it'll select 10.6
on
> bots. If we're paranoid about bots and don't set mac_sdk in the bot configs
yet,
> we should set it in the bot configs.)

We can’t set mac_sdk in the bot configs, that’s the wrong place to do it. “Which
SDK do I use?” is a property of the branch and the associated code. Bots (even
official ones) build multiple branches simultaneously, and the SDK value needs
to change from time to time (as it did last week). Hard-coding it somewhere in
the bot config makes it unnecessarily difficult to make these changes in the
tree or to run SDK changes through the try server.

Furthermore, “official build” means some special things that’s not necessarily
true of a developer build, and the SDK used for the build is part of that. Those
bots shouldn’t just pick whatever SDK they happen to have handy, hoping that
it’s 10.6. This should remain an explicit setting on those bots. That’s why I
asked for this to be made conditional on officialness.

[Nico, 2012-07-27 18:28:54]

On Fri, Jul 27, 2012 at 11:18 AM,  <mark@chromium.org> wrote:
>
> https://chromiumcodereview.appspot.com/10824055/diff/4002/build/common.gypi
> File build/common.gypi (right):
>
>
https://chromiumcodereview.appspot.com/10824055/diff/4002/build/common.gypi#n...
> build/common.gypi:646: 'mac_sdk%': '<!(python mac/find_sdk.py)',
> Nico wrote:
>>
>> Explicitly setting mac_sdk still overrides the output of the script,
>
> this just
>>
>> sets the default value. (Also, bots all have a 10.6 SDK, so it'll
>
> select 10.6 on
>>
>> bots. If we're paranoid about bots and don't set mac_sdk in the bot
>
> configs yet,
>>
>> we should set it in the bot configs.)
>
>
> We can’t set mac_sdk in the bot configs, that’s the wrong place to do
> it. “Which SDK do I use?” is a property of the branch and the associated
> code. Bots (even official ones) build multiple branches simultaneously,
> and the SDK value needs to change from time to time (as it did last
> week). Hard-coding it somewhere in the bot config makes it unnecessarily
> difficult to make these changes in the tree or to run SDK changes
> through the try server.
>
> Furthermore, “official build” means some special things that’s not
> necessarily true of a developer build, and the SDK used for the build is
> part of that. Those bots shouldn’t just pick whatever SDK they happen to
> have handy, hoping that it’s 10.6. This should remain an explicit
> setting on those bots. That’s why I asked for this to be made
> conditional on officialness.

That's a good point. Done.

[Mark Mentovai, 2012-07-27 19:01:54]

https://chromiumcodereview.appspot.com/10824055/diff/6003/build/common.gypi
File build/common.gypi (right):

https://chromiumcodereview.appspot.com/10824055/diff/6003/build/common.gypi#n...
build/common.gypi:640: # non-empty string, mac_sdk_path will be used in
preference to mac_sdk.
Now that mac_sdk is not set here where this whole descriptive comment talking
about SDKs is, and where mac_sdk_path is, you should probably say that mac_sdk
is normally used to control this, but that it’s set somewhere else.

https://chromiumcodereview.appspot.com/10824055/diff/6003/build/common.gypi#n...
build/common.gypi:1021: 'mac_sdk%': '10.6',
Since you’re doing the condition thing anyway, and you’re inside OS=="mac", you
can actually make the script raise an error on non-Mac.

https://chromiumcodereview.appspot.com/10824055/diff/6003/build/common.gypi#n...
build/common.gypi:1029: 'mac_sdk%': '<!(python mac/find_sdk.py)',
I’m slightly inclined to pass 10.6 as an argument to the script from here so
this file is a one-stop shop for setting the [minimum] SDK version, but I don’t
feel strongly enough about it if you don’t like that idea.

[Nico, 2012-07-27 19:11:15]

All done.

On Fri, Jul 27, 2012 at 12:01 PM,  <mark@chromium.org> wrote:
>
> https://chromiumcodereview.appspot.com/10824055/diff/6003/build/common.gypi
> File build/common.gypi (right):
>
>
https://chromiumcodereview.appspot.com/10824055/diff/6003/build/common.gypi#n...
> build/common.gypi:640: # non-empty string, mac_sdk_path will be used in
> preference to mac_sdk.
> Now that mac_sdk is not set here where this whole descriptive comment
> talking about SDKs is, and where mac_sdk_path is, you should probably
> say that mac_sdk is normally used to control this, but that it’s set
> somewhere else.
>
>
https://chromiumcodereview.appspot.com/10824055/diff/6003/build/common.gypi#n...
> build/common.gypi:1021: 'mac_sdk%': '10.6',
> Since you’re doing the condition thing anyway, and you’re inside
> OS=="mac", you can actually make the script raise an error on non-Mac.
>
>
https://chromiumcodereview.appspot.com/10824055/diff/6003/build/common.gypi#n...
> build/common.gypi:1029: 'mac_sdk%': '<!(python mac/find_sdk.py)',
> I’m slightly inclined to pass 10.6 as an argument to the script from
> here so this file is a one-stop shop for setting the [minimum] SDK
> version, but I don’t feel strongly enough about it if you don’t like
> that idea.
>
> https://chromiumcodereview.appspot.com/10824055/

[Mark Mentovai, 2012-07-27 19:13:35]

LGTM

Nice job!

https://chromiumcodereview.appspot.com/10824055/diff/5/build/mac/find_sdk.py
File build/mac/find_sdk.py (right):

https://chromiumcodereview.appspot.com/10824055/diff/5/build/mac/find_sdk.py#...
build/mac/find_sdk.py:11: """Prints the best locally available SDK version to
standard output."""
Something somewhere needs to say that this script is intended to be called with
an argument that tells it what the minimum SDK version to choose is, and this
should probably also document that the script will always choose the lowest
available SDK version that’s at or above the version specified by the argument.

[Nico, 2012-07-27 19:16:15]

Done too. Thanks!

[commit-bot: I haz the power, 2012-07-27 19:16:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10824055/2005

[Mark Mentovai, 2012-07-27 19:17:03]

https://chromiumcodereview.appspot.com/10824055/diff/2005/build/mac/find_sdk.py
File build/mac/find_sdk.py (right):

https://chromiumcodereview.appspot.com/10824055/diff/2005/build/mac/find_sdk....
build/mac/find_sdk.py:46: raise Exception('No 10.6+ SDK found')
This message is not totally correct now.

[Nico, 2012-07-27 19:18:09]

Done too.

[commit-bot: I haz the power, 2012-07-27 19:18:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10824055/8006

[commit-bot: I haz the power, 2012-07-27 21:31:07]

Change committed as 148817