Add a variable use_system_xcode to GN.

Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
TBR=sdefresne@chromium.org

Committed: https://crrev.com/b58a723ab22333fe40575f6992c3cb8c7dde0b0a
Cr-Commit-Position: refs/heads/master@{#423673}

[erikchen, 2016-10-03 23:15:43]

Description was changed from

==========
Begin the process of making all tools hermeticized.

BUG=651267
==========

to

==========
Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
==========

[erikchen, 2016-10-03 23:15:56]

Description was changed from

==========
Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
==========

to

==========
Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
==========

[erikchen, 2016-10-03 23:16:44]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 23:17:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2016-10-03 23:17:37]

erikchen@chromium.org changed reviewers:
+ dpranke@chromium.org, thakis@chromium.org

[erikchen, 2016-10-03 23:17:38]

dpranke, thakis: PTAL

Assuming this looks reasonable, I'll finish plumbing the parameter through
WebKit [there are some slightly less trivial python changes], and other
repositories.

[erikchen, 2016-10-03 23:27:06]

erikchen@chromium.org changed reviewers:
+ justincohen@chromium.org

[justincohen, 2016-10-03 23:46:18]

Does
https://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.11%20F...
have CLT installed?

[erikchen, 2016-10-04 00:09:28]

On 2016/10/03 23:46:18, justincohen wrote:
> Does
>
https://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.11%20F...
> have CLT installed?

Yes it does:
"""
chrome-bot@build9-m1:(Mac 10.11.4):~$ xcode-select -p
/Library/Developer/CommandLineTools
"""

[Dirk Pranke, 2016-10-04 20:25:40]

lgtm as-is, though I'd probably prefer the single-GN-arg approach if it can be
made to work (see comments below).

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/toolcha...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/toolcha...
build/toolchain/toolchain.gni:54: rebase_path("//build/mac_files/Xcode.app", "",
root_build_dir)
Can we just set xcode_path to "/Applications/Xcode.app" (or whatever the
equivalent is) by default, and not need two flags?

On a related note, on windows, we configure things by calling out to a script
(vs_toolchain.py) to figure out whether to use the hermetic toolchain or a
system one, and just return the path to use, which then gets passed around as
appropriate. See build/config/win/visual_studio_version.gni for the details.

It seems like it would be good to mirror that approach here.

But we can probably also make such changes later, when we're closer to have
hermetic be on by default.

WDYT?

https://codereview.chromium.org/2388063003/diff/60001/chrome/app/nibs/generat...
File chrome/app/nibs/generate_localizer.py (right):

https://codereview.chromium.org/2388063003/diff/60001/chrome/app/nibs/generat...
chrome/app/nibs/generate_localizer.py:55: ibtool_env['DEVELOPER_DIR'] =
xcode_path
This logic is a little strange, in that you copy() the environment in one case
but not the others, particularly when you're adding a var in the latter case.

Does it make sense to unconditionally clone the environment just modify the
cloned instance?

[Nico, 2016-10-04 20:39:33]

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:68: env_wrapper = "export
DEVELOPER_DIR=$hermetic_xcode_path; "
why not pass this as an arg to the libtool / ld wrapper script?

...oh, looks like we have an ld wrapper script on linux/android but not on os x?
why is this different? the linkers are almost identical. do we want an ld
wrapper script here?

Come to think of it, does the env var do anything but tell the ld trampoline
which real ld binary to invoke? Could we just call the right ld binary directory
if !use_system_xcode?

[erikchen, 2016-10-05 19:57:19]

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:68: env_wrapper = "export
DEVELOPER_DIR=$hermetic_xcode_path; "
On 2016/10/04 20:39:33, Nico wrote:
> why not pass this as an arg to the libtool / ld wrapper script?
> 
> ...oh, looks like we have an ld wrapper script on linux/android but not on os
x?
> why is this different? the linkers are almost identical. do we want an ld
> wrapper script here?
> 
> Come to think of it, does the env var do anything but tell the ld trampoline
> which real ld binary to invoke? Could we just call the right ld binary
directory
> if !use_system_xcode?

