Add option make_clang_dir to GN

Add option make_clang_dir to GN

Same as the GYP flag with the same name. Useful for e.g. compiling with
Icecc.

BUG=None

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

[mkollaro, 2016-01-20 14:53:36]

martina.kollarova@intel.com changed reviewers:
+ agrieve@chromium.org, jbudorick@chromium.org

[mkollaro, 2016-01-20 14:53:37]

Maybe it could be called use_clang_dir?

[dshwang, 2016-01-20 15:04:08]

dongseong.hwang@intel.com changed reviewers:
+ dongseong.hwang@intel.com

[dshwang, 2016-01-20 15:04:08]

cool! lgtm with nits

[dshwang, 2016-01-20 15:05:06]

https://codereview.chromium.org/1610093003/diff/1/build/toolchain/gcc_toolcha...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1610093003/diff/1/build/toolchain/gcc_toolcha...
build/toolchain/gcc_toolchain.gni:455: prefix = use_clang_prefix
how about naming similar to gyp 'make_clang_dir'?

[mkollaro, 2016-01-21 14:33:34]

Description was changed from

==========
Add option use_clang_prefix to GN

Same as the GYP flag make_clang_dir. Useful for e.g. compiling with
Icecc.

BUG=None
==========

to

==========
Add option make_clang_dir to GN

Same as the GYP flag with the same name. Useful for e.g. compiling with
Icecc.

BUG=None
==========

[mkollaro, 2016-01-21 14:33:59]

https://codereview.chromium.org/1610093003/diff/1/build/toolchain/gcc_toolcha...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1610093003/diff/1/build/toolchain/gcc_toolcha...
build/toolchain/gcc_toolchain.gni:455: prefix = use_clang_prefix
On 2016/01/20 15:05:05, dshwang wrote:
> how about naming similar to gyp 'make_clang_dir'?

Done.

[dshwang, 2016-01-21 19:36:59]

On 2016/01/21 14:33:59, mkollaro wrote:
>
https://codereview.chromium.org/1610093003/diff/1/build/toolchain/gcc_toolcha...
> File build/toolchain/gcc_toolchain.gni (right):
> 
>
https://codereview.chromium.org/1610093003/diff/1/build/toolchain/gcc_toolcha...
> build/toolchain/gcc_toolchain.gni:455: prefix = use_clang_prefix
> On 2016/01/20 15:05:05, dshwang wrote:
> > how about naming similar to gyp 'make_clang_dir'?
> 
> Done.

thx! lgtm as non-owner

[mkollaro, 2016-01-25 09:39:40]

Ping? Could one of the owners please review?

[jbudorick, 2016-01-25 15:26:17]

jbudorick@chromium.org changed reviewers:
+ dpranke@chromium.org

[jbudorick, 2016-01-25 15:26:17]

While I think this looks fine, I'm not the right person to give owners on this.
+dpranke

[Dirk Pranke, 2016-01-25 20:47:46]

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

[Dirk Pranke, 2016-01-25 20:47:47]

+thakis, in case he has a leaning here as well ...

https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
build/toolchain/gcc_toolchain.gni:430: make_clang_dir = "default"
this seems kinda crufty.

Can you just do:

  declare_args() {
    clang_dir = "//third_party/llvm-build/Release+Asserts/bin"
  }

and change line 452 to

  prefix = rebase_path(clang_dir, root_build_dir)

instead?

It's better to use a good variable name than it is to match GYP ...

[Nico, 2016-01-25 21:07:19]

https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
build/toolchain/gcc_toolchain.gni:430: make_clang_dir = "default"
On 2016/01/25 20:47:46, Dirk Pranke wrote:
> this seems kinda crufty.
> 
> Can you just do:
> 
>   declare_args() {
>     clang_dir = "//third_party/llvm-build/Release+Asserts/bin"
>   }
> 
> and change line 452 to
> 
>   prefix = rebase_path(clang_dir, root_build_dir)
> 
> instead?
> 
> It's better to use a good variable name than it is to match GYP ...

I agree that the current name isn't great. It's historically named after a
feature in gyp's make generator (hence "make_") -- it shouldn't have this name
in gn :-)

I'd suggest defaulting it to //third_party/llvm-build/Release+Asserts/ (no bin/)
though, since some bits will probably need stuff below lib/ too.

