Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10898019:
Adding support for ninja. (Closed)
|
Created:
8 years, 3 months ago by sivachandra Modified:
8 years, 3 months ago CC:
chromium-reviews, peter+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding support for ninja.
BUG=142465
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154587
Patch Set 1 #
Total comments: 2
Patch Set 2 : Response to yfriedman's comment. #
Total comments: 6
Patch Set 3 : Refactored based on ilevy's comments. #Patch Set 4 : Another round of refactor.wq #
Total comments: 1
Patch Set 5 : Add 'All' target for the ninja command. #
Total comments: 1
Patch Set 6 : Another refactor to fix failures on the try bot.wq #Messages
Total messages: 26 (0 generated)
https://chromiumcodereview.appspot.com/10898019/diff/1/build/android/buildbot... File build/android/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/10898019/diff/1/build/android/buildbot... build/android/buildbot_functions.sh:87: export GYP_GENERATORS=ninja You only want to set this if you're building for ninja.
https://chromiumcodereview.appspot.com/10898019/diff/1/build/android/buildbot... File build/android/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/10898019/diff/1/build/android/buildbot... build/android/buildbot_functions.sh:87: export GYP_GENERATORS=ninja On 2012/08/28 23:38:33, Yaron wrote: > You only want to set this if you're building for ninja. Moved to a more appropriate place in patch set 2.
https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... File build/android/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... build/android/buildbot_functions.sh:202: BUILDTOOL=$(bb_get_json_prop "$FACTORY_PROPERTIES" buildtool) Pass these variables as args rather than relying on them getting added to function's environment. https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... build/android/buildbot_functions.sh:207: export GYP_GENERATORS=ninja GYP_GENERATORS needs to be set to ninja before envsetup. See the email about building w/ ninja from yaron.
PTAL https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... File build/android/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... build/android/buildbot_functions.sh:202: BUILDTOOL=$(bb_get_json_prop "$FACTORY_PROPERTIES" buildtool) On 2012/08/29 17:29:05, Isaac wrote: > Pass these variables as args rather than relying on them getting added to > function's environment. I do not really understand you here. Are you saying BUILDTYPE and BUILDTOOL can be made arguments to bb_compile? I am not sure if this is OK as bb_compile is invoked from many files. However, I have moved the initialization of these variables to a different place in patch set 3. PTAL https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... build/android/buildbot_functions.sh:207: export GYP_GENERATORS=ninja On 2012/08/29 17:29:05, Isaac wrote: > GYP_GENERATORS needs to be set to ninja before envsetup. See the email about > building w/ ninja from yaron. I have moved it in patch set 3. PTAL
https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... File build/android/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... build/android/buildbot_functions.sh:202: BUILDTOOL=$(bb_get_json_prop "$FACTORY_PROPERTIES" buildtool) I'm saying bb_goma_ninja and bb_goma make are relying on these variables getting set and you should pass them as arguments instead of setting them in the environment. We want less environment variables and more explicit params. It's fine to parse them inside of bb_compile.
https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... File build/android/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/10898019/diff/4002/build/android/build... build/android/buildbot_functions.sh:202: BUILDTOOL=$(bb_get_json_prop "$FACTORY_PROPERTIES" buildtool) On 2012/08/29 19:48:59, Isaac wrote: > I'm saying bb_goma_ninja and bb_goma make are relying on these variables getting > set and you should pass them as arguments instead of setting them in the > environment. We want less environment variables and more explicit params. > > It's fine to parse them inside of bb_compile. OK I get the idea. However, since we need to set GYP_GENERATORS=ninja in bb_baseline_setup, we will need to parse FACTORY_PROPERTIES in or before bb_baseline_setup. In this view, is patch set 3 OK?
no, because you're still adding additional variables into the environment, which makes the code harder to follow and makes functions harder to call on their own.
On 2012/08/29 21:25:12, Isaac wrote: > no, because you're still adding additional variables into the environment, which > makes the code harder to follow and makes functions harder to call on their own. PTAL, patch set 4.
lgtm, thanks for fixing
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional 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.
rubberstamp lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@google.com/10898019/4003
https://chromiumcodereview.appspot.com/10898019/diff/4003/build/android/build... File build/android/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/10898019/diff/4003/build/android/build... build/android/buildbot_functions.sh:201: ninja -C out/$BUILDTYPE -j120 -l20 Ended up looking because Isaac asked me too. This doesn't specify what target to actually build. Make correctly defaults to "All" but for some reason ninja doesn't. Please specify it here.
Sorry about that -- thought we had corrected the 'all' target to point at 'All'. Maybe that was only downstream or for make.
PTAL https://chromiumcodereview.appspot.com/10898019/diff/16001/build/android/buil... File build/android/buildbot_functions.sh (right): https://chromiumcodereview.appspot.com/10898019/diff/16001/build/android/buil... build/android/buildbot_functions.sh:201: ninja -C out/$BUILDTYPE -j120 -l20 All It is all 'All'?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@google.com/10898019/16001
lgtm
Try job failure for 10898019-16001 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
Whoops, bb_goma_make was already taking arguments...
On 2012/08/30 22:29:53, Isaac wrote: > Whoops, bb_goma_make was already taking arguments... Id the last $@ argument to the make command not correct?
On 2012/08/31 16:23:00, sivachandra wrote: > On 2012/08/30 22:29:53, Isaac wrote: > > Whoops, bb_goma_make was already taking arguments... > > Id the last $@ argument to the make command not correct? I have uploaded patch set 6 based on offline discussion with ilevy.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@google.com/10898019/21002
Change committed as 154587 |