Note that sometimes we use linker_driver.py, which in turn may invoke 3
different xcode binaries: dsymutil, strip and ld. Is there a particular reason
you want to see us specify absolute paths to binaries [which relies on the
assumption that these binaries don't call out to other xcode binaries, which I
haven't checked] rather than setting a DEVELOPER_DIR? The latter seems a lot
simpler.

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/toolcha...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/toolcha...
build/toolchain/toolchain.gni:54: rebase_path("//build/mac_files/Xcode.app", "",
root_build_dir)
On 2016/10/04 20:25:40, Dirk Pranke (slow) wrote:
> Can we just set xcode_path to "/Applications/Xcode.app" (or whatever the
> equivalent is) by default, and not need two flags?
> 
> On a related note, on windows, we configure things by calling out to a script
> (vs_toolchain.py) to figure out whether to use the hermetic toolchain or a
> system one, and just return the path to use, which then gets passed around as
> appropriate. See build/config/win/visual_studio_version.gni for the details.
> 
> It seems like it would be good to mirror that approach here.
> 
> But we can probably also make such changes later, when we're closer to have
> hermetic be on by default.
> 
> WDYT?

I like the idea of only having a single variable that is always set. This will
also simplify a lot of the logic.

But I also want there to be a single boolean parameter that users can configure
to switch between hermetic and non-hermetic. Maybe the script should check
FORCE_MAC_TOOLCHAIN and there will be no GN parameter. But you're moving
FORCE_MAC_TOOLCHAIN into a gclient config...which seems like the wrong place to
specify this type of build configuration. Should we discuss this more when you
get back?

https://codereview.chromium.org/2388063003/diff/60001/chrome/app/nibs/generat...
File chrome/app/nibs/generate_localizer.py (right):

https://codereview.chromium.org/2388063003/diff/60001/chrome/app/nibs/generat...
chrome/app/nibs/generate_localizer.py:55: ibtool_env['DEVELOPER_DIR'] =
xcode_path
On 2016/10/04 20:25:40, Dirk Pranke (slow) wrote:
> This logic is a little strange, in that you copy() the environment in one case
> but not the others, particularly when you're adding a var in the latter case.
> 
> Does it make sense to unconditionally clone the environment just modify the
> cloned instance?

Good point, done.

[Nico, 2016-10-05 20:00:44]

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/mac/BUI...
File build/toolchain/mac/BUILD.gn (right):

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/mac/BUI...
build/toolchain/mac/BUILD.gn:68: env_wrapper = "export
DEVELOPER_DIR=$hermetic_xcode_path; "
On 2016/10/05 19:57:19, erikchen wrote:
> On 2016/10/04 20:39:33, Nico wrote:
> > why not pass this as an arg to the libtool / ld wrapper script?
> > 
> > ...oh, looks like we have an ld wrapper script on linux/android but not on
os
> x?
> > why is this different? the linkers are almost identical. do we want an ld
> > wrapper script here?
> > 
> > Come to think of it, does the env var do anything but tell the ld trampoline
> > which real ld binary to invoke? Could we just call the right ld binary
> directory
> > if !use_system_xcode?
> 
> Note that sometimes we use linker_driver.py, which in turn may invoke 3
> different xcode binaries: dsymutil, strip and ld. Is there a particular reason
> you want to see us specify absolute paths to binaries [which relies on the
> assumption that these binaries don't call out to other xcode binaries, which I
> haven't checked] rather than setting a DEVELOPER_DIR? The latter seems a lot
> simpler.

Mostly "less magic" -- we'd understand better what's actually happening.

[erikchen, 2016-10-05 20:23:41]

> Mostly "less magic" -- we'd understand better what's actually happening.

Point taken. Note that your response applies to all uses of Xcode binaries. I
don't think that moving from DEVELOPER_DIR -> explicit binaries is an efficient
use of developer time given that there's no change in functionality.

[Dirk Pranke, 2016-10-06 01:08:44]

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/toolcha...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2388063003/diff/60001/build/toolchain/toolcha...
build/toolchain/toolchain.gni:54: rebase_path("//build/mac_files/Xcode.app", "",
root_build_dir)
On 2016/10/05 19:57:19, erikchen wrote:
> On 2016/10/04 20:25:40, Dirk Pranke (slow) wrote:
> > Can we just set xcode_path to "/Applications/Xcode.app" (or whatever the
> > equivalent is) by default, and not need two flags?
> > 
> > On a related note, on windows, we configure things by calling out to a
script
> > (vs_toolchain.py) to figure out whether to use the hermetic toolchain or a
> > system one, and just return the path to use, which then gets passed around
as
> > appropriate. See build/config/win/visual_studio_version.gni for the details.
> > 
> > It seems like it would be good to mirror that approach here.
> > 
> > But we can probably also make such changes later, when we're closer to have
> > hermetic be on by default.
> > 
> > WDYT?
> 
> I like the idea of only having a single variable that is always set. This will
> also simplify a lot of the logic.
> 
> But I also want there to be a single boolean parameter that users can
configure
> to switch between hermetic and non-hermetic. Maybe the script should check
> FORCE_MAC_TOOLCHAIN and there will be no GN parameter. But you're moving
> FORCE_MAC_TOOLCHAIN into a gclient config...which seems like the wrong place
to
> specify this type of build configuration. Should we discuss this more when you
> get back?

Good point re: a simple flag.

We can certainly discuss more when I'm back in (likely tomorrow), and this CL
doesn't
need to be blocked on a decision either way, so two flags for now is fine.

The way I expect the gclient integration to work is that whether we pulled in
the hermetic
toolchain will affect what the default build setting is, but it won't solely
control it (i.e., you
can't build w/ the hermetic toolchain if you don't have it, but you can still
turn it off).

[erikchen, 2016-10-06 17:13:29]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-10-06 17:13:29]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2388063003/#ps80001
(title: "Comments from dpranke.")

[commit-bot: I haz the power, 2016-10-06 17:13:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 17:23:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 17:23:55]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erikchen, 2016-10-06 17:38:17]