[mkollaro, 2016-01-26 09:03:37]

https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
File build/toolchain/gcc_toolchain.gni (right):

https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
build/toolchain/gcc_toolchain.gni:430: make_clang_dir = "default"
On 2016/01/25 20:47:46, Dirk Pranke wrote:
> this seems kinda crufty.
> 
> Can you just do:
> 
>   declare_args() {
>     clang_dir = "//third_party/llvm-build/Release+Asserts/bin"
>   }
> 
> and change line 452 to
> 
>   prefix = rebase_path(clang_dir, root_build_dir)
> 
> instead?
> 
> It's better to use a good variable name than it is to match GYP ...

Done.

https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
build/toolchain/gcc_toolchain.gni:430: make_clang_dir = "default"
On 2016/01/25 21:07:19, Nico wrote:
> On 2016/01/25 20:47:46, Dirk Pranke wrote:
> > this seems kinda crufty.
> > 
> > Can you just do:
> > 
> >   declare_args() {
> >     clang_dir = "//third_party/llvm-build/Release+Asserts/bin"
> >   }
> > 
> > and change line 452 to
> > 
> >   prefix = rebase_path(clang_dir, root_build_dir)
> > 
> > instead?
> > 
> > It's better to use a good variable name than it is to match GYP ...
> 
> I agree that the current name isn't great. It's historically named after a
> feature in gyp's make generator (hence "make_") -- it shouldn't have this name
> in gn :-)
> 
> I'd suggest defaulting it to //third_party/llvm-build/Release+Asserts/ (no
bin/)
> though, since some bits will probably need stuff below lib/ too.

I'm not sure how to do that in a reasonable manner - if I do

  prefix = rebase_path(clang_dir + "/bin", root_build_dir)

then the user who wants to change clang_dir would have to point it to the parent
directory, which is not very intuitive. I'd rather leave it with the "bin"
suffix, because YAGNI.

[Dirk Pranke, 2016-01-26 17:41:35]

I'm fine w/ leaving "bin" in for now, so lgtm .

But Nico should weigh in as well.

[Nico, 2016-01-26 17:43:24]

On 2016/01/26 09:03:37, mkollaro wrote:
>
https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
> File build/toolchain/gcc_toolchain.gni (right):
> 
>
https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
> build/toolchain/gcc_toolchain.gni:430: make_clang_dir = "default"
> On 2016/01/25 20:47:46, Dirk Pranke wrote:
> > this seems kinda crufty.
> > 
> > Can you just do:
> > 
> >   declare_args() {
> >     clang_dir = "//third_party/llvm-build/Release+Asserts/bin"
> >   }
> > 
> > and change line 452 to
> > 
> >   prefix = rebase_path(clang_dir, root_build_dir)
> > 
> > instead?
> > 
> > It's better to use a good variable name than it is to match GYP ...
> 
> Done.
> 
>
https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
> build/toolchain/gcc_toolchain.gni:430: make_clang_dir = "default"
> On 2016/01/25 21:07:19, Nico wrote:
> > On 2016/01/25 20:47:46, Dirk Pranke wrote:
> > > this seems kinda crufty.
> > > 
> > > Can you just do:
> > > 
> > >   declare_args() {
> > >     clang_dir = "//third_party/llvm-build/Release+Asserts/bin"
> > >   }
> > > 
> > > and change line 452 to
> > > 
> > >   prefix = rebase_path(clang_dir, root_build_dir)
> > > 
> > > instead?
> > > 
> > > It's better to use a good variable name than it is to match GYP ...
> > 
> > I agree that the current name isn't great. It's historically named after a
> > feature in gyp's make generator (hence "make_") -- it shouldn't have this
name
> > in gn :-)
> > 
> > I'd suggest defaulting it to //third_party/llvm-build/Release+Asserts/ (no
> bin/)
> > though, since some bits will probably need stuff below lib/ too.
> 
> I'm not sure how to do that in a reasonable manner - if I do
> 
>   prefix = rebase_path(clang_dir + "/bin", root_build_dir)
> 
> then the user who wants to change clang_dir would have to point it to the
parent
> directory, which is not very intuitive. I'd rather leave it with the "bin"
> suffix, because YAGNI.

