Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815453004:
For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64. (Closed)
DescriptionFor building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64(both LE and BE).
Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively -
https://codereview.chromium.org/2809963004/
https://codereview.chromium.org/2812173002/
R=machenbach@chromium.org, dpranke@chromium.org, adamk@chromium.org
BUG=706728
Review-Url: https://codereview.chromium.org/2815453004
Cr-Commit-Position: refs/heads/master@{#470463}
Committed: https://chromium.googlesource.com/chromium/src/+/367a04209fcb6c3700daee65511945da7dd31f20
Patch Set 1 #
Total comments: 36
Patch Set 2 : rebased, addressed reviews, added host_byteorder.gni. #
Total comments: 6
Patch Set 3 : addressed reviews #
Total comments: 2
Patch Set 4 : clarified the need for host_byteorder.gni #
Total comments: 13
Patch Set 5 : addressed reviews #Patch Set 6 : rebased #Patch Set 7 : removed the is_power_or_z variable, the cpu checks are now done inline #Patch Set 8 : removed the /build/config/features.gni changes #Patch Set 9 : rebased #
Messages
Total messages: 75 (36 generated)
Description was changed from ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64. Also added Host byteorder logic. Related to the v8_Issue= BUG=706728 ========== to ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64. Also added Host byteorder logic. Work in progess. Related to the v8_Issue= BUG=706728 ==========
Description was changed from ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64. Also added Host byteorder logic. Work in progess. Related to the v8_Issue= BUG=706728 ========== to ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64. Also added Host byteorder logic. Work in progress. Related to the v8_Issue=2809963004 BUG=706728 ==========
Description was changed from ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64. Also added Host byteorder logic. Work in progress. Related to the v8_Issue=2809963004 BUG=706728 ========== to ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64. Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively - https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ R=machenbach@chromium.org, dpranke@chromium.org, agrieve@chromium.org BUG=706728 ==========
Description was changed from ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64. Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively - https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ R=machenbach@chromium.org, dpranke@chromium.org, agrieve@chromium.org BUG=706728 ========== to ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64 (both LE and BE). Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively - https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ R=machenbach@chromium.org, dpranke@chromium.org, agrieve@chromium.org BUG=706728 ==========
Description was changed from ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64 (both LE and BE). Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively - https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ R=machenbach@chromium.org, dpranke@chromium.org, agrieve@chromium.org BUG=706728 ========== to ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64(both LE and BE). Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively - https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ R=machenbach@chromium.org, dpranke@chromium.org, agrieve@chromium.org BUG=706728 ==========
Description was changed from ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64(both LE and BE). Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively - https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ R=machenbach@chromium.org, dpranke@chromium.org, agrieve@chromium.org BUG=706728 ========== to ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64(both LE and BE). Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively - https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ R=machenbach@chromium.org, dpranke@chromium.org, adamk@chromium.org BUG=706728 ==========
rayb@ca.ibm.com changed reviewers: + adamk@chromium.org, dpranke@chromium.org, machenbach@chromium.org
ptal
dpranke@chromium.org changed reviewers: + brettw@chromium.org
I'm strongly tempted to add is_aix, is_ppc, and is_s390 to BUILDCONFIG.gn so that there are fewer string compare checks against current_cpu and current_os (and the others). Brettw, WDYT of this CL? https://codereview.chromium.org/2815453004/diff/1/build/build_config.h File build/build_config.h (right): https://codereview.chromium.org/2815453004/diff/1/build/build_config.h#newcode83 build/build_config.h:83: defined(OS_ANDROID) || defined(OS_NACL) || defined(OS_QNX) || \ It seems like some of these checks are duplicated. Can you fix that? https://codereview.chromium.org/2815453004/diff/1/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2815453004/diff/1/build/config/features.gni#n... build/config/features.gni:33: target_cpu != "ppc64") Are you planning to port NaCl to these architectures? I'm guessing not, and maybe we can just simplify this logic to only look at the target_cpus?
https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn... build/config/BUILDCONFIG.gn:79: host_byteorder = exec_script("//build/get_byteorder.py", [], "trim string") Maybe we should make host_byteorder a built in variable (like host_cpu) instead?
Lg if non-nit comments addressed and Dirk agrees. Most of my comments are readability suggestions. https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn... build/config/BUILDCONFIG.gn:158: current_os == "chromeos" nit: Maybe move the chromeos clause before the linux clause for readability (to have the most complicated clause last)? Not sure what formatting does to it though... https://codereview.chromium.org/2815453004/diff/1/build/config/aix/BUILD.gn File build/config/aix/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/aix/BUILD.gn#n... build/config/aix/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2815453004/diff/1/build/config/aix/BUILD.gn#n... build/config/aix/BUILD.gn:7: import("//build/toolchain/toolchain.gni") Not all these imports look necessary. https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:183: cflags_cc += [ This doesn't include -fno-strict-aliasing in cflags anymore. Also for the non-aix/non-ppc/etc cases. If this is intended, I'd prefer to have it in a separate CL and with separate description. https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1151: current_cpu != "ppc64" && current_cpu != "ppc")) { nit: For readability, maybe we could add a helper variable somewhere for: current_cpu != "s390x" && current_cpu != "s390" && current_cpu != "ppc64" && current_cpu != "ppc" Or rather the inverse case, used in line 181. This or the inverse case are used many times in multiple files. The four parts of the clause are always going together and are hard to read if nested within other clauses. https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1385: if (current_os != "aix") { This is already nested in a current_os != "aix" scope. https://codereview.chromium.org/2815453004/diff/1/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2815453004/diff/1/build/config/features.gni#n... build/config/features.gni:30: (current_cpu != "s390" && current_cpu != "s390x" && nit: Why parentheses? https://codereview.chromium.org/2815453004/diff/1/build/dotfile_settings.gni File build/dotfile_settings.gni (right): https://codereview.chromium.org/2815453004/diff/1/build/dotfile_settings.gni#... build/dotfile_settings.gni:13: "//build/config/BUILDCONFIG.gn", nit: maybe alphasort this rather with A-Za-z order? https://codereview.chromium.org/2815453004/diff/1/build/get_byteorder.py File build/get_byteorder.py (right): https://codereview.chromium.org/2815453004/diff/1/build/get_byteorder.py#newc... build/get_byteorder.py:2: # Copyright 2014 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2815453004/diff/1/build/get_byteorder.py#newc... build/get_byteorder.py:16: sys.exit(main()) nit: Wonder if the boiler plate is necessary. Maybe replace lines 11-16 with: print sys.byteorder Or is gn loading the script as a library and calling main? https://codereview.chromium.org/2815453004/diff/1/build/toolchain/aix/BUILD.gn File build/toolchain/aix/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/toolchain/aix/BUILD.g... build/toolchain/aix/BUILD.gn:1: # Copyright 2013 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2815453004/diff/1/build/toolchain/aix/BUILD.g... build/toolchain/aix/BUILD.gn:5: import("//build/config/sysroot.gni") Why is sysroot included here? https://codereview.chromium.org/2815453004/diff/1/build/toolchain/gcc_ar_wrap... File build/toolchain/gcc_ar_wrapper_aix.py (right): https://codereview.chromium.org/2815453004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper_aix.py:2: # Copyright 2015 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2815453004/diff/1/build/toolchain/gcc_ar_wrap... build/toolchain/gcc_ar_wrapper_aix.py:34: parser.add_argument('--resource-whitelist', nit: This is unused. Not sure if relevant. The original file does some stuff with it. Though not really clear to me what: https://cs.chromium.org/chromium/src/build/toolchain/gcc_ar_wrapper.py?q=gcc_... These 3 lines are also the only difference between the new and the old file. any chance you can modify the original to do what you want without breaking the old code? https://codereview.chromium.org/2815453004/diff/1/build/toolchain/gcc_toolcha... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2815453004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:320: ar_wrapper = rebase_path("//build/toolchain/gcc_ar_wrapper_aix.py", Maybe pull out the two differences (ar_wrapper, operation) into variables and then have the command-variable line only once? https://codereview.chromium.org/2815453004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:506: link_command = "$ld {{ldflags}}${extra_ldflags} -o \"$unstripped_outfile\" -Wl,--start-group @\"$rspfile\" {{solibs}} -Wl,--end-group $libs_section_prefix {{libs}} $libs_section_postfix" nit: Readability: Similar to my comment above, maybe factor out two variables and have the line for link_command only once? Similar to how the whitelist_flag variable is used above.
Also, I take back what I said about adding is_aix et al. Let's not do that until we have a sense of how broadly that might be needed. https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn... build/config/BUILDCONFIG.gn:79: host_byteorder = exec_script("//build/get_byteorder.py", [], "trim string") I've changed my mind. To repeat my latest comment from the v8-side patch: I think we should leave this here, at least until we have a need for host_byteorder somewhere outside of v8. If we have that need at that time, we should start by creating a separate host_byteorder.gni file, rather than setting it in BUILDCONFIG. We should avoid changes to BUILDCONFIG if at all possible.
On 2017/04/18 20:31:31, Dirk Pranke wrote: > Also, I take back what I said about adding is_aix et al. Let's not do that until > we have a sense of how broadly that might be needed. > > https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn > File build/config/BUILDCONFIG.gn (right): > > https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn... > build/config/BUILDCONFIG.gn:79: host_byteorder = > exec_script("//build/get_byteorder.py", [], "trim string") > I've changed my mind. To repeat my latest comment from the v8-side patch: > > I think we should leave this here, at least until we have a need for > host_byteorder somewhere outside of v8. > > If we have that need at that time, we should start by creating a separate > host_byteorder.gni file, rather than setting it in BUILDCONFIG. We should avoid > changes to BUILDCONFIG if at all possible. That was kind of the whole reason why I didn't add is_aix. Also it was relatively straight forward to just get away with using - current_os = "aix". Regarding the need for adding "host_byteorder", see the ICU project cl mentioned in the description (https://codereview.chromium.org/2812173002/). The variable 'v8_host_byteorder' (removed in https://codereview.chromium.org/2809963004/) isn't available in ICU's context. An alternative would have been to figure out the host byte order in v8 and ICU separately (instead of making it available through BUILDCONFIG.gn). Which doesn't seem like the best approach, since we would just be duplicating code and more importantly we would want to avoid calling a script as much as possible (let alone call the same script twice). Also notice, this approach would only get worse if a newly added third party project required byte order support as well.
On 2017/04/18 20:55:38, Ray Banerjee wrote: > On 2017/04/18 20:31:31, Dirk Pranke wrote: > > Also, I take back what I said about adding is_aix et al. Let's not do that > until > > we have a sense of how broadly that might be needed. > > > > https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn > > File build/config/BUILDCONFIG.gn (right): > > > > > https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn... > > build/config/BUILDCONFIG.gn:79: host_byteorder = > > exec_script("//build/get_byteorder.py", [], "trim string") > > I've changed my mind. To repeat my latest comment from the v8-side patch: > > > > I think we should leave this here, at least until we have a need for > > host_byteorder somewhere outside of v8. > > > > If we have that need at that time, we should start by creating a separate > > host_byteorder.gni file, rather than setting it in BUILDCONFIG. We should > avoid > > changes to BUILDCONFIG if at all possible. > > That was kind of the whole reason why I didn't add is_aix. Also it was > relatively straight forward to just get away with using - current_os = "aix". Ack :). > > Regarding the need for adding "host_byteorder", see the ICU project cl mentioned > in the description (https://codereview.chromium.org/2812173002/). The variable > 'v8_host_byteorder' (removed in https://codereview.chromium.org/2809963004/) > isn't available in ICU's context. > > An alternative would have been to figure out the host byte order in v8 and ICU > separately (instead of making it available through BUILDCONFIG.gn). Which > doesn't seem like the best approach, since we would just be duplicating code and > more importantly we would want to avoid calling a script as much as possible > (let alone call the same script twice). Also notice, this approach would only > get worse if a newly added third party project required byte order support as > well. Ah, okay, I missed the ICU need. Okay, given that, I'd add the .gni file.
ptal. Also updated the other three related cls - https://codereview.chromium.org/2807463004/ https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ https://codereview.chromium.org/2815453004/diff/1/build/build_config.h File build/build_config.h (right): https://codereview.chromium.org/2815453004/diff/1/build/build_config.h#newcode83 build/build_config.h:83: defined(OS_ANDROID) || defined(OS_NACL) || defined(OS_QNX) || \ On 2017/04/14 01:29:02, Dirk Pranke wrote: > It seems like some of these checks are duplicated. Can you fix that? Done. https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn... build/config/BUILDCONFIG.gn:79: host_byteorder = exec_script("//build/get_byteorder.py", [], "trim string") On 2017/04/18 20:31:31, Dirk Pranke wrote: > I've changed my mind. To repeat my latest comment from the v8-side patch: > > I think we should leave this here, at least until we have a need for > host_byteorder somewhere outside of v8. > > If we have that need at that time, we should start by creating a separate > host_byteorder.gni file, rather than setting it in BUILDCONFIG. We should avoid > changes to BUILDCONFIG if at all possible. using host_byteorder.gni now. https://codereview.chromium.org/2815453004/diff/1/build/config/BUILDCONFIG.gn... build/config/BUILDCONFIG.gn:158: current_os == "chromeos" On 2017/04/18 13:46:03, Michael Achenbach wrote: > nit: Maybe move the chromeos clause before the linux clause for readability (to > have the most complicated clause last)? Not sure what formatting does to it > though... Done. https://codereview.chromium.org/2815453004/diff/1/build/config/aix/BUILD.gn File build/config/aix/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/aix/BUILD.gn#n... build/config/aix/BUILD.gn:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2017/04/18 13:46:03, Michael Achenbach wrote: > nit: 2017 Done. https://codereview.chromium.org/2815453004/diff/1/build/config/aix/BUILD.gn#n... build/config/aix/BUILD.gn:7: import("//build/toolchain/toolchain.gni") On 2017/04/18 13:46:03, Michael Achenbach wrote: > Not all these imports look necessary. Done. https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:183: cflags_cc += [ On 2017/04/18 13:46:04, Michael Achenbach wrote: > This doesn't include -fno-strict-aliasing in cflags anymore. Also for the > non-aix/non-ppc/etc cases. If this is intended, I'd prefer to have it in a > separate CL and with separate description. Acknowledged. https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1151: current_cpu != "ppc64" && current_cpu != "ppc")) { On 2017/04/18 13:46:04, Michael Achenbach wrote: > nit: For readability, maybe we could add a helper variable somewhere for: > current_cpu != "s390x" && current_cpu != "s390" && current_cpu != "ppc64" && > current_cpu != "ppc" > > Or rather the inverse case, used in line 181. This or the inverse case are used > many times in multiple files. The four parts of the clause are always going > together and are hard to read if nested within other clauses. Done. https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1385: if (current_os != "aix") { On 2017/04/18 13:46:03, Michael Achenbach wrote: > This is already nested in a current_os != "aix" scope. Done. https://codereview.chromium.org/2815453004/diff/1/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2815453004/diff/1/build/config/features.gni#n... build/config/features.gni:30: (current_cpu != "s390" && current_cpu != "s390x" && On 2017/04/18 13:46:04, Michael Achenbach wrote: > nit: Why parentheses? Done. https://codereview.chromium.org/2815453004/diff/1/build/config/features.gni#n... build/config/features.gni:33: target_cpu != "ppc64") On 2017/04/14 01:29:02, Dirk Pranke wrote: > Are you planning to port NaCl to these architectures? I'm guessing not, and > maybe we can just simplify this logic to only look at the target_cpus? Not that I know of. Also simplified the logic a little. https://codereview.chromium.org/2815453004/diff/1/build/toolchain/aix/BUILD.gn File build/toolchain/aix/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/toolchain/aix/BUILD.g... build/toolchain/aix/BUILD.gn:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2017/04/18 13:46:04, Michael Achenbach wrote: > nit: 2017 Done. https://codereview.chromium.org/2815453004/diff/1/build/toolchain/aix/BUILD.g... build/toolchain/aix/BUILD.gn:5: import("//build/config/sysroot.gni") On 2017/04/18 13:46:04, Michael Achenbach wrote: > Why is sysroot included here? That was a mistake. https://codereview.chromium.org/2815453004/diff/1/build/toolchain/gcc_toolcha... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2815453004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:320: ar_wrapper = rebase_path("//build/toolchain/gcc_ar_wrapper_aix.py", On 2017/04/18 13:46:04, Michael Achenbach wrote: > Maybe pull out the two differences (ar_wrapper, operation) into variables and > then have the command-variable line only once? Done. Also using the same gcc_ar_wrapper.py instead of having a separate one for Aix. https://codereview.chromium.org/2815453004/diff/1/build/toolchain/gcc_toolcha... build/toolchain/gcc_toolchain.gni:506: link_command = "$ld {{ldflags}}${extra_ldflags} -o \"$unstripped_outfile\" -Wl,--start-group @\"$rspfile\" {{solibs}} -Wl,--end-group $libs_section_prefix {{libs}} $libs_section_postfix" On 2017/04/18 13:46:04, Michael Achenbach wrote: > nit: Readability: Similar to my comment above, maybe factor out two variables > and have the line for link_command only once? Similar to how the whitelist_flag > variable is used above. Done.
lgtm. brettw, are you okay with the BUILDCONFIG changes? Seems like a reasonable compromise. https://codereview.chromium.org/2815453004/diff/20001/build/config/ui.gni File build/config/ui.gni (right): https://codereview.chromium.org/2815453004/diff/20001/build/config/ui.gni#new... build/config/ui.gni:38: current_cpu != "ppc64" && current_cpu != "ppc") change to `is_linux && !is_power_or_z`, right?
lgtm with comments future-nit: please always address reviews and do rebase in separate patchsets... eases re-review. https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:180: if (current_os != "aix" && Just double-checking: You don't need the aix check anymore? https://codereview.chromium.org/2815453004/diff/20001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2815453004/diff/20001/build/config/features.g... build/config/features.gni:30: !(is_linux && target_cpu == "arm64") && !is_power_or_z Double-checking: Here's a subtle difference now to the last patchset. Before, you enumerated also the target_cpu cases while is_power_or_z contains only current_cpu. IIUC, this would now enable nacl for the host-build (if supported) when cross-compiling. https://codereview.chromium.org/2815453004/diff/20001/build/toolchain/gcc_too... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2815453004/diff/20001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:500: start_group_flag = " " nit: Why not empty string?
ptal https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:180: if (current_os != "aix" && On 2017/04/25 14:08:30, Michael Achenbach wrote: > Just double-checking: You don't need the aix check anymore? We only support AIX on ppc64 BE. So further checking current_os != aix, seemed redundant. Also I don't know the behavior for AIX running on other platforms, but I do know AIX on ppc64 does not have support for these flags. https://codereview.chromium.org/2815453004/diff/20001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2815453004/diff/20001/build/config/features.g... build/config/features.gni:30: !(is_linux && target_cpu == "arm64") && !is_power_or_z On 2017/04/25 14:08:30, Michael Achenbach wrote: > Double-checking: Here's a subtle difference now to the last patchset. Before, > you enumerated also the target_cpu cases while is_power_or_z contains only > current_cpu. IIUC, this would now enable nacl for the host-build (if supported) > when cross-compiling. Form my understanding it's not supported on the host builds when cross-compiling. But also further changes need to be made to support cross-compilation using GN on these platforms. So maybe we want that clause to be added in a separate cl (the one which would contain changes for supporting cross-compilation). https://codereview.chromium.org/2815453004/diff/20001/build/config/ui.gni File build/config/ui.gni (right): https://codereview.chromium.org/2815453004/diff/20001/build/config/ui.gni#new... build/config/ui.gni:38: current_cpu != "ppc64" && current_cpu != "ppc") On 2017/04/25 01:42:48, Dirk Pranke wrote: > change to `is_linux && !is_power_or_z`, right? Done. https://codereview.chromium.org/2815453004/diff/20001/build/toolchain/gcc_too... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2815453004/diff/20001/build/toolchain/gcc_too... build/toolchain/gcc_toolchain.gni:500: start_group_flag = " " On 2017/04/25 14:08:30, Michael Achenbach wrote: > nit: Why not empty string? That was a mistake. Fixed it.
thakis@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/2815453004/diff/40001/build/config/host_byteo... File build/config/host_byteorder.gni (right): https://codereview.chromium.org/2815453004/diff/40001/build/config/host_byteo... build/config/host_byteorder.gni:5: # This header file defines the "host_byteorder" variable. Please very clearly state in this file that this is only for v8 and that chromium code generally assumes little-endianness.
lgtm - ack
https://codereview.chromium.org/2815453004/diff/40001/build/config/host_byteo... File build/config/host_byteorder.gni (right): https://codereview.chromium.org/2815453004/diff/40001/build/config/host_byteo... build/config/host_byteorder.gni:5: # This header file defines the "host_byteorder" variable. On 2017/04/25 22:42:31, Nico wrote: > Please very clearly state in this file that this is only for v8 and that > chromium code generally assumes little-endianness. Done.
The CQ bit was checked by rayb@ca.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by rayb@ca.ibm.com
The CQ bit was checked by rayb@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2815453004/#ps60001 (title: "clarified the need for host_byteorder.gni")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
It seems I still need an lgtm from brettw(for BUILDCONFIG.gn) to get this through.
https://codereview.chromium.org/2815453004/diff/60001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:75: # Creating a helper variable vastly improves readability. I think this line: "Creating a helper variable vastly improves readability." can just be deleted, I think it's implicit in the above. https://codereview.chromium.org/2815453004/diff/60001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:76: is_power_or_z = current_cpu == "s390x" || current_cpu == "s390" || If we're going to keep this here, it should be called something like "current_cpu_is_power_or_z" so it's clear that this applies to the current CPU. https://codereview.chromium.org/2815453004/diff/60001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/compiler/B... build/config/compiler/BUILD.gn:183: if (!is_power_or_z) { Why does this condition exist? https://codereview.chromium.org/2815453004/diff/60001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/features.g... build/config/features.gni:25: # Temporarily disable nacl on arm64 linux, ppc64 and s390x I thought the changes you're doing is to support V8 and Node stuff. How are you hitting NaCl stuff? Maybe I don't understand your overall goals for what you're doing with the build. https://codereview.chromium.org/2815453004/diff/60001/build/config/ui.gni File build/config/ui.gni (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/ui.gni#new... build/config/ui.gni:37: use_glib = is_linux && !is_power_or_z As with the is_nacl flag, can you explain why this value affects your V8 compiles? For this one, why does the CPU architecture affect whether glib is used? I would expect that to be a function of the OS.
https://codereview.chromium.org/2815453004/diff/60001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/compiler/B... build/config/compiler/BUILD.gn:183: if (!is_power_or_z) { On 2017/04/26 20:07:42, brettw (plz ping after 24h) wrote: > Why does this condition exist? I believe these flags don't work on the IBM versions of gcc. Perhaps add some comment to that effect? https://codereview.chromium.org/2815453004/diff/60001/build/config/ui.gni File build/config/ui.gni (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/ui.gni#new... build/config/ui.gni:37: use_glib = is_linux && !is_power_or_z On 2017/04/26 20:07:42, brettw (plz ping after 24h) wrote: > As with the is_nacl flag, can you explain why this value affects your V8 > compiles? > > For this one, why does the CPU architecture affect whether glib is used? I would > expect that to be a function of the OS. I think this is because //base has dependencies on glib, and I'd guess that maybe glib isn't available on their ports?
The CQ bit was checked by rayb@ca.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by rayb@ca.ibm.com to run a CQ dry run
ptal https://codereview.chromium.org/2815453004/diff/60001/build/config/BUILDCONFI... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:75: # Creating a helper variable vastly improves readability. On 2017/04/26 20:07:42, brettw (plz ping after 24h) wrote: > I think this line: > "Creating a helper variable vastly improves readability." > can just be deleted, I think it's implicit in the above. Done. https://codereview.chromium.org/2815453004/diff/60001/build/config/BUILDCONFI... build/config/BUILDCONFIG.gn:76: is_power_or_z = current_cpu == "s390x" || current_cpu == "s390" || On 2017/04/26 20:07:42, brettw (plz ping after 24h) wrote: > If we're going to keep this here, it should be called something like > "current_cpu_is_power_or_z" so it's clear that this applies to the current CPU. Done. https://codereview.chromium.org/2815453004/diff/60001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/compiler/B... build/config/compiler/BUILD.gn:183: if (!is_power_or_z) { On 2017/04/26 23:32:13, Dirk Pranke wrote: > On 2017/04/26 20:07:42, brettw (plz ping after 24h) wrote: > > Why does this condition exist? > > I believe these flags don't work on the IBM versions of gcc. Perhaps add some > comment to that effect? So for the -fno-strict-aliasing one, I wasn't sure if we needed it as the justification provided in the gyp files for v8 is - "# Some versions of GCC 4.5 seem to need -fno-strict-aliasing.". Also the linked Issue dates back to 2010 (and mostly talks about chromium-code), so I wasn't sure if this was still a thing. But by your comment, I am assuming it is still relevant, so I am adding it back now. (Initially I was aiming for the smallest subset of flags to get everything built properly on the mentioned platforms, if that makes sense. And there doesn't seem to be any errors caused by the exclusion of the flag, at least on the platforms I have been testing on.) As for the "-fvisibility-inlines-hidden" flag, I can confirm that gcc on AIX does not have support for the visibility attribute. It was a mistake from my part to exclude that one from the p/Linux and z/Linux builds though. Thanks for catching that. Fixed now. https://codereview.chromium.org/2815453004/diff/60001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/features.g... build/config/features.gni:25: # Temporarily disable nacl on arm64 linux, ppc64 and s390x On 2017/04/26 20:07:42, brettw (plz ping after 24h) wrote: > I thought the changes you're doing is to support V8 and Node stuff. How are you > hitting NaCl stuff? Maybe I don't understand your overall goals for what you're > doing with the build. Hmm, I see what you mean. Since IIUC, I shouldn't even be checking whether to disable or enable nacl for building v8. But this (features.gni) gets imported regardless of what I'm building via 'build/config/BUILD.gn'. And if I don't disable this here, then this tries to define a nacl_toolchain inside 'build/config/nacl/config.gni' and of course there is no such nacl_toolchain for the aforementioned platforms (and pretty sure it doesn't make sense for us to have one as well). That's why I'm disabling it so that the whole logic reagarding nacl never kicks in. Even though, 'build/config/nacl/config.gni' again does get imported regardless of what I'm building through 'build/config/compiler/BUILD.gn'. Notice that none of the mentioned '*.gni' imports are guarded by anything. Also yes, all the changes here are to build v8 using GN. Currently on the mentioned platforms, gyp is used to build v8. I have already ported GN (on the mentioned platforms) and this (along with the rest of the linked cls) are so that we can build v8 using that GN. And of course GN relies on the already existing infrastructure in place inside //build to create v8. Sorry for the rather long reply. Please let me know if something is still unclear or in case I am misunderstanding something. https://codereview.chromium.org/2815453004/diff/60001/build/config/ui.gni File build/config/ui.gni (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/ui.gni#new... build/config/ui.gni:37: use_glib = is_linux && !is_power_or_z On 2017/04/26 23:32:13, Dirk Pranke wrote: > On 2017/04/26 20:07:42, brettw (plz ping after 24h) wrote: > > As with the is_nacl flag, can you explain why this value affects your V8 > > compiles? > > > > For this one, why does the CPU architecture affect whether glib is used? I > would > > expect that to be a function of the OS. > > I think this is because //base has dependencies on glib, and I'd guess that > maybe glib isn't available on their ports? //base does have dependencies on glib. My initial assumption was also that glib wasn't available on all of our ports. As it certainly wasn't available on the machines I was testing on. But upon closer inspection, it might just have been an environment issue, as since then I have managed to get hold of a pLinux 64 BE machine with the required libraries. So that makes me almost certain that the required ones(glib-2.0 gmodule-2.0 gobject-2.0 gthread-2.0) do indeed exist on both power and z linux. They are just not that common, or at least they are uncommon enough for me to not have them on the machines I was running on. Anyways, I will turn them back on for p/Linux and z/Linux. Again, thanks for catching that.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rayb@ca.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. Re-based after addressing reviews.
I'm trying really hard to keep the BUILDCONFIG file simple since these variables are implicitly global in every build file. As with C++, globals are bad :) If we can remove the nacl one, then we're down to 4 uses of this variable. To me, four uses doesn't meet the bar and I would prefer to just inline your 4 CPU names in the places where it's needed. My perception is that these CPUs don't really get added on to and we don't expect more uses, so this shouldn't be a maintenance problem. https://codereview.chromium.org/2815453004/diff/60001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/2815453004/diff/60001/build/config/features.g... build/config/features.gni:25: # Temporarily disable nacl on arm64 linux, ppc64 and s390x I don't quite follow. build/config/compiler/BUILD.gn references build/config;nacl/config.gni but that .gni file doesn't reference any targets or actually instantiate any toolchains. Just having the nacl_toolchain variable defined shouldn't affect you at all. I agree defining this is theoretically yucky, but shouldn't actually be blocking you. For cleanliness I did https://codereview.chromium.org/2845943005/ which should only do this import when compiling with the nacl toolchain. But from what I'm understanding, not changing this file should still make the build work OK. If it doesn't can you explain precisely what's broken? We should fix that regardless.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/04/27 20:45:52, brettw (plz ping after 24h) wrote: > I'm trying really hard to keep the BUILDCONFIG file simple since these variables > are implicitly global in every build file. As with C++, globals are bad :) > > If we can remove the nacl one, then we're down to 4 uses of this variable. To > me, four uses doesn't meet the bar and I would prefer to just inline your 4 CPU > names in the places where it's needed. My perception is that these CPUs don't > really get added on to and we don't expect more uses, so this shouldn't be a > maintenance problem. Thanks for explaining your though process. And I do agree with the fact that global variables are bad :) But can we come up with solution that doesn't involve having these four clauses be inline every time? For example, I could declare a helper variable separately in each of these 4(potentially 5 if you count nacl) files. But of course that would mean duplicating some code. Another idea would be to put this in a .gni file, but not sure if this one variable warrants it's own .gni file. However IMO, the trade offs for both these alternatives are still worth the added ease in readability. I mean these were inline to begin with and it made even trivial clauses hard to read. Let me know what you think. > https://codereview.chromium.org/2815453004/diff/60001/build/config/features.gni > File build/config/features.gni (right): > > https://codereview.chromium.org/2815453004/diff/60001/build/config/features.g... > build/config/features.gni:25: # Temporarily disable nacl on arm64 linux, ppc64 > and s390x > I don't quite follow. > > build/config/compiler/BUILD.gn references build/config;nacl/config.gni but that > .gni file doesn't reference any targets or actually instantiate any toolchains. > Just having the nacl_toolchain variable defined shouldn't affect you at all. > > I agree defining this is theoretically yucky, but shouldn't actually be blocking > you. For cleanliness I did https://codereview.chromium.org/2845943005/ which > should only do this import when compiling with the nacl toolchain. > > But from what I'm understanding, not changing this file should still make the > build work OK. If it doesn't can you explain precisely what's broken? We should > fix that regardless. Well with enable_nacl set to true, '//build/config/nacl/config.gni' tries to define things like 'nacl_toolchain_tooldir' (line 44) where these definitions require 'nacl_tuple' to be defined. And 'nacl_tuple' is set according to the cpu architecture. So we could potentially set 'nacl_tuple' to something like 'ppc64-nacl' as a dummy value and that does work. But that fix seems even "yukier", don't you agree? I mean it doesn't make sense for me to even go inside that branch of code. Do let me know if I'm missing something.
On 2017/04/27 22:10:47, rayb wrote: > On 2017/04/27 20:45:52, brettw (plz ping after 24h) wrote: > > I'm trying really hard to keep the BUILDCONFIG file simple since these > variables > > are implicitly global in every build file. As with C++, globals are bad :) > > > > If we can remove the nacl one, then we're down to 4 uses of this variable. To > > me, four uses doesn't meet the bar and I would prefer to just inline your 4 > CPU > > names in the places where it's needed. My perception is that these CPUs don't > > really get added on to and we don't expect more uses, so this shouldn't be a > > maintenance problem. > > Thanks for explaining your though process. And I do agree with the fact that > global variables are bad :) > > But can we come up with solution that doesn't involve having these four clauses > be inline every time? For example, I could declare a helper variable separately > in each of these 4(potentially 5 if you count nacl) files. But of course that > would mean duplicating some code. Another idea would be to put this in a .gni > file, but not sure if this one variable warrants it's own .gni file. However > IMO, the trade offs for both these alternatives are still worth the added ease > in readability. I mean these were inline to begin with and it made even trivial > clauses hard to read. Let me know what you think. Let me talk to Dirk, he seems to be out today. > https://codereview.chromium.org/2815453004/diff/60001/build/config/features.gni > > File build/config/features.gni (right): > > > > > https://codereview.chromium.org/2815453004/diff/60001/build/config/features.g... > > build/config/features.gni:25: # Temporarily disable nacl on arm64 linux, ppc64 > > and s390x > > I don't quite follow. > > > > build/config/compiler/BUILD.gn references build/config;nacl/config.gni but > that > > .gni file doesn't reference any targets or actually instantiate any > toolchains. > > Just having the nacl_toolchain variable defined shouldn't affect you at all. > > > > I agree defining this is theoretically yucky, but shouldn't actually be > blocking > > you. For cleanliness I did https://codereview.chromium.org/2845943005/ which > > should only do this import when compiling with the nacl toolchain. > > > > But from what I'm understanding, not changing this file should still make the > > build work OK. If it doesn't can you explain precisely what's broken? We > should > > fix that regardless. > > Well with enable_nacl set to true, '//build/config/nacl/config.gni' tries to > define things like 'nacl_toolchain_tooldir' (line 44) where these definitions > require 'nacl_tuple' to be defined. And 'nacl_tuple' is set according to the cpu > architecture. So we could potentially set 'nacl_tuple' to something like > 'ppc64-nacl' as a dummy value and that does work. > > But that fix seems even "yukier", don't you agree? I mean it doesn't make sense > for me to even go inside that branch of code. Do let me know if I'm missing > something. Ah I see. Then I think the patch I linked above is the correct solution to this problem. I don't think //build/config/* should have any dependency on NaCl other than the is_nacl global for cases exactly like yours. Feel free to review that one since Dirk seems to be out.
On 2017/04/27 23:13:51, brettw (plz ping after 24h) wrote: > On 2017/04/27 22:10:47, rayb wrote: > > On 2017/04/27 20:45:52, brettw (plz ping after 24h) wrote: > > > I'm trying really hard to keep the BUILDCONFIG file simple since these > > variables > > > are implicitly global in every build file. As with C++, globals are bad :) > > > > > > If we can remove the nacl one, then we're down to 4 uses of this variable. > To > > > me, four uses doesn't meet the bar and I would prefer to just inline your 4 > > CPU > > > names in the places where it's needed. My perception is that these CPUs > don't > > > really get added on to and we don't expect more uses, so this shouldn't be a > > > maintenance problem. > > > > Thanks for explaining your though process. And I do agree with the fact that > > global variables are bad :) > > > > But can we come up with solution that doesn't involve having these four > clauses > > be inline every time? For example, I could declare a helper variable > separately > > in each of these 4(potentially 5 if you count nacl) files. But of course that > > would mean duplicating some code. Another idea would be to put this in a .gni > > file, but not sure if this one variable warrants it's own .gni file. However > > IMO, the trade offs for both these alternatives are still worth the added ease > > in readability. I mean these were inline to begin with and it made even > trivial > > clauses hard to read. Let me know what you think. > > Let me talk to Dirk, he seems to be out today. Ack. > > > > > https://codereview.chromium.org/2815453004/diff/60001/build/config/features.gni > > > File build/config/features.gni (right): > > > > > > > > > https://codereview.chromium.org/2815453004/diff/60001/build/config/features.g... > > > build/config/features.gni:25: # Temporarily disable nacl on arm64 linux, > ppc64 > > > and s390x > > > I don't quite follow. > > > > > > build/config/compiler/BUILD.gn references build/config;nacl/config.gni but > > that > > > .gni file doesn't reference any targets or actually instantiate any > > toolchains. > > > Just having the nacl_toolchain variable defined shouldn't affect you at all. > > > > > > I agree defining this is theoretically yucky, but shouldn't actually be > > blocking > > > you. For cleanliness I did https://codereview.chromium.org/2845943005/ which > > > should only do this import when compiling with the nacl toolchain. > > > > > > But from what I'm understanding, not changing this file should still make > the > > > build work OK. If it doesn't can you explain precisely what's broken? We > > should > > > fix that regardless. > > > > Well with enable_nacl set to true, '//build/config/nacl/config.gni' tries to > > define things like 'nacl_toolchain_tooldir' (line 44) where these definitions > > require 'nacl_tuple' to be defined. And 'nacl_tuple' is set according to the > cpu > > architecture. So we could potentially set 'nacl_tuple' to something like > > 'ppc64-nacl' as a dummy value and that does work. > > > > But that fix seems even "yukier", don't you agree? I mean it doesn't make > sense > > for me to even go inside that branch of code. Do let me know if I'm missing > > something. > > Ah I see. Then I think the patch I linked above is the correct solution to this > problem. I don't think //build/config/* should have any dependency on NaCl other > than the is_nacl global for cases exactly like yours. Feel free to review that > one since Dirk seems to be out. Agreed.
The alternative is to make a flat in e.g. compiler.gni. That can't be used in BUILDCONFIG.gn, so the CPU check would need to be inlined in the is_clang computation. But if we remove that one and the is_nacl one, I think we only have two uses of this variable: one in compiler/BUILD.gn, and one in sysroot.gni. I really think it's better for this narrow case to just check the CPU names rather than having yet another variable in the already complicated build directory.
On 2017/04/28 17:11:53, brettw (busy this week) wrote: > The alternative is to make a flat in e.g. compiler.gni. That can't be used in > BUILDCONFIG.gn, so the CPU check would need to be inlined in the is_clang > computation. > > But if we remove that one and the is_nacl one, I think we only have two uses of > this variable: one in compiler/BUILD.gn, and one in sysroot.gni. I really think > it's better for this narrow case to just check the CPU names rather than having > yet another variable in the already complicated build directory. Okay then, I will make them inline. Also the patch referenced earlier - https://codereview.chromium.org/2845943005/, doesn't completely fix our problem with the enable_nacl variable (I did leave a comment on that issue as well). Even with that fix, '//build/toolchain/gcc_toolchain.gni' still imports '/nacl/config.gni' regardless of what is_nacl is set to (similar to the problem you fixed in that patch). And we eventually hit the same code where it expects 'nacl_tuple' to be defined, but of course it's not and then it fails. As we discussed earlier, the reasonable fix should probably be to do what you did for 'build/config/compiler/BUILD.gn'. I never got a reply on that comment, so I was just curious whether I'm missing something here or it just went unnoticed. Please do let me know and if required I can just make that change in a separate cl.
On 2017/04/29 01:14:02, rayb wrote: > On 2017/04/28 17:11:53, brettw (busy this week) wrote: > > The alternative is to make a flat in e.g. compiler.gni. That can't be used in > > BUILDCONFIG.gn, so the CPU check would need to be inlined in the is_clang > > computation. > > > > But if we remove that one and the is_nacl one, I think we only have two uses > of > > this variable: one in compiler/BUILD.gn, and one in sysroot.gni. I really > think > > it's better for this narrow case to just check the CPU names rather than > having > > yet another variable in the already complicated build directory. > > Okay then, I will make them inline. > > Also the patch referenced earlier - > https://codereview.chromium.org/2845943005/, doesn't completely fix our problem > with the enable_nacl variable (I did leave a comment on that issue as well). > Even with that fix, '//build/toolchain/gcc_toolchain.gni' still imports > '/nacl/config.gni' regardless of what is_nacl is set to (similar to the problem > you fixed in that patch). And we eventually hit the same code where it expects > 'nacl_tuple' to be defined, but of course it's not and then it fails. > > As we discussed earlier, the reasonable fix should probably be to do what you > did for 'build/config/compiler/BUILD.gn'. I never got a reply on that comment, > so I was just curious whether I'm missing something here or it just went > unnoticed. Please do let me know and if required I can just make that change in > a separate cl. Sorry, I just didn't notice your comment. Please make it work and send a patch.
I've lost track of this CL; are we waiting for changes in this CL, or in another CL, or both?
On 2017/05/04 00:34:48, Dirk Pranke wrote: > I've lost track of this CL; are we waiting for changes in this CL, or in another > CL, or both? Sorry, I was away for the week. As stated earlier, I think the change for separating the nacl_toolchain (the one in 'build/config/compiler/BUILD.gn') should go in a separate cl. Does that make sense? And after that goes through, we can just finish off the remaining 'building v8 with GN' cls. Also the cl for building the actual GN binary for the aforementioned platforms has been committed. Again, sorry for the delay.
ptal. On 2017/04/30 06:40:03, brettw (busy this week) wrote: > On 2017/04/29 01:14:02, rayb wrote: > > On 2017/04/28 17:11:53, brettw (busy this week) wrote: > > > The alternative is to make a flat in e.g. compiler.gni. That can't be used > in > > > BUILDCONFIG.gn, so the CPU check would need to be inlined in the is_clang > > > computation. > > > > > > But if we remove that one and the is_nacl one, I think we only have two uses > > of > > > this variable: one in compiler/BUILD.gn, and one in sysroot.gni. I really > > think > > > it's better for this narrow case to just check the CPU names rather than > > having > > > yet another variable in the already complicated build directory. > > > > Okay then, I will make them inline. Done. > > > > Also the patch referenced earlier - > > https://codereview.chromium.org/2845943005/, doesn't completely fix our > problem > > with the enable_nacl variable (I did leave a comment on that issue as well). > > Even with that fix, '//build/toolchain/gcc_toolchain.gni' still imports > > '/nacl/config.gni' regardless of what is_nacl is set to (similar to the > problem > > you fixed in that patch). And we eventually hit the same code where it expects > > 'nacl_tuple' to be defined, but of course it's not and then it fails. > > > > As we discussed earlier, the reasonable fix should probably be to do what you > > did for 'build/config/compiler/BUILD.gn'. I never got a reply on that comment, > > so I was just curious whether I'm missing something here or it just went > > unnoticed. Please do let me know and if required I can just make that change > in > > a separate cl. > > Sorry, I just didn't notice your comment. Please make it work and send a patch. This is now done in https://codereview.chromium.org/2870553002/ .
ptal.
lgtm. @brettw, does this lgty?
The CQ bit was checked by rayb@ca.ibm.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm
The CQ bit was checked by rayb@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2815453004/#ps180001 (title: "rebased")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by rayb@ca.ibm.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494390224816110,
"parent_rev": "e69195aa6646b2f6e481e5f1e0fbc82cc807d58a", "commit_rev":
"367a04209fcb6c3700daee65511945da7dd31f20"}
Message was sent while issue was closed.
Description was changed from ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64(both LE and BE). Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively - https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ R=machenbach@chromium.org, dpranke@chromium.org, adamk@chromium.org BUG=706728 ========== to ========== For building v8 using gn on aix_ppc64, linux_s390x and linux_ppc64(both LE and BE). Also add support for host_byteorder logic which is used in the following v8 and icu issues respectively - https://codereview.chromium.org/2809963004/ https://codereview.chromium.org/2812173002/ R=machenbach@chromium.org, dpranke@chromium.org, adamk@chromium.org BUG=706728 Review-Url: https://codereview.chromium.org/2815453004 Cr-Commit-Position: refs/heads/master@{#470463} Committed: https://chromium.googlesource.com/chromium/src/+/367a04209fcb6c3700daee655119... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/367a04209fcb6c3700daee655119... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