erikchen@chromium.org changed reviewers:
+ sdefresne@chromium.org

[erikchen, 2016-10-06 17:38:18]

sdefresne: Please review components/
thakis: Does chrome/ lg to you?

[Nico, 2016-10-06 17:48:13]

yes, lgtm

I'm kind of unhappy with the linker situation though. Why do we have both a py
wrapper and a bunch of shell magic? Not your doing and mostly unrelated to this
CL though.

[Dirk Pranke, 2016-10-06 18:09:18]

On 2016/10/06 17:48:13, Nico wrote:
> yes, lgtm
> 
> I'm kind of unhappy with the linker situation though. Why do we have both a py
> wrapper and a bunch of shell magic? Not your doing and mostly unrelated to
this
> CL though.

Agreed. I'd like to push the shell logic into the wrapper if possible; I really
don't care for
having GN issue strings of shell commands.

[erikchen, 2016-10-06 20:49:53]

Description was changed from

==========
Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
==========

to

==========
Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
TBR=sdefresne@chromium.org
==========

[erikchen, 2016-10-06 20:50:08]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-10-06 20:50:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 20:59:15]

Description was changed from

==========
Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
TBR=sdefresne@chromium.org
==========

to

==========
Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
TBR=sdefresne@chromium.org
==========

[commit-bot: I haz the power, 2016-10-06 20:59:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-06 21:02:24]

Description was changed from

==========
Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
TBR=sdefresne@chromium.org
==========

to

==========
Add a variable use_system_xcode to GN.

Start plumbing it through to actions that require binaries from the Xcode
toolchain.

BUG=651267
TBR=sdefresne@chromium.org

