Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1432313002:
Factor out nmf generation into a GN template (Closed)
|
Created:
5 years, 1 month ago by Petr Hosek Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFactor out nmf generation into a GN template
This change factors out the logic for generating nmf files into a GN
template so that it can be reused across different GN files.
BUG=462791
R=dpranke@chromium.org, brettw@chromium.org, mcgrathr@chromium.org
Committed: https://crrev.com/998705e6e775b7af409775d980bf237ce3652780
Cr-Commit-Position: refs/heads/master@{#359661}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Address (some) of the code review feedback #Patch Set 3 : Better path handling #Patch Set 4 : Allow setting path for staging libraries #
Total comments: 4
Patch Set 5 : Address remaining feedback #Messages
Total messages: 21 (4 generated)
https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni File build/config/nacl/rules.gni (right): https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:8: template("generate_nmf") { All templates should have comments at the top listing what they do (a quick description of what an NMF is would be good since not everybody will know), and all the arguments that they expect. Same for the template below. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:11: objdump = rebase_path("${nacl_toolprefix}objdump") You only use this once, so I'd just inline the rebase call in the args below. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:43: "$root_build_dir/lib32/", It seems surprising to me that depending on a single nacl nmf then inserts every nacl output into my data files. Instead, this should list only what's actually needed (same for the other architectures). Also, this doesn't match the code that writes into it. Above you specify a library path of "${nacl_toolchain_tooldir}/lib32" which is not necessarily the same as the value you write in the outputs here. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:45: } else if (target_cpu == "x64" || (target_cpu == "x86" && is_win)) { I'm confused by these conditions. The first if tests the current_cpu, this one tests the target. I don't think you want to use target_cpu anywhere in this file, since that will only allow you to generate a single type of NMF that matches the current output. If my understanding is correct, I think on Windows you want to be able to generate both 32-bit and 64-bit ones in the same build, which means everything should key off of current_cpu.
https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni File build/config/nacl/rules.gni (right): https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:38: args += [ "--library-path=" + rebase_path(root_out_dir) ] Maybe the library path dir should be listed in inputs? https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:45: } else if (target_cpu == "x64" || (target_cpu == "x86" && is_win)) { Why is Windows special? If this is the proper condition, it needs a comment. But it seems like this should just depend on current_cpu, and the whole thing will be instantiated twice for split 32/64 Windows. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:51: } else if (current_cpu == "arm") { Just make this the default 'else' unless you are going to assert that current_cpu is one of the recognized set. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:92: } else if (target_cpu == "arm") { This can just default to arch = current_cpu. All of these should be current_cpu instead of target_cpu. https://codereview.chromium.org/1432313002/diff/1/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1432313002/diff/1/ppapi/BUILD.gn#newcode325 ppapi/BUILD.gn:325: nmfflags = [ "--stage-dependencies=" + rebase_path(root_build_dir) ] Why isn't this the default behavior of the template?
I defer to Brett and Roland here; you don't need three reviewers :).
https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni File build/config/nacl/rules.gni (right): https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:38: args += [ "--library-path=" + rebase_path(root_out_dir) ] On 2015/11/11 21:44:31, Roland McGrath wrote: > Maybe the library path dir should be listed in inputs? You can't list directories as inputs, only files. If the script's behavior depends on anything in here, though, it should be listed in the inputs and the action should depend on whatever generated that stuff.
On 2015/11/11 22:32:56, brettw wrote: > https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni > File build/config/nacl/rules.gni (right): > > https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... > build/config/nacl/rules.gni:38: args += [ "--library-path=" + > rebase_path(root_out_dir) ] > On 2015/11/11 21:44:31, Roland McGrath wrote: > > Maybe the library path dir should be listed in inputs? > > You can't list directories as inputs, only files. If the script's behavior > depends on anything in here, though, it should be listed in the inputs and the > action should depend on whatever generated that stuff. It depends on the particular libraries and you don't know which ones. But perhaps there is no actual need for these dependencies, because the rebuild_define hack will cause everything to be rebuilt when the toolchain changes anyway.
https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni File build/config/nacl/rules.gni (right): https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:8: template("generate_nmf") { On 2015/11/11 21:20:12, brettw wrote: > All templates should have comments at the top listing what they do (a quick > description of what an NMF is would be good since not everybody will know), and > all the arguments that they expect. > > Same for the template below. Done. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:11: objdump = rebase_path("${nacl_toolprefix}objdump") On 2015/11/11 21:20:12, brettw wrote: > You only use this once, so I'd just inline the rebase call in the args below. Done. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:38: args += [ "--library-path=" + rebase_path(root_out_dir) ] On 2015/11/11 22:32:56, brettw wrote: > On 2015/11/11 21:44:31, Roland McGrath wrote: > > Maybe the library path dir should be listed in inputs? > > You can't list directories as inputs, only files. If the script's behavior > depends on anything in here, though, it should be listed in the inputs and the > action should depend on whatever generated that stuff. I could remove this and it'll be up to the invoker to append that path to nmfflags if it's needed. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:43: "$root_build_dir/lib32/", On 2015/11/11 21:20:12, brettw wrote: > It seems surprising to me that depending on a single nacl nmf then inserts every > nacl output into my data files. Instead, this should list only what's actually > needed (same for the other architectures). > > Also, this doesn't match the code that writes into it. Above you specify a > library path of "${nacl_toolchain_tooldir}/lib32" which is not necessarily the > same as the value you write in the outputs here. That's intentional because those are two different things. The library path is where the runtime libraries are looked for. The scripts then takes all the dependencies and drops them into the lib32/lib64 folder (relative to the current directory). That path can be overridden by passing an extra argument, so I can check whether that argument is being passed in and if so use that path instead of root_build_dir. However, there is no way to find out what those libraries will be ahead of time so I cannot list them (we'd need another script to do that and parse it's output from GN which is probably doable but it's an extra work). https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:45: } else if (target_cpu == "x64" || (target_cpu == "x86" && is_win)) { On 2015/11/11 21:44:32, Roland McGrath wrote: > Why is Windows special? If this is the proper condition, it needs a comment. > But it seems like this should just depend on current_cpu, and the whole thing > will be instantiated twice for split 32/64 Windows. This is taken from GYP and I haven't found any comment as to why it was setup this way, but I guess we can remove this for now and see if it's still needed after we get the Windows GN build working. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:51: } else if (current_cpu == "arm") { On 2015/11/11 21:44:31, Roland McGrath wrote: > Just make this the default 'else' unless you are going to assert that > current_cpu is one of the recognized set. Done. https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:92: } else if (target_cpu == "arm") { On 2015/11/11 21:44:32, Roland McGrath wrote: > This can just default to arch = current_cpu. > All of these should be current_cpu instead of target_cpu. Done.
lgtm https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni File build/config/nacl/rules.gni (right): https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:43: "$root_build_dir/lib32/", Okay, I guess the analogy would be that normal programs all depend on random stuff in /usr/lib. So this seems fine.
On 2015/11/12 17:40:58, brettw wrote: > lgtm > > https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni > File build/config/nacl/rules.gni (right): > > https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... > build/config/nacl/rules.gni:43: "$root_build_dir/lib32/", > Okay, I guess the analogy would be that normal programs all depend on random > stuff in /usr/lib. So this seems fine. I incorporated some more changes, the biggest one is support for "lib_prefix" option which forces the script to install the libraries into a subdirectory in which case we want to add that subdirectory into data (that feature is not used by ppapi tests but it's used by browser tests). That also partially solves the problem you've pointed out because each target can have it's own directory, we still don't know what libraries are going to be installed there but at least these wouldn't get mixed across targets. Is this still lgtm?
https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni File build/config/nacl/rules.gni (right): https://codereview.chromium.org/1432313002/diff/1/build/config/nacl/rules.gni... build/config/nacl/rules.gni:92: } else if (target_cpu == "arm") { On 2015/11/11 23:35:03, Petr Hosek wrote: > On 2015/11/11 21:44:32, Roland McGrath wrote: > > This can just default to arch = current_cpu. > > All of these should be current_cpu instead of target_cpu. > > Done. Actually, in this case we really want target_cpu and not current_cpu because current_cpu will always be pnacl for nonsfi while we're targeting for the target platform (i.e. the cpu of the default toolchain).
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:70001) has been deleted
LGTM with a couple of nits. https://codereview.chromium.org/1432313002/diff/90001/build/config/nacl/rules... File build/config/nacl/rules.gni (right): https://codereview.chromium.org/1432313002/diff/90001/build/config/nacl/rules... build/config/nacl/rules.gni:121: if (target_cpu == "x86") { Add a comment saying why this is target_cpu rather than current_cpu. https://codereview.chromium.org/1432313002/diff/90001/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1432313002/diff/90001/ppapi/BUILD.gn#newcode326 ppapi/BUILD.gn:326: nmfflags = [ "--library-path=" + rebase_path(root_out_dir) ] This seems like it ought to be part of the default behavior of the template. An executable possibly using shared libraries that are part of the same build is not a special case.
https://codereview.chromium.org/1432313002/diff/90001/build/config/nacl/rules... File build/config/nacl/rules.gni (right): https://codereview.chromium.org/1432313002/diff/90001/build/config/nacl/rules... build/config/nacl/rules.gni:121: if (target_cpu == "x86") { On 2015/11/13 19:08:20, Roland McGrath wrote: > Add a comment saying why this is target_cpu rather than current_cpu. Done. https://codereview.chromium.org/1432313002/diff/90001/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1432313002/diff/90001/ppapi/BUILD.gn#newcode326 ppapi/BUILD.gn:326: nmfflags = [ "--library-path=" + rebase_path(root_out_dir) ] On 2015/11/13 19:08:20, Roland McGrath wrote: > This seems like it ought to be part of the default behavior of the template. An > executable possibly using shared libraries that are part of the same build is > not a special case. Done.
lgtm
The CQ bit was checked by phosek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1432313002/#ps110001 (title: "Address remaining feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1432313002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1432313002/110001
Message was sent while issue was closed.
Committed patchset #5 (id:110001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/998705e6e775b7af409775d980bf237ce3652780 Cr-Commit-Position: refs/heads/master@{#359661} |
