Add BUILD.gn for third_party/yasm.

Add BUILD.gn for third_party/yasm.

This is a full conversion of yasm.gyp with some cleanups.

All hail gn. orz

BUG=none

[awong, 2014-04-30 21:02:46]

DO NOT COMMIT.

This is not ready for commit yet, but it illustrates what I've got currently.

I'm thinking of pulling up a template for calling //build/gn_run_binary.py.

I also want to do something with visibility to hide everything except the yasm
binary.

Any idea how to do that?

[awong, 2014-05-01 00:25:05]

Okay, ready for real review.

FYI, this still can't be checked in because there will be a race caused by the
lack of deps support in actions. However, you can make it build by manually
running the targets in tiers.

tier-1:
ninja -C out/mybuild/ genmacro genmodule genperf genstring genversion re2c

tier-2:
ninja -C out/mybuild/ compile_re2c compile_re2c_lc3b generate_license
generate_module generate_version generate_nasm_macros generate_nasm_version
generate_win64_gas generate_win64_nasm generate_x86_insn compile_gperf
compile_gperf_for_include

tier-3:
ninja -C out/mybuild/ yasm

[awong, 2014-05-01 15:33:18]

And now...with a yasm_assemble.gni.

FYI, I gotz the ffmpeg 2 buildz 2.

https://gerrit.chromium.org/gerrit/#/c/69971/

-Albert

On 2014/05/01 00:25:05, awong wrote:
> Okay, ready for real review.
> 
> FYI, this still can't be checked in because there will be a race caused by the
> lack of deps support in actions. However, you can make it build by manually
> running the targets in tiers.
> 
> tier-1:
> ninja -C out/mybuild/ genmacro genmodule genperf genstring genversion re2c
> 
> tier-2:
> ninja -C out/mybuild/ compile_re2c compile_re2c_lc3b generate_license
> generate_module generate_version generate_nasm_macros generate_nasm_version
> generate_win64_gas generate_win64_nasm generate_x86_insn compile_gperf
> compile_gperf_for_include
> 
> tier-3:
> ninja -C out/mybuild/ yasm

[awong, 2014-05-01 21:33:03]

@hclam: OWNERS for third_party/yasm
@brettw: GN style + review of addition of script to //build/.

[brettw, 2014-05-05 16:46:50]

