Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2219953002:
Use new toolchain_args variable in GN (Closed)
|
Created:
4 years, 4 months ago by brettw Modified:
4 years, 4 months ago Reviewers:
Dirk Pranke CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@toolchain_args Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse new toolchain_args variable in GN.
toolchain_args is changing from a function call to a scope that can be passed around. Pipe these args through the various toolchain templates. This adds complexity in some cases, but it eliminates the need for the toolchain templates to know about every build flag they might ever be called with.
BUG=634446
Committed: https://crrev.com/3c38c5cdab9e6fc35852ae79be5351be2bd6d49e
Cr-Commit-Position: refs/heads/master@{#410853}
Patch Set 1 #Patch Set 2 : Format #Patch Set 3 : Merge remote-tracking branch 'origin/master' into args_impl #Patch Set 4 : . #Patch Set 5 : Fixes #Patch Set 6 : fix_win #Patch Set 7 : fix_and_merge #Patch Set 8 : v8 #
Total comments: 10
Patch Set 9 : PTAL #Patch Set 10 : Merge remote-tracking branch 'origin/master' into args_impl #
Total comments: 2
Patch Set 11 : wrap #
Depends on Patchset: Messages
Total messages: 62 (43 generated)
Description was changed from ========== Use new toolchain_args variable in GN BUG= ========== to ========== Use new toolchain_args variable in GN BUG=634446 ==========
.
The CQ bit was checked by brettw@chromium.org 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...
Description was changed from ========== Use new toolchain_args variable in GN BUG=634446 ========== to ========== Use new toolchain_args variable in GN. toolchain_args is changing from a function call to a scope that can be passed around. Pipe these args through the various toolchain templates. This adds complexity in some cases, but it eliminates the need for the toolchain templates to know about every build flag they might ever be called with. BUG=634446 ==========
brettw@chromium.org changed reviewers: + dpranke@chromium.org
This was much harder than I expected!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Fixes
The CQ bit was checked by brettw@chromium.org 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_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
fix_win
The CQ bit was checked by brettw@chromium.org 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
fix_and_merge
The CQ bit was checked by brettw@chromium.org 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
v8
The CQ bit was checked by brettw@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by brettw@chromium.org 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: This issue passed the CQ dry run.
I'm not quite sure I understand how everything hangs together yet, and I'd like to ... https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:113: target_cpu = target_cpu host_toolchain is derived from host_os, host_cpu, and is_clang. The first two are constants. target_os and target_cpu are also supposed to be constants. You're not actually forwarding/resetting is_clang, which makes sense because that is usually a toolchain arg. So, I'm not sure what you're protecting against? https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl/B... File build/toolchain/nacl/BUILD.gn (right): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl/B... build/toolchain/nacl/BUILD.gn:143: current_cpu = toolchain_cpu where does toolchain_cpu come from? https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl_t... File build/toolchain/nacl_toolchain.gni (left): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl_t... build/toolchain/nacl_toolchain.gni:17: # build using this toolchain.) what, if anything, enforces that current_cpu was set?
PTAL
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:113: target_cpu = target_cpu On 2016/08/08 23:28:39, Dirk Pranke wrote: > host_toolchain is derived from host_os, host_cpu, and is_clang. The first two > are constants. target_os and target_cpu are also supposed to be constants. > You're not actually forwarding/resetting is_clang, which makes sense because > that is usually a toolchain arg. So, I'm not sure what you're protecting > against? This logic isn't changing from the old build, this patch is trying not to change anything. I agree target_os and target_cpu can probably be removed here. I'll make a note to try that in a follow up. My perception is that this patch is very risky so would like to have as small an effect as possible. host_toolchain is necessary to pass here because is_clang will get modified by the various toolchains. Those toolchains' values will be propagatedfrom the invoker's toolchain args (the whole point of this change is that toolchains can set arguments without having to list them all in this file). https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl/B... File build/toolchain/nacl/BUILD.gn (right): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl/B... build/toolchain/nacl/BUILD.gn:143: current_cpu = toolchain_cpu On 2016/08/08 23:28:39, Dirk Pranke wrote: > where does toolchain_cpu come from? The top of the template definition. Before, this was threaded through to the gcc_toolchain template which used it to set the current_cpu in the toolchain_args() {} block. https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl_t... File build/toolchain/nacl_toolchain.gni (left): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl_t... build/toolchain/nacl_toolchain.gni:17: # build using this toolchain.) On 2016/08/08 23:28:39, Dirk Pranke wrote: > what, if anything, enforces that current_cpu was set? I just added an assert to the gcc_toolchain template.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:113: target_cpu = target_cpu If you're curious why this forwarding was added in the first place, you added it here: https://codereview.chromium.org/936103003/diff/80001/build/toolchain/gcc_tool...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 brettw@chromium.org 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...
https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:113: target_cpu = target_cpu Dependent patch to remove these: https://codereview.chromium.org/2229063002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by brettw@chromium.org 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...
We might want to fix the android x64 thing first in a different CL to make sure that we understand what's going on there. Otherwise, lgtm. https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... File build/toolchain/gcc_toolchain.gni (right): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/gcc_to... build/toolchain/gcc_toolchain.gni:113: target_cpu = target_cpu Right, forwarding host_toolchain still makes sense. Thx. https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl/B... File build/toolchain/nacl/BUILD.gn (right): https://codereview.chromium.org/2219953002/diff/140001/build/toolchain/nacl/B... build/toolchain/nacl/BUILD.gn:143: current_cpu = toolchain_cpu On 2016/08/09 17:26:29, brettw (ping on IM after 24h) wrote: > On 2016/08/08 23:28:39, Dirk Pranke wrote: > > where does toolchain_cpu come from? > > The top of the template definition. Before, this was threaded through to the > gcc_toolchain template which used it to set the current_cpu in the > toolchain_args() {} block. Ah, right, line 111. I missed that in my first review (and had forgotten how these templates worked). https://codereview.chromium.org/2219953002/diff/180001/build/toolchain/androi... File build/toolchain/android/BUILD.gn (right): https://codereview.chromium.org/2219953002/diff/180001/build/toolchain/androi... build/toolchain/android/BUILD.gn:120: current_cpu = "x86_64" hm. this can't be right. "x86_64" isn't a legal value. I wonder if we don't actually have any x64 builders. https://codereview.chromium.org/2219953002/diff/180001/build/toolchain/win/BU... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/2219953002/diff/180001/build/toolchain/win/BU... build/toolchain/win/BUILD.gn:116: # %PATH%) -- e.g. 32-bit MSVS builds require %PATH% to be set and just passing nit: line wrapping?
The CQ bit was checked by brettw@chromium.org 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: This issue passed the CQ dry run.
The CQ bit was checked by brettw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2219953002/#ps200001 (title: "wrap")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use new toolchain_args variable in GN. toolchain_args is changing from a function call to a scope that can be passed around. Pipe these args through the various toolchain templates. This adds complexity in some cases, but it eliminates the need for the toolchain templates to know about every build flag they might ever be called with. BUG=634446 ========== to ========== Use new toolchain_args variable in GN. toolchain_args is changing from a function call to a scope that can be passed around. Pipe these args through the various toolchain templates. This adds complexity in some cases, but it eliminates the need for the toolchain templates to know about every build flag they might ever be called with. BUG=634446 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Use new toolchain_args variable in GN. toolchain_args is changing from a function call to a scope that can be passed around. Pipe these args through the various toolchain templates. This adds complexity in some cases, but it eliminates the need for the toolchain templates to know about every build flag they might ever be called with. BUG=634446 ========== to ========== Use new toolchain_args variable in GN. toolchain_args is changing from a function call to a scope that can be passed around. Pipe these args through the various toolchain templates. This adds complexity in some cases, but it eliminates the need for the toolchain templates to know about every build flag they might ever be called with. BUG=634446 Committed: https://crrev.com/3c38c5cdab9e6fc35852ae79be5351be2bd6d49e Cr-Commit-Position: refs/heads/master@{#410853} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/3c38c5cdab9e6fc35852ae79be5351be2bd6d49e Cr-Commit-Position: refs/heads/master@{#410853}
Message was sent while issue was closed.
On 2016/08/09 22:23:34, commit-bot: I haz the power wrote: > Patchset 11 (id:??) landed as > https://crrev.com/3c38c5cdab9e6fc35852ae79be5351be2bd6d49e > Cr-Commit-Position: refs/heads/master@{#410853} On Chrome OS I am getting: ERROR at //build/toolchain/cros/BUILD.gn:125:24: Build argument has no effect. v8_toolchain_cpu = v8_target_cpu ^------------ The variable "v8_toolchain_cpu" was set as a build argument but never appeared in a declare_args() block in any buildfile.
Message was sent while issue was closed.
On 2016/08/09 23:49:05, stevenjb wrote: > On 2016/08/09 22:23:34, commit-bot: I haz the power wrote: > > Patchset 11 (id:??) landed as > > https://crrev.com/3c38c5cdab9e6fc35852ae79be5351be2bd6d49e > > Cr-Commit-Position: refs/heads/master@{#410853} > > On Chrome OS I am getting: > > ERROR at //build/toolchain/cros/BUILD.gn:125:24: Build argument has no effect. > v8_toolchain_cpu = v8_target_cpu > ^------------ > The variable "v8_toolchain_cpu" was set as a build argument > but never appeared in a declare_args() block in any buildfile. Yup, this actually shows up in https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-g... (the tryjob on this CL) but this is not a fatal error, unfortunately. I think https://codereview.chromium.org/2231643002/ should fix this. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
