Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2317693002:
icecc support for cros host build
Descriptionicecc support for cros host build
GN supports icecc via |cc_wrapper| on all platforms except for cros. [1]
This CL makes |cc_wrapper| work on cros also [2]
icecc needs folloing additioinal GN flags
use_debug_fission=false linux_use_bundled_binutils=false clang_use_chrome_plugins=false cc_wrapper="icecc"
In addition, it addes an assertion "cc_wrapper is not supported on target build."
[1] https://crrev.com/e279af1c9c511972000049b2c4d261161b48075d
[2] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chromium-browser
BUG=645435
Patch Set 1 #
Total comments: 1
Patch Set 2 : apply only host #Patch Set 3 : add assertion #Messages
Total messages: 23 (17 generated)
The CQ bit was checked by dongseong.hwang@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...
dongseong.hwang@intel.com changed reviewers: + brettw@chromium.org
brettw@chromium.org: Please review the changes. It's kind of follow-up of https://codereview.chromium.org/1660053005
The CQ bit was checked by dongseong.hwang@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/2317693002/diff/1/build/toolchain/cros/BUILD.gn File build/toolchain/cros/BUILD.gn (right): https://codereview.chromium.org/2317693002/diff/1/build/toolchain/cros/BUILD.... build/toolchain/cros/BUILD.gn:19: cxx = compiler_prefix + cros_target_cxx exclude target build, because for some reason, iceccd doesn't dispatch target-build-jobs to clients /bin/sh -c icecc x86_64-cros-linux-gnu-g++ XXXXX
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/06 15:00:12, dshwang wrote: > https://codereview.chromium.org/2317693002/diff/1/build/toolchain/cros/BUILD.gn > File build/toolchain/cros/BUILD.gn (right): > > https://codereview.chromium.org/2317693002/diff/1/build/toolchain/cros/BUILD.... > build/toolchain/cros/BUILD.gn:19: cxx = compiler_prefix + cros_target_cxx > exclude target build, because for some reason, iceccd doesn't dispatch > target-build-jobs to clients > /bin/sh -c icecc x86_64-cros-linux-gnu-g++ XXXXX Is this a bug? It seems like doing distributed builds for only the host part of the compile is pretty useless (it's a tiny part of the build), and it's misleading to say this option is supported. I'm thinking we should add an assert about icecc usage working, and add an assertion that this flag isn't set on ChromeOS (referencing the bug) until the bug is fixed. Then we enable it everywhere.
On 2016/09/06 17:37:09, brettw (ping on IM after 24h) wrote: > On 2016/09/06 15:00:12, dshwang wrote: > > > https://codereview.chromium.org/2317693002/diff/1/build/toolchain/cros/BUILD.gn > > File build/toolchain/cros/BUILD.gn (right): > > > > > https://codereview.chromium.org/2317693002/diff/1/build/toolchain/cros/BUILD.... > > build/toolchain/cros/BUILD.gn:19: cxx = compiler_prefix + cros_target_cxx > > exclude target build, because for some reason, iceccd doesn't dispatch > > target-build-jobs to clients > > /bin/sh -c icecc x86_64-cros-linux-gnu-g++ XXXXX > > Is this a bug? It seems like doing distributed builds for only the host part of > the compile is pretty useless (it's a tiny part of the build), and it's > misleading to say this option is supported. Agree. It isn't chromium issue. icecc-create-env expects gcc toolchain consisting of certain folder tree and files. However, x86_64-cros-linux-gnu toolchain doesn't respect the convention. https://github.com/icecc/icecream#cross-compiling-for-multiple-targets-in-the... So iceccd could not distribute x86_64-cros-linux-gnu toolchain to clients. There are two solution. * x86_64-cros-linux-gnu toolchain respects the convention. * change icecc source to recognize x86_64-cros-linux-gnu folder structure. > I'm thinking we should add an assert about icecc usage working, and add an > assertion that this flag isn't set on ChromeOS (referencing the bug) until the > bug is fixed. Then we enable it everywhere. Good idea. I will do it tomorrow.
Description was changed from ========== icecc support for cros GN supports icecc via |cc_wrapper| on all platforms except for cros. [1] This CL makes |cc_wrapper| work on cros also [2] icecc needs folloing additioinal GN flags use_debug_fission=false linux_use_bundled_binutils=false clang_use_chrome_plugins=false cc_wrapper="icecc" [1] https://crrev.com/e279af1c9c511972000049b2c4d261161b48075d [2] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro... ========== to ========== icecc support for cros GN supports icecc via |cc_wrapper| on all platforms except for cros. [1] This CL makes |cc_wrapper| work on cros also [2] icecc needs folloing additioinal GN flags use_debug_fission=false linux_use_bundled_binutils=false clang_use_chrome_plugins=false cc_wrapper="icecc" [1] https://crrev.com/e279af1c9c511972000049b2c4d261161b48075d [2] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro... ==========
dongseong.hwang@intel.com changed reviewers: - brettw@chromium.org, dongseong.hwang@chromium.org
Description was changed from ========== icecc support for cros GN supports icecc via |cc_wrapper| on all platforms except for cros. [1] This CL makes |cc_wrapper| work on cros also [2] icecc needs folloing additioinal GN flags use_debug_fission=false linux_use_bundled_binutils=false clang_use_chrome_plugins=false cc_wrapper="icecc" [1] https://crrev.com/e279af1c9c511972000049b2c4d261161b48075d [2] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro... ========== to ========== icecc support for cros GN supports icecc via |cc_wrapper| on all platforms except for cros. [1] This CL makes |cc_wrapper| work on cros also [2] icecc needs folloing additioinal GN flags use_debug_fission=false linux_use_bundled_binutils=false clang_use_chrome_plugins=false cc_wrapper="icecc" [1] https://crrev.com/e279af1c9c511972000049b2c4d261161b48075d [2] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro... BUG=645435 ==========
The CQ bit was checked by dongseong.hwang@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 ========== icecc support for cros GN supports icecc via |cc_wrapper| on all platforms except for cros. [1] This CL makes |cc_wrapper| work on cros also [2] icecc needs folloing additioinal GN flags use_debug_fission=false linux_use_bundled_binutils=false clang_use_chrome_plugins=false cc_wrapper="icecc" [1] https://crrev.com/e279af1c9c511972000049b2c4d261161b48075d [2] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro... BUG=645435 ========== to ========== icecc support for cros GN supports icecc via |cc_wrapper| on all platforms except for cros. [1] This CL makes |cc_wrapper| work on cros also [2] icecc needs folloing additioinal GN flags use_debug_fission=false linux_use_bundled_binutils=false clang_use_chrome_plugins=false cc_wrapper="icecc" In addition, it addes an assertion "cc_wrapper is not supported on target build." [1] https://crrev.com/e279af1c9c511972000049b2c4d261161b48075d [2] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro... BUG=645435 ==========
dongseong.hwang@intel.com changed reviewers: + brettw@chromium.org
On 2016/09/06 17:37:09, brettw (ping on IM after 24h) wrote: > I'm thinking we should add an assert about icecc usage working, and add an > assertion that this flag isn't set on ChromeOS (referencing the bug) until the > bug is fixed. Then we enable it everywhere. I filed crbug https://bugs.chromium.org/p/chromium/issues/detail?id=645435 and add an assertion in patch set 3. Preserve host build support, because it just works and assertion let user know target is not supported.
Description was changed from ========== icecc support for cros GN supports icecc via |cc_wrapper| on all platforms except for cros. [1] This CL makes |cc_wrapper| work on cros also [2] icecc needs folloing additioinal GN flags use_debug_fission=false linux_use_bundled_binutils=false clang_use_chrome_plugins=false cc_wrapper="icecc" In addition, it addes an assertion "cc_wrapper is not supported on target build." [1] https://crrev.com/e279af1c9c511972000049b2c4d261161b48075d [2] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro... BUG=645435 ========== to ========== icecc support for cros host build GN supports icecc via |cc_wrapper| on all platforms except for cros. [1] This CL makes |cc_wrapper| work on cros also [2] icecc needs folloing additioinal GN flags use_debug_fission=false linux_use_bundled_binutils=false clang_use_chrome_plugins=false cc_wrapper="icecc" In addition, it addes an assertion "cc_wrapper is not supported on target build." [1] https://crrev.com/e279af1c9c511972000049b2c4d261161b48075d [2] http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro... BUG=645435 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/09 12:22:05, dshwang wrote: > On 2016/09/06 17:37:09, brettw (ping on IM after 24h) wrote: > > I'm thinking we should add an assert about icecc usage working, and add an > > assertion that this flag isn't set on ChromeOS (referencing the bug) until the > > bug is fixed. Then we enable it everywhere. > > I filed crbug https://bugs.chromium.org/p/chromium/issues/detail?id=645435 > and add an assertion in patch set 3. > > Preserve host build support, because it just works and assertion let user know > target is not supported. Can you use the GN "assert" function instead, which will mark the run as failed? Otherwise this will just spam the console and nothing will happen. |