https://codereview.chromium.org/266613002/diff/170001/build/BUILD.gn
File build/BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/170001/build/BUILD.gn#newcode5
build/BUILD.gn:5: source_set("gn_helper_scripts") {
You don't need this target for python files. You can just exec_script random
ones without this.

https://codereview.chromium.org/266613002/diff/170001/build/gn_helper_scripts...
File build/gn_helper_scripts.gni (right):

https://codereview.chromium.org/266613002/diff/170001/build/gn_helper_scripts...
build/gn_helper_scripts.gni:26: if (defined(invoker.deps)) { deps = invoker.deps
}
Can you expand this onto multiple lines? Same above.

https://codereview.chromium.org/266613002/diff/170001/third_party/yasm/BUILD.gn
File third_party/yasm/BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/170001/third_party/yasm/BUILD....
third_party/yasm/BUILD.gn:55: configs -= [
"//build/config/compiler:chromium_code" ]
Can you add back "no_chromium_code". Same in other places below.

https://codereview.chromium.org/266613002/diff/170001/third_party/yasm/yasm_a...
File third_party/yasm/yasm_assemble.gni (right):

https://codereview.chromium.org/266613002/diff/170001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:29: # TODO(ajwong): This should be
is_posix...
We already have an is_posix flag, but there is a mac arm below that wouldn't get
run if we changed it. What do we really want here?

[awong, 2014-05-05 19:28:33]

https://codereview.chromium.org/266613002/diff/170001/build/BUILD.gn
File build/BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/170001/build/BUILD.gn#newcode5
build/BUILD.gn:5: source_set("gn_helper_scripts") {
On 2014/05/05 16:46:50, brettw wrote:
> You don't need this target for python files. You can just exec_script random
> ones without this.

Removed and also removed the datadeps.

Is GN smart enough to run actions if the script changes if you don't declare
this dependency?  IIRC, google3 required you declare various filesets in BUILD
because the dependency graph of the various scripts isn't mineable. For example,
if gn_run_binary.py depends on gn_helpers.py, the target wont' know to rebuild
itself if gn_helpers.py is changed. On the otherhand, if with a bundled datadep,
any change to the scripts in a directory will trigger a rebuild of a target.
This biases away from stale binaries.

https://codereview.chromium.org/266613002/diff/170001/build/gn_helper_scripts...
File build/gn_helper_scripts.gni (right):

https://codereview.chromium.org/266613002/diff/170001/build/gn_helper_scripts...
build/gn_helper_scripts.gni:26: if (defined(invoker.deps)) { deps = invoker.deps
}
On 2014/05/05 16:46:50, brettw wrote:
> Can you expand this onto multiple lines? Same above.

Done.

https://codereview.chromium.org/266613002/diff/170001/third_party/yasm/BUILD.gn
File third_party/yasm/BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/170001/third_party/yasm/BUILD....
third_party/yasm/BUILD.gn:55: configs -= [
"//build/config/compiler:chromium_code" ]
On 2014/05/05 16:46:50, brettw wrote:
> Can you add back "no_chromium_code". Same in other places below.

Done.

https://codereview.chromium.org/266613002/diff/170001/third_party/yasm/yasm_a...
File third_party/yasm/yasm_assemble.gni (right):

https://codereview.chromium.org/266613002/diff/170001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:29: # TODO(ajwong): This should be
is_posix...
On 2014/05/05 16:46:50, brettw wrote:
> We already have an is_posix flag, but there is a mac arm below that wouldn't
get
> run if we changed it. What do we really want here?

I was simulating the yasm_compile.gypi. Really, it's any elf system which
pragmatically is posix - ios - mac

[brettw, 2014-05-05 19:31:21]

https://codereview.chromium.org/266613002/diff/170001/build/BUILD.gn
File build/BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/170001/build/BUILD.gn#newcode5
build/BUILD.gn:5: source_set("gn_helper_scripts") {
Yes, the script itself will be part of the deps of the action dependencies.

[brettw, 2014-05-05 19:34:26]

https://codereview.chromium.org/266613002/diff/210001/third_party/yasm/yasm_a...
File third_party/yasm/yasm_assemble.gni (right):

https://codereview.chromium.org/266613002/diff/210001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:29: # TODO(ajwong): This should be
is_posix...
Myabe just remove this comment and it this block in an else if (is_posix) block
after the Mac ones?

[awong, 2014-05-05 19:47:07]

https://codereview.chromium.org/266613002/diff/210001/third_party/yasm/yasm_a...
File third_party/yasm/yasm_assemble.gni (right):

https://codereview.chromium.org/266613002/diff/210001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:29: # TODO(ajwong): This should be
is_posix...
On 2014/05/05 19:34:26, brettw wrote:
> Myabe just remove this comment and it this block in an else if (is_posix)
block
> after the Mac ones?

Done.

[Alpha Left Google, 2014-05-09 00:51:48]

LGTM. :)

[awong, 2014-05-09 00:52:15]

Brett: How's this look to you?

[brettw, 2014-05-09 20:17:54]

Very neato!

https://codereview.chromium.org/266613002/diff/230001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/230001/BUILD.gn#newcode51
BUILD.gn:51: # TODO(ajwong): Reenable yasm once gn has action dependencies.
This should be fixed now.

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/BUILD.gn
File third_party/yasm/BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/BUILD....
third_party/yasm/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
This file is very complicated. I'm curious how long it takes to run. If you feel
inclined, run:
  gn gen out/whatever --time

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/yasm_a...
File third_party/yasm/yasm_assemble.gni (right):

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:29: if (is_mac || is_ios) {
Do you think this file will be used a lot? If so, you can move these static
conditionals above the template. They'll be run once when the .gni file is
processed, and then the results captured in the closure, rather than executing
it for every instantiation of the template.

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:87: binary_name = "yasm"
While this works for the main toolchain, it won't work for any kind of
cross-compiling.

The current dir for all tools executed is the same as the output dir for the
main toolchain. Say we're targeting Android ARM, the Android binaries will go
into the root build directory, and the host build will go into "out/Debug/x64"
or whatever.

I think gn_run_binary.py should take two arguments, the current toolchain and
the host toolchain. If the're equal, it runs the command from the build dir. If
they're not, it prepends the name of the host toolchain (just grab everything
after the colon) to the command. I also like this because it makes this less of
a way to run random binaries (I don't want to encourage people to run shell
scripts, this is abused in some places in GYP).

[brettw, 2014-05-09 20:56:56]

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/yasm_a...
File third_party/yasm/yasm_assemble.gni (right):

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:29: if (is_mac || is_ios) {
On 2014/05/09 20:17:54, brettw wrote:
> Do you think this file will be used a lot? If so, you can move these static
> conditionals above the template. They'll be run once when the .gni file is
> processed, and then the results captured in the closure, rather than executing
> it for every instantiation of the template.

Oh: prefix the names with _ if you do this so they aren't the same, or bad
things will happen

[awong, 2014-05-10 08:55:34]

https://codereview.chromium.org/266613002/diff/230001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/230001/BUILD.gn#newcode51
BUILD.gn:51: # TODO(ajwong): Reenable yasm once gn has action dependencies.
On 2014/05/09 20:17:54, brettw wrote:
> This should be fixed now.

Done.

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/BUILD.gn
File third_party/yasm/BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/BUILD....
third_party/yasm/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
On 2014/05/09 20:17:54, brettw wrote:
> This file is very complicated. I'm curious how long it takes to run. If you
feel
> inclined, run:
>   gn gen out/whatever --time

http://pastebin.com/AtMzHWqj

3 seconds to parse, 11 seconds to execute. Not so hot.

Oddly enough, this file didn't feel all *that* complex to me. ffmpeg and media
feel a lot worse.

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/yasm_a...
File third_party/yasm/yasm_assemble.gni (right):

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:29: if (is_mac || is_ios) {
On 2014/05/09 20:56:56, brettw wrote:
> On 2014/05/09 20:17:54, brettw wrote:
> > Do you think this file will be used a lot? If so, you can move these static
> > conditionals above the template. They'll be run once when the .gni file is
> > processed, and then the results captured in the closure, rather than
executing
> > it for every instantiation of the template.
> 
> Oh: prefix the names with _ if you do this so they aren't the same, or bad
> things will happen
> 

Should GN generate a warning or error on a shadow inside a template if bad
things will happen?

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:29: if (is_mac || is_ios) {
On 2014/05/09 20:17:54, brettw wrote:
> Do you think this file will be used a lot? If so, you can move these static
> conditionals above the template. They'll be run once when the .gni file is
> processed, and then the results captured in the closure, rather than executing
> it for every instantiation of the template.

It's not super common, but moved it out anyways.  The ivory-tower purist in me
wants to say that if a basic block doesn't depend on "invoker" inside a
template, maybe it should just be evaluated when the template is parsed and
remembered...

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/yasm_a...
third_party/yasm/yasm_assemble.gni:87: binary_name = "yasm"
On 2014/05/09 20:17:54, brettw wrote:
> While this works for the main toolchain, it won't work for any kind of
> cross-compiling.
> 
> The current dir for all tools executed is the same as the output dir for the
> main toolchain. Say we're targeting Android ARM, the Android binaries will go
> into the root build directory, and the host build will go into "out/Debug/x64"
> or whatever.
> 
> I think gn_run_binary.py should take two arguments, the current toolchain and
> the host toolchain. If the're equal, it runs the command from the build dir.
If
> they're not, it prepends the name of the host toolchain (just grab everything
> after the colon) to the command. I also like this because it makes this less
of
> a way to run random binaries (I don't want to encourage people to run shell
> scripts, this is abused in some places in GYP).

I think I finally understand enough to make this work. The run_executable
extraction layers I added mean no changes in this file.

However, I just tried setting cpu_arch=x86 via gn args and cross compiling. I'm
having a number of problems so I'm thinking this code doesn't work for
cross-compiles. :(

[brettw, 2014-05-11 20:30:33]

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/BUILD.gn
File third_party/yasm/BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/230001/third_party/yasm/BUILD....
third_party/yasm/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights
reserved.
Those are milliseconds so I think we're OK. Thanks for checking.

[brettw, 2014-05-12 16:28:59]

I'm adding a new function which might be relevant:
https://codereview.chromium.org/279023002/
With this function you can say:
  get_label_info("//third_party/yasm($host_toolchain)", "root_out_dir")
to get the directory containing the binary for yasm, taking into account all of
the toolchain stuff. I think run_executable should take a label of the
executable to run, then the template converts it to a path, and then pass it off
to the python script.

[awong, 2014-06-10 01:26:17]

On 2014/05/12 16:28:59, brettw wrote:
> I'm adding a new function which might be relevant:
> https://codereview.chromium.org/279023002/
> With this function you can say:
>   get_label_info("//third_party/yasm($host_toolchain)", "root_out_dir")
> to get the directory containing the binary for yasm, taking into account all
of
> the toolchain stuff. I think run_executable should take a label of the
> executable to run, then the template converts it to a path, and then pass it
off
> to the python script.

I'm going to punt on this comment in favor of just getting this in.

[awong, 2014-06-10 01:48:23]

The CQ bit was checked by ajwong@chromium.org

[commit-bot: I haz the power, 2014-06-10 01:50:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ajwong@chromium.org/266613002/270001

[commit-bot: I haz the power, 2014-06-10 16:17:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-10 16:22:52]

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

[commit-bot: I haz the power, 2014-06-10 16:22:54]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[brettw, 2014-06-10 21:32:31]

https://codereview.chromium.org/266613002/diff/170001/build/BUILD.gn
File build/BUILD.gn (right):

https://codereview.chromium.org/266613002/diff/170001/build/BUILD.gn#newcode5
build/BUILD.gn:5: source_set("gn_helper_scripts") {
On 2014/05/05 19:31:21, brettw wrote:
> Yes, the script itself will be part of the deps of the action dependencies.

I only responded to part of this. When you exec a script the script itself will
be part of the dependency list. But if that imports something else it won't.
I'll look at this.