That's what you have to do in gyp too. If you don't do this, then once we also
have a dep that needs lib/, the alternative is having to set two variables and
if you forget that you end up with mixed bin/ and lib/ dirs, which seems worse.
And since we need this in gyp, I don't think YAGNI applies here. Not doing this
now just make things harder later.

[mkollaro, 2016-01-28 15:03:27]

On 2016/01/26 17:43:24, Nico wrote:
> On 2016/01/26 09:03:37, mkollaro wrote:
> >
>
https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
> > File build/toolchain/gcc_toolchain.gni (right):
> > 
> >
>
https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
> > build/toolchain/gcc_toolchain.gni:430: make_clang_dir = "default"
> > On 2016/01/25 20:47:46, Dirk Pranke wrote:
> > > this seems kinda crufty.
> > > 
> > > Can you just do:
> > > 
> > >   declare_args() {
> > >     clang_dir = "//third_party/llvm-build/Release+Asserts/bin"
> > >   }
> > > 
> > > and change line 452 to
> > > 
> > >   prefix = rebase_path(clang_dir, root_build_dir)
> > > 
> > > instead?
> > > 
> > > It's better to use a good variable name than it is to match GYP ...
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1610093003/diff/20001/build/toolchain/gcc_too...
> > build/toolchain/gcc_toolchain.gni:430: make_clang_dir = "default"
> > On 2016/01/25 21:07:19, Nico wrote:
> > > On 2016/01/25 20:47:46, Dirk Pranke wrote:
> > > > this seems kinda crufty.
> > > > 
> > > > Can you just do:
> > > > 
> > > >   declare_args() {
> > > >     clang_dir = "//third_party/llvm-build/Release+Asserts/bin"
> > > >   }
> > > > 
> > > > and change line 452 to
> > > > 
> > > >   prefix = rebase_path(clang_dir, root_build_dir)
> > > > 
> > > > instead?
> > > > 
> > > > It's better to use a good variable name than it is to match GYP ...
> > > 
> > > I agree that the current name isn't great. It's historically named after a
> > > feature in gyp's make generator (hence "make_") -- it shouldn't have this
> name
> > > in gn :-)
> > > 
> > > I'd suggest defaulting it to //third_party/llvm-build/Release+Asserts/ (no
> > bin/)
> > > though, since some bits will probably need stuff below lib/ too.
> > 
> > I'm not sure how to do that in a reasonable manner - if I do
> > 
> >   prefix = rebase_path(clang_dir + "/bin", root_build_dir)
> > 
> > then the user who wants to change clang_dir would have to point it to the
> parent
> > directory, which is not very intuitive. I'd rather leave it with the "bin"
> > suffix, because YAGNI.
> 
> That's what you have to do in gyp too. If you don't do this, then once we also
> have a dep that needs lib/, the alternative is having to set two variables and
> if you forget that you end up with mixed bin/ and lib/ dirs, which seems
worse.
> And since we need this in gyp, I don't think YAGNI applies here. Not doing
this
> now just make things harder later.

My mistake, I didn't notice that /bin isn't part of make_clang_dir in gyp.
Thanks, I changed it now.

[Nico, 2016-01-28 15:07:04]

thanks, this lgtm

[mkollaro, 2016-01-29 07:55:09]

The CQ bit was checked by martina.kollarova@intel.com

[mkollaro, 2016-01-29 07:55:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dongseong.hwang@intel.com, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1610093003/#ps60001
(title: "remove /bin suffix")

[commit-bot: I haz the power, 2016-01-29 07:55:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1610093003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1610093003/60001

[commit-bot: I haz the power, 2016-01-29 08:28:56]

Description was changed from

==========
Add option make_clang_dir to GN

Same as the GYP flag with the same name. Useful for e.g. compiling with
Icecc.

BUG=None
==========

to

==========
Add option make_clang_dir to GN

Same as the GYP flag with the same name. Useful for e.g. compiling with
Icecc.

BUG=None
==========

[commit-bot: I haz the power, 2016-01-29 08:28:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-29 08:29:46]

Description was changed from

==========
Add option make_clang_dir to GN

Same as the GYP flag with the same name. Useful for e.g. compiling with
Icecc.

BUG=None
==========

to

==========
Add option make_clang_dir to GN

Same as the GYP flag with the same name. Useful for e.g. compiling with
Icecc.

BUG=None

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

[commit-bot: I haz the power, 2016-01-29 08:29:47]

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