Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10834296:
Second attempt at fixing Ninja build for Android and change make for host os optionally use goma (Closed)
|
Created:
8 years, 4 months ago by Yaron Modified:
8 years, 4 months ago Reviewers:
Nico CC:
chromium-reviews, peter+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, michaelbai, Peter Beverloo Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSecond attempt at fixing Ninja build for Android and change make for host os optionally use goma
Now, to build with goma, you must:
1) Set $GOMA_DIR
2) Specify Goma in the path such as:
PATH=$GOMA_DIR/android/:$PATH ninja -j250 -C out/Release base_unittests_apk
"2)" above is necessary temporarily until the *_target variables are removed from build/android/envsetup.sh
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151532
Patch Set 1 #
Total comments: 2
Patch Set 2 : simplify changes to just build/common.gyip #Messages
Total messages: 15 (0 generated)
Nico, can you take a look? I don't like that make uses "make_global_settings" and Ninja uses environment variables, but it seems like it's necessary since http://codereview.chromium.org/10836135/ as CC is now used for both build-types and Make uses exact paths to toolchain and ninja doesn't.
Nico, can you take a look? I don't like that make uses "make_global_settings" and Ninja uses environment variables, but it seems like it's necessary since http://codereview.chromium.org/10836135/ as CC is now used for both build-types and Make uses exact paths to toolchain and ninja doesn't.
On 2012/08/13 22:20:22, Yaron wrote: > Nico, can you take a look? I don't like that make uses "make_global_settings" > and Ninja uses environment variables, but it seems like it's necessary since > http://codereview.chromium.org/10836135/ as CC is now used for both build-types > and Make uses exact paths to toolchain and ninja doesn't. Ugh. Looks like I didn't get the make build right.. you can hold off or for now.
Since build/common.gypi mentions ANDROID_GOMA_WRAPPER , I don't understand why that approach wouldn't work with goma. http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh#newc... build/android/envsetup.sh:108: export CXX_host=g++ This makes it hard to build with clang I suppose? Also, wouldn't removing the GENERATOR!=ninja in build/common.gypi effectively do this same thing? As for the basename / absolute path issue, isn't that independent of make vs ninja and instead dependent on goma vs not goma?
http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh#newc... build/android/envsetup.sh:108: export CXX_host=g++ On 2012/08/13 22:34:31, Nico wrote: > This makes it hard to build with clang I suppose? > > Also, wouldn't removing the GENERATOR!=ninja in build/common.gypi effectively do > this same thing? > > As for the basename / absolute path issue, isn't that independent of make vs > ninja and instead dependent on goma vs not goma? True, this makes it so that ninja+clang doesn't work and yes it's more of a goma/non-goma issue but not entirely. You're right that removing the ninja guard in build/common.gypi works. However, it's a bit peculiar because it then is actually relying on the fact that CXX_target is being set and it's not actually using the fact that CC is being set in gyp. Also, it makes the local build not-gomaified. I think it's easier to follow if I just include this: $ head out/Release/build.ninja cc = arm-linux-androideabi-gcc cxx = arm-linux-androideabi-g++ ld = flock linker.lock $cxx ar = arm-linux-androideabi-ar ar_host = ar cc_host = /usr/bin/gcc cxx_host = /usr/bin/g++
On 2012/08/13 23:12:05, Yaron wrote: > http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh > File build/android/envsetup.sh (right): > > http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh#newc... > build/android/envsetup.sh:108: export CXX_host=g++ > On 2012/08/13 22:34:31, Nico wrote: > > This makes it hard to build with clang I suppose? > > > > Also, wouldn't removing the GENERATOR!=ninja in build/common.gypi effectively > do > > this same thing? > > > > As for the basename / absolute path issue, isn't that independent of make vs > > ninja and instead dependent on goma vs not goma? > > True, this makes it so that ninja+clang doesn't work and yes it's more of a > goma/non-goma issue but not entirely. > > You're right that removing the ninja guard in build/common.gypi works. However, > it's a bit peculiar because it then is actually relying on the fact that > CXX_target is being set What relies on this? > and it's not actually using the fact that CC is being > set in gyp. Also, it makes the local build not-gomaified. I think it's easier to > follow if I just include this: > > $ head out/Release/build.ninja > cc = arm-linux-androideabi-gcc > cxx = arm-linux-androideabi-g++ > ld = flock linker.lock $cxx > ar = arm-linux-androideabi-ar > ar_host = ar > cc_host = /usr/bin/gcc > cxx_host = /usr/bin/g++
On Mon, Aug 13, 2012 at 4:16 PM, <thakis@chromium.org> wrote: > On 2012/08/13 23:12:05, Yaron wrote: >> >> http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh >> File build/android/envsetup.sh (right): > > > > http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh#newc... >> >> build/android/envsetup.sh:108: export CXX_host=g++ >> On 2012/08/13 22:34:31, Nico wrote: >> > This makes it hard to build with clang I suppose? >> > >> > Also, wouldn't removing the GENERATOR!=ninja in build/common.gypi > > effectively >> >> do >> > this same thing? >> > >> > As for the basename / absolute path issue, isn't that independent of >> > make vs >> > ninja and instead dependent on goma vs not goma? > > >> True, this makes it so that ninja+clang doesn't work and yes it's more of >> a >> goma/non-goma issue but not entirely. > > >> You're right that removing the ninja guard in build/common.gypi works. > > However, >> >> it's a bit peculiar because it then is actually relying on the fact that >> CXX_target is being set > > > What relies on this? 1) v8 in v8/build/common.gypi (can be fixed separately) 2) I thought the build wouldn't use GOMA but turns out I wasn't specifying the GOMA_DIR variable. Ok! So one way to move forward be to: 1) remove ninja guard in build/common.gypi and 2) make the host toolchain always use the ANDROID_GOMA_WRAPPER I can then separately fix V8 gyp and afterwards remove the environment variables from envsetup. Thoughts? > > >> and it's not actually using the fact that CC is being >> set in gyp. Also, it makes the local build not-gomaified. I think it's >> easier > > to >> >> follow if I just include this: > > >> $ head out/Release/build.ninja >> cc = arm-linux-androideabi-gcc >> cxx = arm-linux-androideabi-g++ >> ld = flock linker.lock $cxx >> ar = arm-linux-androideabi-ar >> ar_host = ar >> cc_host = /usr/bin/gcc >> cxx_host = /usr/bin/g++ > > > > > http://codereview.chromium.org/10834296/
On Mon, Aug 13, 2012 at 4:43 PM, Yaron Friedman <yfriedman@chromium.org> wrote: > On Mon, Aug 13, 2012 at 4:16 PM, <thakis@chromium.org> wrote: >> On 2012/08/13 23:12:05, Yaron wrote: >>> >>> http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh >>> File build/android/envsetup.sh (right): >> >> >> >> http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh#newc... >>> >>> build/android/envsetup.sh:108: export CXX_host=g++ >>> On 2012/08/13 22:34:31, Nico wrote: >>> > This makes it hard to build with clang I suppose? >>> > >>> > Also, wouldn't removing the GENERATOR!=ninja in build/common.gypi >> >> effectively >>> >>> do >>> > this same thing? >>> > >>> > As for the basename / absolute path issue, isn't that independent of >>> > make vs >>> > ninja and instead dependent on goma vs not goma? >> >> >>> True, this makes it so that ninja+clang doesn't work and yes it's more of >>> a >>> goma/non-goma issue but not entirely. >> >> >>> You're right that removing the ninja guard in build/common.gypi works. >> >> However, >>> >>> it's a bit peculiar because it then is actually relying on the fact that >>> CXX_target is being set >> >> >> What relies on this? > > 1) v8 in v8/build/common.gypi (can be fixed separately) > 2) I thought the build wouldn't use GOMA but turns out I wasn't > specifying the GOMA_DIR variable. > > Ok! So one way to move forward be to: > 1) remove ninja guard in build/common.gypi and > 2) make the host toolchain always use the ANDROID_GOMA_WRAPPER > > I can then separately fix V8 gyp and afterwards remove the environment > variables from envsetup. ^ This sounds good to me. It'd also align the make and ninja builds more, right? > > Thoughts? >> >> >>> and it's not actually using the fact that CC is being >>> set in gyp. Also, it makes the local build not-gomaified. I think it's >>> easier >> >> to >>> >>> follow if I just include this: >> >> >>> $ head out/Release/build.ninja >>> cc = arm-linux-androideabi-gcc >>> cxx = arm-linux-androideabi-g++ >>> ld = flock linker.lock $cxx >>> ar = arm-linux-androideabi-ar >>> ar_host = ar >>> cc_host = /usr/bin/gcc >>> cxx_host = /usr/bin/g++ >> >> >> >> >> http://codereview.chromium.org/10834296/
On Mon, Aug 13, 2012 at 4:54 PM, Nico Weber <thakis@chromium.org> wrote: > On Mon, Aug 13, 2012 at 4:43 PM, Yaron Friedman <yfriedman@chromium.org> wrote: >> On Mon, Aug 13, 2012 at 4:16 PM, <thakis@chromium.org> wrote: >>> On 2012/08/13 23:12:05, Yaron wrote: >>>> >>>> http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh >>>> File build/android/envsetup.sh (right): >>> >>> >>> >>> http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh#newc... >>>> >>>> build/android/envsetup.sh:108: export CXX_host=g++ >>>> On 2012/08/13 22:34:31, Nico wrote: >>>> > This makes it hard to build with clang I suppose? >>>> > >>>> > Also, wouldn't removing the GENERATOR!=ninja in build/common.gypi >>> >>> effectively >>>> >>>> do >>>> > this same thing? >>>> > >>>> > As for the basename / absolute path issue, isn't that independent of >>>> > make vs >>>> > ninja and instead dependent on goma vs not goma? >>> >>> >>>> True, this makes it so that ninja+clang doesn't work and yes it's more of >>>> a >>>> goma/non-goma issue but not entirely. >>> >>> >>>> You're right that removing the ninja guard in build/common.gypi works. >>> >>> However, >>>> >>>> it's a bit peculiar because it then is actually relying on the fact that >>>> CXX_target is being set >>> >>> >>> What relies on this? >> >> 1) v8 in v8/build/common.gypi (can be fixed separately) >> 2) I thought the build wouldn't use GOMA but turns out I wasn't >> specifying the GOMA_DIR variable. >> >> Ok! So one way to move forward be to: >> 1) remove ninja guard in build/common.gypi and >> 2) make the host toolchain always use the ANDROID_GOMA_WRAPPER >> >> I can then separately fix V8 gyp and afterwards remove the environment >> variables from envsetup. > > ^ This sounds good to me. It'd also align the make and ninja builds more, right? > Yes, except that other ports rely on path configuration to choose goma/non-goma whereas Android would depend on GOMA_DIR. So consistent one way and not in another. Whatever. I guess I'll keep the two consistent for Android. >> >> Thoughts? >>> >>> >>>> and it's not actually using the fact that CC is being >>>> set in gyp. Also, it makes the local build not-gomaified. I think it's >>>> easier >>> >>> to >>>> >>>> follow if I just include this: >>> >>> >>>> $ head out/Release/build.ninja >>>> cc = arm-linux-androideabi-gcc >>>> cxx = arm-linux-androideabi-g++ >>>> ld = flock linker.lock $cxx >>>> ar = arm-linux-androideabi-ar >>>> ar_host = ar >>>> cc_host = /usr/bin/gcc >>>> cxx_host = /usr/bin/g++ >>> >>> >>> >>> >>> http://codereview.chromium.org/10834296/
On Mon, Aug 13, 2012 at 5:01 PM, Yaron Friedman <yfriedman@chromium.org> wrote: > On Mon, Aug 13, 2012 at 4:54 PM, Nico Weber <thakis@chromium.org> wrote: >> On Mon, Aug 13, 2012 at 4:43 PM, Yaron Friedman <yfriedman@chromium.org> wrote: >>> On Mon, Aug 13, 2012 at 4:16 PM, <thakis@chromium.org> wrote: >>>> On 2012/08/13 23:12:05, Yaron wrote: >>>>> >>>>> http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh >>>>> File build/android/envsetup.sh (right): >>>> >>>> >>>> >>>> http://codereview.chromium.org/10834296/diff/1/build/android/envsetup.sh#newc... >>>>> >>>>> build/android/envsetup.sh:108: export CXX_host=g++ >>>>> On 2012/08/13 22:34:31, Nico wrote: >>>>> > This makes it hard to build with clang I suppose? >>>>> > >>>>> > Also, wouldn't removing the GENERATOR!=ninja in build/common.gypi >>>> >>>> effectively >>>>> >>>>> do >>>>> > this same thing? >>>>> > >>>>> > As for the basename / absolute path issue, isn't that independent of >>>>> > make vs >>>>> > ninja and instead dependent on goma vs not goma? >>>> >>>> >>>>> True, this makes it so that ninja+clang doesn't work and yes it's more of >>>>> a >>>>> goma/non-goma issue but not entirely. >>>> >>>> >>>>> You're right that removing the ninja guard in build/common.gypi works. >>>> >>>> However, >>>>> >>>>> it's a bit peculiar because it then is actually relying on the fact that >>>>> CXX_target is being set >>>> >>>> >>>> What relies on this? >>> >>> 1) v8 in v8/build/common.gypi (can be fixed separately) >>> 2) I thought the build wouldn't use GOMA but turns out I wasn't >>> specifying the GOMA_DIR variable. >>> >>> Ok! So one way to move forward be to: >>> 1) remove ninja guard in build/common.gypi and >>> 2) make the host toolchain always use the ANDROID_GOMA_WRAPPER >>> >>> I can then separately fix V8 gyp and afterwards remove the environment >>> variables from envsetup. >> >> ^ This sounds good to me. It'd also align the make and ninja builds more, right? >> > Yes, except that other ports rely on path configuration to choose > goma/non-goma whereas Android would depend on GOMA_DIR. Path configuration as described on go/ma should still work with that change, since environment variables win over make_global_settings. So I think it's consistent with the rest of the world, but the Chrome/Android way of all these envsetup.sh files becomes internally consistent too as far as I understand. > So consistent > one way and not in another. Whatever. I guess I'll keep the two > consistent for Android. >>> >>> Thoughts? >>>> >>>> >>>>> and it's not actually using the fact that CC is being >>>>> set in gyp. Also, it makes the local build not-gomaified. I think it's >>>>> easier >>>> >>>> to >>>>> >>>>> follow if I just include this: >>>> >>>> >>>>> $ head out/Release/build.ninja >>>>> cc = arm-linux-androideabi-gcc >>>>> cxx = arm-linux-androideabi-g++ >>>>> ld = flock linker.lock $cxx >>>>> ar = arm-linux-androideabi-ar >>>>> ar_host = ar >>>>> cc_host = /usr/bin/gcc >>>>> cxx_host = /usr/bin/g++ >>>> >>>> >>>> >>>> >>>> http://codereview.chromium.org/10834296/
On goma/ it says to do something like: PATH=$GOMA_DIR/android/:$PATH ninja -j250 -C out/Release unit_tests_apk However, that won't change anything because |cc| would be set to /usr/bin/gcc, thus changing the path doesn't affect anything, right? I.e. after this current change, my out/Release/build.ninja is: cc = arm-linux-androideabi-gcc cxx = arm-linux-androideabi-g++ ld = flock linker.lock $cxx ar = arm-linux-androideabi-ar ar_host = ar cc_host = /usr/local/google/code/goma/gomacc /usr/bin/gcc cxx_host = /usr/local/google/code/goma/gomacc /usr/bin/g++ ld_host = flock linker.lock $cxx_host Once those variables are removed, the target binaries will look like the *_host ones and if gomacc isn't present, changing the path doesn't help. Of course I could just be missing something.
On 2012/08/14 00:36:52, Yaron wrote: > On goma/ it says to do something like: > > PATH=$GOMA_DIR/android/:$PATH ninja -j250 -C out/Release unit_tests_apk > > However, that won't change anything because |cc| would be set to /usr/bin/gcc, > thus changing the path doesn't affect anything, right? The goma clang instructions say to set CC too, so i suppose for android you'd have to set CC, CXX, CC_host, CXX_host to use goma. (Or, looking at your Release/build.ninja, just CC_host and CXX_host). What does your documentation recommend for make + goma? (In general, I feel that using goma is too hard on all platforms. I'm considering adding something like tools/goma/gyp.py and tools/goma/build.py which does the right gyp and ninja invocations with the right env vars and whatnot -- that could support android too) (Patch set 2 lgtm for what it's worth.) > I.e. after this current > change, my out/Release/build.ninja is: > > cc = arm-linux-androideabi-gcc > cxx = arm-linux-androideabi-g++ > ld = flock linker.lock $cxx > ar = arm-linux-androideabi-ar > ar_host = ar > cc_host = /usr/local/google/code/goma/gomacc /usr/bin/gcc > cxx_host = /usr/local/google/code/goma/gomacc /usr/bin/g++ > ld_host = flock linker.lock $cxx_host > > Once those variables are removed, the target binaries will look like the *_host > ones and if gomacc isn't present, changing the path doesn't help. Of course I > could just be missing something.
On 2012/08/14 00:41:39, Nico wrote: > On 2012/08/14 00:36:52, Yaron wrote: > > On goma/ it says to do something like: > > > > PATH=$GOMA_DIR/android/:$PATH ninja -j250 -C out/Release unit_tests_apk > > > > However, that won't change anything because |cc| would be set to /usr/bin/gcc, > > thus changing the path doesn't affect anything, right? > > The goma clang instructions say to set CC too, so i suppose for android you'd > have to set CC, CXX, CC_host, CXX_host to use goma. (Or, looking at your > Release/build.ninja, just CC_host and CXX_host). > > What does your documentation recommend for make + goma? > Ah, yes for clang that would be right. I was talking about the non-clang case. I don't think we've ever enabled goma+ninja+clang for cross-compiles on Android. I'll land this downstream first as we have more build configs there to verify. > (In general, I feel that using goma is too hard on all platforms. I'm > considering adding something like tools/goma/gyp.py and tools/goma/build.py > which does the right gyp and ninja invocations with the right env vars and > whatnot -- that could support android too) > > (Patch set 2 lgtm for what it's worth.) > > > I.e. after this current > > change, my out/Release/build.ninja is: > > > > cc = arm-linux-androideabi-gcc > > cxx = arm-linux-androideabi-g++ > > ld = flock linker.lock $cxx > > ar = arm-linux-androideabi-ar > > ar_host = ar > > cc_host = /usr/local/google/code/goma/gomacc /usr/bin/gcc > > cxx_host = /usr/local/google/code/goma/gomacc /usr/bin/g++ > > ld_host = flock linker.lock $cxx_host > > > > Once those variables are removed, the target binaries will look like the > *_host > > ones and if gomacc isn't present, changing the path doesn't help. Of course I > > could just be missing something.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10834296/9001
Change committed as 151532 |
