Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1434823002:
Build NaCl browser tests with GN (Closed)
|
Created:
5 years, 1 month ago by Petr Hosek Modified:
5 years ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBuild NaCl browser tests with GN
Support for building NaCl browser tests under GN.
BUG=462791
Committed: https://crrev.com/aceb9d08d689f31ef987bdc56c5630640807e09a
Cr-Commit-Position: refs/heads/master@{#362075}
Patch Set 1 #
Total comments: 39
Patch Set 2 : Address code review feedback #Patch Set 3 : Move the nacl group to the top of the file #
Total comments: 6
Patch Set 4 : Resolve code review feedback #
Total comments: 7
Patch Set 5 : Comments added #Patch Set 6 : Rebase #
Messages
Total messages: 34 (11 generated)
Description was changed from ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. Only newlib, glibc pnacl toolchain is supported at the moment. BUG=462791 ========== to ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. Only newlib, glibc pnacl toolchain is supported at the moment. BUG=462791 ==========
phosek@chromium.org changed reviewers: + brettw@chromium.org, dpranke@chromium.org, mcgrathr@chromium.org
I plan to port non-SFI browser tests in a separate patch since those will add another layer of complexity.
The "only" in the log message means, "everything but nonsfi", I take it? Perhaps say that explicitly. Or perhaps just but a TODO comment about nonsfi support into the source file. https://codereview.chromium.org/1434823002/diff/1/build/config/nacl/BUILD.gn File build/config/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/build/config/nacl/BUILD.gn#... build/config/nacl/BUILD.gn:21: defines += [ "NACL_BUILD_ARCH=pnacl" ] What needs this? I thought we'd changed the code not to require the NACL_BUILD_* predefines any more. https://codereview.chromium.org/1434823002/diff/1/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/chrome/test/BUILD.gn#newcod... chrome/test/BUILD.gn:958: "//chrome/test/data/nacl", Don't we usually use an explicit :something here? Even if it's just "...:*"? https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD.gn File chrome/test/data/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:8: if (enable_nacl) { Why nested if and double indentation instead of if (enable_nacl && is_nacl) ? In fact, if !enable_nacl, there will never be an is_nacl incarnation, will there? So perhaps just if (is_nacl) is sufficient. Or perhaps it would be better to make it assert(is_nacl, "...") and no conditional on the body. This file shouldn't have any references to it that aren't in the right toolchain context, should it? https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:34: nmf_cpu = "arm" Make it nmf_cpu = target_cpu unless you are going to assert arm. And then you can drop the explicit x86 if case too. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:37: suffix = "glibc_${nmf_cpu}" Could just be suffix = "${variant}_${nmf_cpu}" In fact, target_cpu and current_cpu should always match in an is_nacl context, I think. And current_cpu is probably most proper to test regardless. So the whole thing can be: if (current_cpu == "x64") { nmf_cpu = "x86_64" } else { nmf_cpu = current_cpu } suffixed_target_name = "${base_target_name}_${variant}_${nmf_cpu}" https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:71: cflags = invoker.cflags Use forward_variables_from(invoker, ["cflags", "ldflags"]). https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:81: libs = [ "pthread" ] For C++, the compiler driver should take care of adding -lpthread, which needs to come after the implicit -lstdc++ it adds. So if you have to add anything, it's -pthread to ldcflags (or perhaps just cflags). https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:89: libs += invoker.libs Without the "phtread" case if I'm right that that's not what you want, libs can just be thrown into the forward_variables_from(...) above. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:100: "${root_out_dir}/${suffixed_target_name}.pexe.debug", After a recent change to the pnacl toolchain definition, there is no longer a .pexe.debug file. Instead it's in exe.unstripped/foo.pexe analogous to where the unstripped file goes for other toolchains. But that only exists in the is_debug case. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:104: "${root_out_dir}/${suffixed_target_name}.nexe", I think you can use {{output_extension}} to get .pexe or .nexe as appropriate to the toolchain. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:120: nexe = "$root_build_dir/${destination_dir}/${variant}/${suffixed_target_name}.pexe" {{output_extension}} https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:138: if (is_nacl_glibc) { It would be nice to share all this create_nmf logic with the ppapi test BUILD.gn code that does the same thing. Can it be refactored into a template for a generate_nmf action usable by all cases? https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:552: # Source file does not have asm for ARM. Probably easy to add it and make this unconditional. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:571: deps = [ It might be easier to read this list if you used a local variable for each of the full toolchain names, e.g. ":foo($newlib)", ":foo($glibc)", ":foo($pnacl)".
Description was changed from ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. Only newlib, glibc pnacl toolchain is supported at the moment. BUG=462791 ========== to ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. All but non-SFI toolchains are supported. BUG=462791 ==========
Description was changed from ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. All but non-SFI toolchains are supported. BUG=462791 ========== to ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. BUG=462791 ==========
I mostly defer to Roland here ... https://codereview.chromium.org/1434823002/diff/1/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/chrome/test/BUILD.gn#newcod... chrome/test/BUILD.gn:958: "//chrome/test/data/nacl", On 2015/11/11 06:22:45, Roland McGrath wrote: > Don't we usually use an explicit :something here? > Even if it's just "...:*"? not using a ":something" defaults to ":dirname", e.g., "//chrome/test/data/nacl:nacl". GN does not support wildcards. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD.gn File chrome/test/data/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:8: if (enable_nacl) { On 2015/11/11 06:22:45, Roland McGrath wrote: > Why nested if and double indentation instead of if (enable_nacl && is_nacl) ? > > In fact, if !enable_nacl, there will never be an is_nacl incarnation, will > there? So perhaps just if (is_nacl) is sufficient. Or perhaps it would be > better to make it assert(is_nacl, "...") and no conditional on the body. This > file shouldn't have any references to it that aren't in the right toolchain > context, should it? I agree that assert(is_nacl, "...") and no indentation is the way to go if possible. https://codereview.chromium.org/1434823002/diff/1/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/ppapi/BUILD.gn#newcode152 ppapi/BUILD.gn:152: static_library("ppapi_cpp_lib") { why this change? https://codereview.chromium.org/1434823002/diff/1/ppapi/native_client/src/unt... File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/ppapi/native_client/src/unt... ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:8: static_library("ppapi_stub_lib") { why this change?
https://codereview.chromium.org/1434823002/diff/1/build/config/nacl/BUILD.gn File build/config/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/build/config/nacl/BUILD.gn#... build/config/nacl/BUILD.gn:21: defines += [ "NACL_BUILD_ARCH=pnacl" ] On 2015/11/11 06:22:44, Roland McGrath wrote: > What needs this? I thought we'd changed the code not to require the > NACL_BUILD_* predefines any more. PNaCl tests which depend on //native_client/src/shared/platform (which seems to be needed for NaClLog). This dependency includes trusted/service_runtime/nacl_config.h, which causes build error because NACL_BUILD_ARCH is undefined. There's a TODO in include/build_config.h from teravest@ related to this which doesn't seem to be fixed yet. https://codereview.chromium.org/1434823002/diff/1/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/chrome/test/BUILD.gn#newcod... chrome/test/BUILD.gn:958: "//chrome/test/data/nacl", On 2015/11/11 06:22:45, Roland McGrath wrote: > Don't we usually use an explicit :something here? > Even if it's just "...:*"? GN defaults to the name of the directory, so this line is equivalent to "//chrome/test/data/nacl:nacl". I can rename the target if you prefer. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD.gn File chrome/test/data/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:8: if (enable_nacl) { On 2015/11/11 22:19:05, Dirk Pranke wrote: > On 2015/11/11 06:22:45, Roland McGrath wrote: > > Why nested if and double indentation instead of if (enable_nacl && is_nacl) ? > > > > In fact, if !enable_nacl, there will never be an is_nacl incarnation, will > > there? So perhaps just if (is_nacl) is sufficient. Or perhaps it would be > > better to make it assert(is_nacl, "...") and no conditional on the body. This > > file shouldn't have any references to it that aren't in the right toolchain > > context, should it? > > I agree that assert(is_nacl, "...") and no indentation is the way to go if > possible. I've dropped enable_nacl but I can't drop is_nacl because the "nacl" group is deps'ed in with a host toolchain. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:34: nmf_cpu = "arm" On 2015/11/11 06:22:45, Roland McGrath wrote: > Make it nmf_cpu = target_cpu unless you are going to assert arm. And then you > can drop the explicit x86 if case too. Done. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:37: suffix = "glibc_${nmf_cpu}" On 2015/11/11 06:22:45, Roland McGrath wrote: > Could just be suffix = "${variant}_${nmf_cpu}" > > In fact, target_cpu and current_cpu should always match in an is_nacl context, I > think. And current_cpu is probably most proper to test regardless. So the > whole thing can be: > > if (current_cpu == "x64") { nmf_cpu = "x86_64" } > else { nmf_cpu = current_cpu } > suffixed_target_name = "${base_target_name}_${variant}_${nmf_cpu}" Except that this breaks for pnacl in which case we end up with "${base_target_name}_pnacl_pnacl". https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:71: cflags = invoker.cflags On 2015/11/11 06:22:45, Roland McGrath wrote: > Use forward_variables_from(invoker, ["cflags", "ldflags"]). Done. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:81: libs = [ "pthread" ] On 2015/11/11 06:22:45, Roland McGrath wrote: > For C++, the compiler driver should take care of adding -lpthread, which needs > to come after the implicit -lstdc++ it adds. So if you have to add anything, > it's -pthread to ldcflags (or perhaps just cflags). Done. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:89: libs += invoker.libs On 2015/11/11 06:22:45, Roland McGrath wrote: > Without the "phtread" case if I'm right that that's not what you want, libs can > just be thrown into the forward_variables_from(...) above. Removed as it's no longer needed. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:100: "${root_out_dir}/${suffixed_target_name}.pexe.debug", On 2015/11/11 06:22:45, Roland McGrath wrote: > After a recent change to the pnacl toolchain definition, there is no longer a > .pexe.debug file. Instead it's in exe.unstripped/foo.pexe analogous to where > the unstripped file goes for other toolchains. But that only exists in the > is_debug case. Done. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:104: "${root_out_dir}/${suffixed_target_name}.nexe", On 2015/11/11 06:22:45, Roland McGrath wrote: > I think you can use {{output_extension}} to get .pexe or .nexe as appropriate to > the toolchain. Unfortunately, {{output_extension}} only works within the toolchain definition. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:120: nexe = "$root_build_dir/${destination_dir}/${variant}/${suffixed_target_name}.pexe" On 2015/11/11 06:22:45, Roland McGrath wrote: > {{output_extension}} ditto https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:138: if (is_nacl_glibc) { On 2015/11/11 06:22:45, Roland McGrath wrote: > It would be nice to share all this create_nmf logic with the ppapi test BUILD.gn > code that does the same thing. Can it be refactored into a template for a > generate_nmf action usable by all cases? Done in https://codereview.chromium.org/1432313002/ https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:552: # Source file does not have asm for ARM. On 2015/11/11 06:22:45, Roland McGrath wrote: > Probably easy to add it and make this unconditional. That would have to be done on the NaCl side though. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:571: deps = [ On 2015/11/11 06:22:45, Roland McGrath wrote: > It might be easier to read this list if you used a local variable for each of > the full toolchain names, e.g. ":foo($newlib)", ":foo($glibc)", ":foo($pnacl)". Done. https://codereview.chromium.org/1434823002/diff/1/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/ppapi/BUILD.gn#newcode152 ppapi/BUILD.gn:152: static_library("ppapi_cpp_lib") { On 2015/11/11 22:19:06, Dirk Pranke wrote: > why this change? Because some of the NaCl browser tests redefine some symbols from this library; that only works when this is a static library but with source set we get multiple symbol definition errors. https://codereview.chromium.org/1434823002/diff/1/ppapi/native_client/src/unt... File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/ppapi/native_client/src/unt... ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:8: static_library("ppapi_stub_lib") { On 2015/11/11 22:19:06, Dirk Pranke wrote: > why this change? ditto
https://codereview.chromium.org/1434823002/diff/1/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/chrome/test/BUILD.gn#newcod... chrome/test/BUILD.gn:958: "//chrome/test/data/nacl", On 2015/11/13 22:18:32, Petr Hosek wrote: > On 2015/11/11 06:22:45, Roland McGrath wrote: > > Don't we usually use an explicit :something here? > > Even if it's just "...:*"? > > GN defaults to the name of the directory, so this line is equivalent to > "//chrome/test/data/nacl:nacl". I can rename the target if you prefer. Don't rename the target. This is the preferred style. https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD.gn File chrome/test/data/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:8: if (enable_nacl) { On 2015/11/13 22:18:33, Petr Hosek wrote: > On 2015/11/11 22:19:05, Dirk Pranke wrote: > > On 2015/11/11 06:22:45, Roland McGrath wrote: > > > Why nested if and double indentation instead of if (enable_nacl && is_nacl) > ? > > > > > > In fact, if !enable_nacl, there will never be an is_nacl incarnation, will > > > there? So perhaps just if (is_nacl) is sufficient. Or perhaps it would be > > > better to make it assert(is_nacl, "...") and no conditional on the body. > This > > > file shouldn't have any references to it that aren't in the right toolchain > > > context, should it? > > > > I agree that assert(is_nacl, "...") and no indentation is the way to go if > > possible. > > I've dropped enable_nacl but I can't drop is_nacl because the "nacl" group is > deps'ed in with a host toolchain. Ah, okay. It might be a bit clearer if you can move group("nacl") above the if block, then (especially since the target name matches the directory name). https://codereview.chromium.org/1434823002/diff/1/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/ppapi/BUILD.gn#newcode152 ppapi/BUILD.gn:152: static_library("ppapi_cpp_lib") { On 2015/11/13 22:18:33, Petr Hosek wrote: > On 2015/11/11 22:19:06, Dirk Pranke wrote: > > why this change? > > Because some of the NaCl browser tests redefine some symbols from this library; > that only works when this is a static library but with source set we get > multiple symbol definition errors. Ew. That seems bad. Can you file a separate bug for that?
https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD.gn File chrome/test/data/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/chrome/test/data/nacl/BUILD... chrome/test/data/nacl/BUILD.gn:8: if (enable_nacl) { On 2015/11/13 23:00:57, Dirk Pranke wrote: > On 2015/11/13 22:18:33, Petr Hosek wrote: > > On 2015/11/11 22:19:05, Dirk Pranke wrote: > > > On 2015/11/11 06:22:45, Roland McGrath wrote: > > > > Why nested if and double indentation instead of if (enable_nacl && > is_nacl) > > ? > > > > > > > > In fact, if !enable_nacl, there will never be an is_nacl incarnation, will > > > > there? So perhaps just if (is_nacl) is sufficient. Or perhaps it would > be > > > > better to make it assert(is_nacl, "...") and no conditional on the body. > > This > > > > file shouldn't have any references to it that aren't in the right > toolchain > > > > context, should it? > > > > > > I agree that assert(is_nacl, "...") and no indentation is the way to go if > > > possible. > > > > I've dropped enable_nacl but I can't drop is_nacl because the "nacl" group is > > deps'ed in with a host toolchain. > > Ah, okay. > > It might be a bit clearer if you can move group("nacl") above the if block, then > (especially since the target name matches the directory name). Done. https://codereview.chromium.org/1434823002/diff/1/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/1/ppapi/BUILD.gn#newcode152 ppapi/BUILD.gn:152: static_library("ppapi_cpp_lib") { On 2015/11/13 23:00:57, Dirk Pranke wrote: > On 2015/11/13 22:18:33, Petr Hosek wrote: > > On 2015/11/11 22:19:06, Dirk Pranke wrote: > > > why this change? > > > > Because some of the NaCl browser tests redefine some symbols from this > library; > > that only works when this is a static library but with source set we get > > multiple symbol definition errors. > > Ew. That seems bad. Can you file a separate bug for that? Done, see https://code.google.com/p/chromium/issues/detail?id=556811
Would it be possible to get this re-reviwed so I can land it?
Mostly LGTM, just some nits, and should have try runs for the latest patch version. https://codereview.chromium.org/1434823002/diff/40001/build/config/nacl/BUILD.gn File build/config/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/40001/build/config/nacl/BUILD... build/config/nacl/BUILD.gn:21: defines += [ "NACL_BUILD_ARCH=pnacl" ] Looks like this should be NACL_pnacl, not just pnacl. Also, put a TODO here because it's clear the intent in native_client/src/include/build_config.h is that this be taken care of by predefines. teravest left the group part way through this cleanup project. https://codereview.chromium.org/1434823002/diff/40001/chrome/test/data/nacl/B... File chrome/test/data/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/40001/chrome/test/data/nacl/B... chrome/test/data/nacl/BUILD.gn:98: nmf_cpu = current_cpu Is this right for x86? I thought we used x86_32 in the nmf files. https://codereview.chromium.org/1434823002/diff/40001/chrome/test/data/nacl/B... chrome/test/data/nacl/BUILD.gn:235: # files that aren't assosiated with any particular executable. "associated". Capitalize the sentence.
https://codereview.chromium.org/1434823002/diff/40001/build/config/nacl/BUILD.gn File build/config/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/40001/build/config/nacl/BUILD... build/config/nacl/BUILD.gn:21: defines += [ "NACL_BUILD_ARCH=pnacl" ] On 2015/11/18 20:36:58, Roland McGrath wrote: > Looks like this should be NACL_pnacl, not just pnacl. > Also, put a TODO here because it's clear the intent in > native_client/src/include/build_config.h is that this be taken care of by > predefines. teravest left the group part way through this cleanup project. pnacl is correct in this case, NACL_ARCH macro prepends the NACL_ prefix. TODO added. https://codereview.chromium.org/1434823002/diff/40001/chrome/test/data/nacl/B... File chrome/test/data/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/40001/chrome/test/data/nacl/B... chrome/test/data/nacl/BUILD.gn:98: nmf_cpu = current_cpu On 2015/11/18 20:36:58, Roland McGrath wrote: > Is this right for x86? I thought we used x86_32 in the nmf files. Done. https://codereview.chromium.org/1434823002/diff/40001/chrome/test/data/nacl/B... chrome/test/data/nacl/BUILD.gn:235: # files that aren't assosiated with any particular executable. On 2015/11/18 20:36:58, Roland McGrath wrote: > "associated". Capitalize the sentence. Done.
Brett, can you do an OWNERS review of this?
lgtm https://codereview.chromium.org/1434823002/diff/60001/build/config/nacl/BUILD.gn File build/config/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/60001/build/config/nacl/BUILD... build/config/nacl/BUILD.gn:21: # TODO: This be taken care of by predefines. I can't understand this comment. I don't know what "predefines" are and I don't know what it's trying to say: this code should be removed when something else happens? https://codereview.chromium.org/1434823002/diff/60001/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/60001/ppapi/BUILD.gn#newcode153 ppapi/BUILD.gn:153: static_library("ppapi_cpp_lib") { Why was this changed? If something depends on a static library to link properly, I think there should be a comment. Otherwise, we should keep it a source set. https://codereview.chromium.org/1434823002/diff/60001/ppapi/native_client/src... File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/60001/ppapi/native_client/src... ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:8: static_library("ppapi_stub_lib") { Ditto
wait, not lgtm, pressed the wrong button.
https://codereview.chromium.org/1434823002/diff/60001/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/60001/ppapi/BUILD.gn#newcode153 ppapi/BUILD.gn:153: static_library("ppapi_cpp_lib") { On 2015/11/19 19:08:16, brettw wrote: > Why was this changed? If something depends on a static library to link properly, > I think there should be a comment. Otherwise, we should keep it a source set. A comment certainly doesn't hurt. But these are static libraries that were always intended to be static libraries and are delivered as static libraries in the NaCl SDK. When the SDK build switches to using GN, it will need these to be libraries anyway.
https://codereview.chromium.org/1434823002/diff/60001/build/config/nacl/BUILD.gn File build/config/nacl/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/60001/build/config/nacl/BUILD... build/config/nacl/BUILD.gn:21: # TODO: This be taken care of by predefines. On 2015/11/19 19:08:16, brettw wrote: > I can't understand this comment. I don't know what "predefines" are and I don't > know what it's trying to say: this code should be removed when something else > happens? Updated. https://codereview.chromium.org/1434823002/diff/60001/ppapi/BUILD.gn File ppapi/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/60001/ppapi/BUILD.gn#newcode153 ppapi/BUILD.gn:153: static_library("ppapi_cpp_lib") { On 2015/11/19 19:16:13, Roland McGrath wrote: > On 2015/11/19 19:08:16, brettw wrote: > > Why was this changed? If something depends on a static library to link > properly, > > I think there should be a comment. Otherwise, we should keep it a source set. > > A comment certainly doesn't hurt. But these are static libraries that were > always intended to be static libraries and are delivered as static libraries in > the NaCl SDK. When the SDK build switches to using GN, it will need these to be > libraries anyway. Comment added. https://codereview.chromium.org/1434823002/diff/60001/ppapi/native_client/src... File ppapi/native_client/src/untrusted/irt_stub/BUILD.gn (right): https://codereview.chromium.org/1434823002/diff/60001/ppapi/native_client/src... ppapi/native_client/src/untrusted/irt_stub/BUILD.gn:8: static_library("ppapi_stub_lib") { On 2015/11/19 19:08:16, brettw wrote: > Ditto Done.
Is there anything else you want me to fix?
lgtm
The CQ bit was checked by phosek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcgrathr@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1434823002/#ps100001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1434823002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1434823002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
phosek@chromium.org changed reviewers: + bradnelson@chromium.org, mseaborn@chromium.org
Mark or Brad, could you do OWNERS review for chrome/test/data/nacl/BUILD.gn?
lgtm
The CQ bit was checked by phosek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1434823002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1434823002/100001
Message was sent while issue was closed.
Description was changed from ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. BUG=462791 ========== to ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. BUG=462791 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. BUG=462791 ========== to ========== Build NaCl browser tests with GN Support for building NaCl browser tests under GN. BUG=462791 Committed: https://crrev.com/aceb9d08d689f31ef987bdc56c5630640807e09a Cr-Commit-Position: refs/heads/master@{#362075} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/aceb9d08d689f31ef987bdc56c5630640807e09a Cr-Commit-Position: refs/heads/master@{#362075} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
