Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184733006:
[Android] Fix gtest test scripts for non-default CHROMIUM_OUT_DIR. (Closed)
|
Created:
6 years, 9 months ago by jbudorick Modified:
6 years, 8 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[Android] Fix gtest test scripts for non-default CHROMIUM_OUT_DIR.
BUG=347834
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262446
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : #Patch Set 4 : rebase #Patch Set 5 : #Messages
Total messages: 23 (0 generated)
ping
https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/setup.py:167: constants.ISOLATE_DEPS_DIR, os.environ.get('CHROMIUM_OUT_DIR', 'out')) You're duplicating constants.GetOutDirectory(). Also what happened to patch to get rid of this env var?
https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/setup.py:167: constants.ISOLATE_DEPS_DIR, os.environ.get('CHROMIUM_OUT_DIR', 'out')) On 2014/03/05 01:33:59, frankf wrote: > You're duplicating constants.GetOutDirectory(). Also what happened to patch to > get rid of this env var? I tried constants.GetOutDirectory() at first because I thought the same thing. Unfortunately, that appends the build type to the end of CHROMIUM_OUT_DIR -- e.g., it would return 'out/Debug', while this has to be 'out'. I don't think I ever posted a patch to get rid of this environment variable, though I did play around with the idea locally. You suggested it in https://codereview.chromium.org/144093008
https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/setup.py:167: constants.ISOLATE_DEPS_DIR, os.environ.get('CHROMIUM_OUT_DIR', 'out')) Why can't we do os.path.join(constants.GetOutDirectory(), os.pardir) On 2014/03/07 07:15:28, jbudorick wrote: > On 2014/03/05 01:33:59, frankf wrote: > > You're duplicating constants.GetOutDirectory(). Also what happened to patch to > > get rid of this env var? > > I tried constants.GetOutDirectory() at first because I thought the same thing. > Unfortunately, that appends the build type to the end of CHROMIUM_OUT_DIR -- > e.g., it would return 'out/Debug', while this has to be 'out'. > > I don't think I ever posted a patch to get rid of this environment variable, > though I did play around with the idea locally. You suggested it in > https://codereview.chromium.org/144093008
https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/setup.py:167: constants.ISOLATE_DEPS_DIR, os.environ.get('CHROMIUM_OUT_DIR', 'out')) On 2014/03/10 17:35:34, frankf wrote: > Why can't we do os.path.join(constants.GetOutDirectory(), os.pardir) > > On 2014/03/07 07:15:28, jbudorick wrote: > > On 2014/03/05 01:33:59, frankf wrote: > > > You're duplicating constants.GetOutDirectory(). Also what happened to patch > to > > > get rid of this env var? > > > > I tried constants.GetOutDirectory() at first because I thought the same thing. > > Unfortunately, that appends the build type to the end of CHROMIUM_OUT_DIR -- > > e.g., it would return 'out/Debug', while this has to be 'out'. > > > > I don't think I ever posted a patch to get rid of this environment variable, > > though I did play around with the idea locally. You suggested it in > > https://codereview.chromium.org/144093008 > We can, and I'm ok with doing so, but how is that any better?
https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... File build/android/pylib/gtest/setup.py (right): https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... build/android/pylib/gtest/setup.py:167: constants.ISOLATE_DEPS_DIR, os.environ.get('CHROMIUM_OUT_DIR', 'out')) It's DRY. Also you're encapsulating how the path is obtained. On 2014/03/11 14:25:09, jbudorick wrote: > On 2014/03/10 17:35:34, frankf wrote: > > Why can't we do os.path.join(constants.GetOutDirectory(), os.pardir) > > > > On 2014/03/07 07:15:28, jbudorick wrote: > > > On 2014/03/05 01:33:59, frankf wrote: > > > > You're duplicating constants.GetOutDirectory(). Also what happened to > patch > > to > > > > get rid of this env var? > > > > > > I tried constants.GetOutDirectory() at first because I thought the same > thing. > > > Unfortunately, that appends the build type to the end of CHROMIUM_OUT_DIR -- > > > e.g., it would return 'out/Debug', while this has to be 'out'. > > > > > > I don't think I ever posted a patch to get rid of this environment variable, > > > though I did play around with the idea locally. You suggested it in > > > https://codereview.chromium.org/144093008 > > > > We can, and I'm ok with doing so, but how is that any better?
On 2014/03/11 16:27:33, frankf wrote: > https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... > File build/android/pylib/gtest/setup.py (right): > > https://codereview.chromium.org/184733006/diff/20001/build/android/pylib/gtes... > build/android/pylib/gtest/setup.py:167: constants.ISOLATE_DEPS_DIR, > os.environ.get('CHROMIUM_OUT_DIR', 'out')) > It's DRY. Also you're encapsulating how the path is obtained. > > On 2014/03/11 14:25:09, jbudorick wrote: > > On 2014/03/10 17:35:34, frankf wrote: > > > Why can't we do os.path.join(constants.GetOutDirectory(), os.pardir) > > > > > > On 2014/03/07 07:15:28, jbudorick wrote: > > > > On 2014/03/05 01:33:59, frankf wrote: > > > > > You're duplicating constants.GetOutDirectory(). Also what happened to > > patch > > > to > > > > > get rid of this env var? > > > > > > > > I tried constants.GetOutDirectory() at first because I thought the same > > thing. > > > > Unfortunately, that appends the build type to the end of CHROMIUM_OUT_DIR > -- > > > > e.g., it would return 'out/Debug', while this has to be 'out'. > > > > > > > > I don't think I ever posted a patch to get rid of this environment > variable, > > > > though I did play around with the idea locally. You suggested it in > > > > https://codereview.chromium.org/144093008 > > > > > > > We can, and I'm ok with doing so, but how is that any better? I don't really agree that either of those factors are relevant -- the suggestion is very dependent on how GetOutDirectory() is implemented, and reversing part of what we do in GetOutDirectory() doesn't seem any better than repeating part of what we do in GetOutDirectory() -- but I guess that, even at that point, it's probably a push, so I can change it.
(old) ping
lgtm.
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbudorick@chromium.org/184733006/60001
The CQ bit was unchecked by jbudorick@chromium.org
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbudorick@chromium.org/184733006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbudorick@chromium.org/184733006/80001
The CQ bit was unchecked by jbudorick@chromium.org
The CQ bit was checked by jbudorick@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbudorick@chromium.org/184733006/80001
Message was sent while issue was closed.
Change committed as 262446 |
