Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11137023:
Remove redundant command line option. (Closed)
|
Created:
8 years, 2 months ago by shashi Modified:
8 years, 2 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. |
DescriptionRemove redundant command line option.
--sdk option is no longer used or needed.
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163456
Patch Set 1 #
Total comments: 8
Patch Set 2 : more removal #
Messages
Total messages: 16 (0 generated)
https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (left): https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... build/android/envsetup.sh:25: -s | --sdk ) export ANDROID_SDK_BUILD=1 ; shift ;; Just thought that I can remove the above while loop. There is already a check for CHROME_ANDROID_BUILD_WEBVIEW for non sdk build. I am looking into downstream changes to enable this. This script will no longer need any options.
https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (left): https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... build/android/envsetup.sh:25: -s | --sdk ) export ANDROID_SDK_BUILD=1 ; shift ;; On 2012/10/15 22:29:20, shashi wrote: > Just thought that I can remove the above while loop. > There is already a check for CHROME_ANDROID_BUILD_WEBVIEW for non sdk build. > I am looking into downstream changes to enable this. > This script will no longer need any options. Yes, I'd prefer to remove this entirely. https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... build/android/envsetup.sh:15: # NOTE(yfriedman): This looks unnecessary but downstream the default value Please remove this NOTE https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... File build/android/envsetup_functions.sh (right): https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... build/android/envsetup_functions.sh:244: non_sdk_build_init() { Why not remove this section entirely?
https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... File build/android/envsetup_functions.sh (right): https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... build/android/envsetup_functions.sh:244: non_sdk_build_init() { On 2012/10/15 22:47:01, Yaron wrote: > Why not remove this section entirely? Partially because of power bot, which seems to be the only place using it. I am looking into ways in which I can remove this, I assume adding exports to buildbot scripts should fix it.
https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... File build/android/envsetup_functions.sh (right): https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... build/android/envsetup_functions.sh:244: non_sdk_build_init() { On 2012/10/15 22:54:50, shashi wrote: > On 2012/10/15 22:47:01, Yaron wrote: > > Why not remove this section entirely? > Partially because of power bot, which seems to be the only place using it. > I am looking into ways in which I can remove this, I assume adding exports to > buildbot scripts should fix it. I thought powerbot only needs lunch and doesn't need the rest of this build.
https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... File build/android/envsetup_functions.sh (right): https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... build/android/envsetup_functions.sh:244: non_sdk_build_init() { Just verified you are right :). Fixing power bot first. On 2012/10/15 23:11:32, Yaron wrote: > On 2012/10/15 22:54:50, shashi wrote: > > On 2012/10/15 22:47:01, Yaron wrote: > > > Why not remove this section entirely? > > Partially because of power bot, which seems to be the only place using it. > > I am looking into ways in which I can remove this, I assume adding exports to > > buildbot scripts should fix it. > > I thought powerbot only needs lunch and doesn't need the rest of this build.
https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... File build/android/envsetup_functions.sh (right): https://chromiumcodereview.appspot.com/11137023/diff/1/build/android/envsetup... build/android/envsetup_functions.sh:244: non_sdk_build_init() { Done. With Frank's change, this should be fixed. The issue was sdk_build_init unsets ANDROID_BUILD_TOP which lead to script failures. So doing a --no-sdk would avoid that. The code which does lunch and envsetup is in build bot repo, and the same code is shared between with both m18 and master for sharded perf bots ideally we should also remove that code, but it can also affect perf bots on m18. On 2012/10/15 23:37:33, shashi wrote: > Just verified you are right :). > Fixing power bot first. > On 2012/10/15 23:11:32, Yaron wrote: > > On 2012/10/15 22:54:50, shashi wrote: > > > On 2012/10/15 22:47:01, Yaron wrote: > > > > Why not remove this section entirely? > > > Partially because of power bot, which seems to be the only place using it. > > > I am looking into ways in which I can remove this, I assume adding exports > to > > > buildbot scripts should fix it. > > > > I thought powerbot only needs lunch and doesn't need the rest of this build. >
PTAL.
lgtm
lgtm I'm hoping we can continue to clean up/remove these configurations.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/11137023/9001
Retried try job too often for step(s) sync_integration_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shashishekhar@chromium.org/11137023/9001
Change committed as 163456
I had to revert part of this downstream during the merge. See: https://gerrit-int.chromium.org/gitweb?p=clank/chromium.git;a=commitdiff;h=13... Specifically, the --no-sdk parameter added by https://gerrit-int.chromium.org/26463 is still being referenced by clank/build/clank_buildbot.sh, so I had to preserve its functionality. Once no bots use non-SDK builds, --no-sdk should be removed from clank_buildbot.sh, and this should be re-landed downstream.
I have a CL specifically for removing this, which is under review. This should not affect any bots since webkit bot no longer relies on this parameter and all others use --sdk parameter. On 2012/10/24 18:45:51, johnme wrote: > I had to revert part of this downstream during the merge. > > See: > https://gerrit-int.chromium.org/gitweb?p=clank/chromium.git;a=commitdiff;h=13... > > Specifically, the --no-sdk parameter added by > https://gerrit-int.chromium.org/26463 is still being referenced by > clank/build/clank_buildbot.sh, so I had to preserve its functionality. > > Once no bots use non-SDK builds, --no-sdk should be removed from > clank_buildbot.sh, and this should be re-landed downstream. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