Committed: https://crrev.com/b58a723ab22333fe40575f6992c3cb8c7dde0b0a
Cr-Commit-Position: refs/heads/master@{#423673}
==========

[commit-bot: I haz the power, 2016-10-06 21:02:26]

Patchset 5 (id:??) landed as
https://crrev.com/b58a723ab22333fe40575f6992c3cb8c7dde0b0a
Cr-Commit-Position: refs/heads/master@{#423673}

[sdefresne, 2016-10-07 13:25:16]

lgtm

https://codereview.chromium.org/2388063003/diff/80001/build/config/mac/compil...
File build/config/mac/compile_xib.py (right):

https://codereview.chromium.org/2388063003/diff/80001/build/config/mac/compil...
build/config/mac/compile_xib.py:26: if (args.developer_dir):
style: parenthesis should have been omitted there

https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s...

https://codereview.chromium.org/2388063003/diff/80001/build/toolchain/toolcha...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2388063003/diff/80001/build/toolchain/toolcha...
build/toolchain/toolchain.gni:53: hermetic_xcode_path =
nit: do you expect developer to change this variable? if not, maybe it would be
better to put it outside of "declare_args()" block

[erikchen, 2016-10-07 23:25:42]

https://codereview.chromium.org/2388063003/diff/80001/build/toolchain/toolcha...
File build/toolchain/toolchain.gni (right):

https://codereview.chromium.org/2388063003/diff/80001/build/toolchain/toolcha...
build/toolchain/toolchain.gni:53: hermetic_xcode_path =
On 2016/10/07 13:25:16, sdefresne wrote:
> nit: do you expect developer to change this variable? if not, maybe it would
be
> better to put it outside of "declare_args()" block

I expect to use this when I rev the hermetic toolchain. Not sure about others...

[sdefresne, 2016-10-10 02:29:20]

On 2016/10/07 23:25:42, erikchen wrote:
>
https://codereview.chromium.org/2388063003/diff/80001/build/toolchain/toolcha...
> File build/toolchain/toolchain.gni (right):
> 
>
https://codereview.chromium.org/2388063003/diff/80001/build/toolchain/toolcha...
> build/toolchain/toolchain.gni:53: hermetic_xcode_path =
> On 2016/10/07 13:25:16, sdefresne wrote:
> > nit: do you expect developer to change this variable? if not, maybe it would
> be
> > better to put it outside of "declare_args()" block
> 
> I expect to use this when I rev the hermetic toolchain. Not sure about
others...

This could probably be used when testing new version of Xcode too, so make sense
to have it as a configurable variable.

[Nico, 2016-10-10 15:01:21]

On Sun, Oct 9, 2016 at 10:29 PM, <sdefresne@chromium.org> wrote:

> On 2016/10/07 23:25:42, erikchen wrote:
> >
> https://codereview.chromium.org/2388063003/diff/80001/
> build/toolchain/toolchain.gni
> > File build/toolchain/toolchain.gni (right):
> >
> >
> https://codereview.chromium.org/2388063003/diff/80001/
> build/toolchain/toolchain.gni#newcode53
> > build/toolchain/toolchain.gni:53: hermetic_xcode_path =
> > On 2016/10/07 13:25:16, sdefresne wrote:
> > > nit: do you expect developer to change this variable? if not, maybe it
> would
> > be
> > > better to put it outside of "declare_args()" block
> >
> > I expect to use this when I rev the hermetic toolchain. Not sure about
> others...
>
> This could probably be used when testing new version of Xcode too, so make
> sense
> to have it as a configurable variable.
>

I don't think "for local testing" is a good motivation for making variables
configurable. Just changing the default in the .gn file locally is no
harder than typing in the override. If we put in a config var for
everything people used locally, we'd have confusingly many configurables.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Nico, 2016-10-10 15:01:35]

On Sun, Oct 9, 2016 at 10:29 PM, <sdefresne@chromium.org> wrote:

> On 2016/10/07 23:25:42, erikchen wrote:
> >
> https://codereview.chromium.org/2388063003/diff/80001/
> build/toolchain/toolchain.gni
> > File build/toolchain/toolchain.gni (right):
> >
> >
> https://codereview.chromium.org/2388063003/diff/80001/
> build/toolchain/toolchain.gni#newcode53
> > build/toolchain/toolchain.gni:53: hermetic_xcode_path =
> > On 2016/10/07 13:25:16, sdefresne wrote:
> > > nit: do you expect developer to change this variable? if not, maybe it
> would
> > be
> > > better to put it outside of "declare_args()" block
> >
> > I expect to use this when I rev the hermetic toolchain. Not sure about
> others...
>
> This could probably be used when testing new version of Xcode too, so make
> sense
> to have it as a configurable variable.
>

I don't think "for local testing" is a good motivation for making variables
configurable. Just changing the default in the .gn file locally is no
harder than typing in the override. If we put in a config var for
everything people used locally, we'd have confusingly many configurables.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.