Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10867008:
Get rid of device/host clock synchronization in android_commands.py. (Closed)
|
Created:
8 years, 4 months ago by Philippe Modified:
8 years, 3 months ago CC:
chromium-reviews, pam+watch_chromium.org, peter+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGet rid of device/host clock synchronization in android_commands.py.
Clocks can't be synchronized programmatically on non-rooted devices due to the
inability to use the SET_TIME permission in non-system apps.
We were depending on synchronization in AndroidCommands.PushIfNeeded() to
handle incremental data push by comparing host and device file timestamps.
This CL adds tools/android/m5sum and uses it in android_commands.py so that we
can avoid depending on dates to handle incremental data push.
This is a first step towards clock synchronization removal.
BUG=143114
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154751
Patch Set 1 : #
Total comments: 20
Patch Set 2 : Make 1024 a constant #Patch Set 3 : Address Isaac's comments #
Total comments: 9
Patch Set 4 : Address Isaac's comments #Patch Set 5 : Get rid of $STRIP in md5sum.gyp #
Total comments: 8
Patch Set 6 : Address Peter's comments #Patch Set 7 : Fix GYP file for Ninja builds #Patch Set 8 : Small changes in android_commands.py. #
Total comments: 4
Patch Set 9 : Address Marcus' comments #
Total comments: 4
Patch Set 10 : Address Marcus' comments #
Messages
Total messages: 34 (0 generated)
+isaac
Thanks for the CC. Looks good! I have a couple suggestions... :-) https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:25: sys.path.append(os.path.join(os.path.abspath(os.path.dirname(__file__)), '..', Extract part of this into a var named CHROME_SRC? https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:168: def _ComputeFileListHash(md5sum_output): Consider using a list comprehension: return [l.split(' ')[0] for l in lines] https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:171: lines = md5sum_output Unnecessary variable? https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:203: chrome_src_path = os.getenv('CHROME_SRC') prefer we avoid new environment variables dependencies. Look at the way we import android_testrunner at the top of file, maybe you can look at CHROME_SRC but have a viable default value. https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:205: self._md5sum_path = '%s/out/Debug/md5sum' % (chrome_src_path) rather than guessing Release vs. Debug, maybe take optional arguments. https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:539: self.RunShellCommand(MD5SUM_DEVICE_PATH + ' ' + device_path)) Push md5sum binary if not on device? https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:542: subprocess.Popen('%s_host_bin %s' % (self._md5sum_path, local_path), Look at cmd_helper.py :: GetCmdOutput() https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:977: status = self._adb.SendShellCommand( 1) Prefer self.RunShellCommand() (they do the same thing) 2) This seems a bit hacky. Would ls -a work better?
https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:25: sys.path.append(os.path.join(os.path.abspath(os.path.dirname(__file__)), '..', On 2012/08/27 10:43:15, Isaac wrote: > Extract part of this into a var named CHROME_SRC? Done. https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:168: def _ComputeFileListHash(md5sum_output): On 2012/08/27 10:43:15, Isaac wrote: > Consider using a list comprehension: > return [l.split(' ')[0] for l in lines] Good point, indeed. https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:171: lines = md5sum_output On 2012/08/27 10:43:15, Isaac wrote: > Unnecessary variable? Done. https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:203: chrome_src_path = os.getenv('CHROME_SRC') On 2012/08/27 10:43:15, Isaac wrote: > prefer we avoid new environment variables dependencies. Look at the way we > import android_testrunner at the top of file, maybe you can look at CHROME_SRC > but have a viable default value. $CHROME_SRC is not new but indeed it's better to use CHROME_SRC defined at the top of the file. https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:205: self._md5sum_path = '%s/out/Debug/md5sum' % (chrome_src_path) On 2012/08/27 10:43:15, Isaac wrote: > rather than guessing Release vs. Debug, maybe take optional arguments. Unfortunately some clients of AndroidCommands don't know the build type and have no simple way to figure it out I think (e.g. buildbot_functions.sh). The problem we have right now that I already faced last week is that we don't clearly export the build type. We should export $BUILD_TYPE very early (e.g. in envsetup.sh) rather than setting it in some late python scripts and then passing it around (we could put it in constants.py and set it from $BUILD_TYPE). What do you think? Xianzhu should be part of this discussion too. About this specific case, we can also push the md5sum binary in all cases. It would be simpler and probably safer without impacting tests time too much assuming that clients don't instantiate AndroidCommands() in a loop. https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:539: self.RunShellCommand(MD5SUM_DEVICE_PATH + ' ' + device_path)) On 2012/08/27 10:43:15, Isaac wrote: > Push md5sum binary if not on device? This is in the constructor on line 212. I avoided putting the check here (needed only once) since FileExistsOnDevice() is surprisingly quite expensive (~150ms IIRC). https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:542: subprocess.Popen('%s_host_bin %s' % (self._md5sum_path, local_path), On 2012/08/27 10:43:15, Isaac wrote: > Look at cmd_helper.py :: GetCmdOutput() Thanks. FYI, I'm using shell=True because subprocess.Popen() uses execvp() with shell=False which only works with a relative path. https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:977: status = self._adb.SendShellCommand( On 2012/08/27 10:43:15, Isaac wrote: > 1) Prefer self.RunShellCommand() (they do the same thing) > > 2) This seems a bit hacky. Would ls -a work better? This is coming from downstream. I personally think test -f is nice but using $? is shorter. Which part did you not like?
https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:205: self._md5sum_path = '%s/out/Debug/md5sum' % (chrome_src_path) I've recently added the build type to buildbot factory properties and as of yesterday, buildbot_functions extract this from factory properties. (https://chromiumcodereview.appspot.com/10878031/). But better to parse factory properties in python. Look at bb_zip_build in buildbot_functions and zip_build.py for how the parsing works. Basically: # /b/build/slave/scripts is in buildbot pythonpath from common import chromium_utils .... chromium_utils.AddPropertiesOptions(parser) options, args = parser.parse_args() target = options.factory_properties['target'] https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:977: status = self._adb.SendShellCommand( On 2012/08/28 09:48:29, Philippe wrote: > On 2012/08/27 10:43:15, Isaac wrote: > > 1) Prefer self.RunShellCommand() (they do the same thing) > > > > 2) This seems a bit hacky. Would ls -a work better? > > This is coming from downstream. I personally think test -f is nice but using $? > is shorter. > Which part did you not like? Quote escaping, as that will be hard to understand later. Can you add a comment about why you needed to do it like that?
https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:205: self._md5sum_path = '%s/out/Debug/md5sum' % (chrome_src_path) On 2012/08/28 17:12:11, Isaac wrote: > I've recently added the build type to buildbot factory properties and as of > yesterday, buildbot_functions extract this from factory properties. > (https://chromiumcodereview.appspot.com/10878031/). But better to parse factory > properties in python. Look at bb_zip_build in buildbot_functions and > zip_build.py for how the parsing works. Basically: > > # /b/build/slave/scripts is in buildbot pythonpath > from common import chromium_utils > .... > chromium_utils.AddPropertiesOptions(parser) > options, args = parser.parse_args() > > target = options.factory_properties['target'] What about non-buildbot environments (i.e. developer environments)? That's why I wanted to handle that in envsetup so that all environments are affected. I think it's good to try to keep bots/developer environments consistent so that we can easily debug locally. Am I misunderstanding something? https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... build/android/pylib/android_commands.py:980: '\'test -f "%s"; echo $?\'' % (file_name)) Do we still need a comment here? I think it's much simpler now.
https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/7001/build/android/pylib... build/android/pylib/android_commands.py:205: self._md5sum_path = '%s/out/Debug/md5sum' % (chrome_src_path) Good point. I think this is OK for now then. https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... build/android/pylib/android_commands.py:206: print >>sys.stderr, 'Please build md5sum (\'make md5sum\')' nit: you can remove the backslash escape chars if you change the outer quotes to double quotes. https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... build/android/pylib/android_commands.py:207: sys.exit(1) Most functions in android_commands do not require md5sum_path. Is there a way to not crash if this is missing? Otherwise users of this class with stale out directories could get a runtime exception until they rebuild https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... build/android/pylib/android_commands.py:212: assert self._adb.SendCommand(command) Is this assert being used correctly? it will success as long as there is output from _adb.SendCommand() https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... build/android/pylib/android_commands.py:980: '\'test -f "%s"; echo $?\'' % (file_name)) On 2012/08/29 08:36:45, Philippe wrote: > Do we still need a comment here? I think it's much simpler now. this is fine, thanks
python lgtm w/ some comments on the latest patch
https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... build/android/pylib/android_commands.py:206: print >>sys.stderr, 'Please build md5sum (\'make md5sum\')' On 2012/08/30 06:50:57, Isaac wrote: > nit: you can remove the backslash escape chars if you change the outer quotes to > double quotes. Done. https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... build/android/pylib/android_commands.py:207: sys.exit(1) On 2012/08/30 06:50:57, Isaac wrote: > Most functions in android_commands do not require md5sum_path. Is there a way > to not crash if this is missing? Otherwise users of this class with stale out > directories could get a runtime exception until they rebuild Good point. I moved this check to PushIfNeeded(). https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... build/android/pylib/android_commands.py:212: assert self._adb.SendCommand(command) On 2012/08/30 06:50:57, Isaac wrote: > Is this assert being used correctly? it will success as long as there is output > from _adb.SendCommand() Indeed. I moved the code we had in PushIfNeeded() to a new helper function, _HasAdbPushSucceeded() so that I can reuse it here. https://chromiumcodereview.appspot.com/10867008/diff/17002/build/android/pyli... build/android/pylib/android_commands.py:980: '\'test -f "%s"; echo $?\'' % (file_name)) On 2012/08/30 06:50:57, Isaac wrote: > On 2012/08/29 08:36:45, Philippe wrote: > > Do we still need a comment here? I think it's much simpler now. > > this is fine, thanks Done.
Thanks for removing $STRIP already :). Some more nits. https://chromiumcodereview.appspot.com/10867008/diff/24003/build/all_android.gyp File build/all_android.gyp (right): https://chromiumcodereview.appspot.com/10867008/diff/24003/build/all_android.... build/all_android.gyp:47: '../tools/android/md5sum/md5sum.gyp:md5sum', nit: unless you plan on writing tests for this, it probably shouldn't be under line 42. Same goes for the other tools. https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... build/android/pylib/android_commands.py:539: |test_data_paths| that already exist on the device with the same md5. nit: s/md5/hash? https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... build/android/pylib/android_commands.py:546: sys.exit(1) Should we do this check just once in the constructor (where the path is being determined as well) instead of during every push? https://chromiumcodereview.appspot.com/10867008/diff/24003/tools/android/md5s... File tools/android/md5sum/md5sum.gyp (right): https://chromiumcodereview.appspot.com/10867008/diff/24003/tools/android/md5s... tools/android/md5sum/md5sum.gyp:51: 'target_name': 'md5sum_host_bin', We can have a single target and include both host as target in the 'toolsets' option.
lgtm
I also had to change the GYP file slightly. It wasn't working with Ninja due to the fact that the 'strip_md5sum_device_bin' action's output had the same name as the 'md5sum' target. https://chromiumcodereview.appspot.com/10867008/diff/24003/build/all_android.gyp File build/all_android.gyp (right): https://chromiumcodereview.appspot.com/10867008/diff/24003/build/all_android.... build/all_android.gyp:47: '../tools/android/md5sum/md5sum.gyp:md5sum', On 2012/08/30 12:45:14, Peter Beverloo wrote: > nit: unless you plan on writing tests for this, it probably shouldn't be under > line 42. Same goes for the other tools. Done. https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... build/android/pylib/android_commands.py:539: |test_data_paths| that already exist on the device with the same md5. On 2012/08/30 12:45:14, Peter Beverloo wrote: > nit: s/md5/hash? Done. https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... build/android/pylib/android_commands.py:546: sys.exit(1) On 2012/08/30 12:45:14, Peter Beverloo wrote: > Should we do this check just once in the constructor (where the path is being > determined as well) instead of during every push? This is what I did initially. But Isaac said that this class has a lot of clients who don't care about pushing files. Therefore it would be a bit overkill for them to exit(1) in the constructor. I agree with him. What do you think? https://chromiumcodereview.appspot.com/10867008/diff/24003/tools/android/md5s... File tools/android/md5sum/md5sum.gyp (right): https://chromiumcodereview.appspot.com/10867008/diff/24003/tools/android/md5s... tools/android/md5sum/md5sum.gyp:51: 'target_name': 'md5sum_host_bin', On 2012/08/30 12:45:14, Peter Beverloo wrote: > We can have a single target and include both host as target in the 'toolsets' > option. Unfortunately I didn't manage to make it work that way. I have this kind of warning when running make: tools/android/md5sum/md5sum.target.mk:231: warning: overriding commands for target `out/Debug/md5sum' tools/android/md5sum/md5sum.host.mk:183: warning: ignoring old commands for target `out/Debug/md5sum'. I tried many different approaches, one of them being having a single 'md5sum' simple target with toolsets = ['host', 'target']. Nothing worked :/
On 2012/08/31 11:57:39, Philippe wrote: > I also had to change the GYP file slightly. It wasn't working with Ninja due to > the fact that the 'strip_md5sum_device_bin' action's output had the same name as > the 'md5sum' target. > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/all_android.gyp > File build/all_android.gyp (right): > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/all_android.... > build/all_android.gyp:47: '../tools/android/md5sum/md5sum.gyp:md5sum', > On 2012/08/30 12:45:14, Peter Beverloo wrote: > > nit: unless you plan on writing tests for this, it probably shouldn't be under > > line 42. Same goes for the other tools. > > Done. > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... > File build/android/pylib/android_commands.py (right): > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... > build/android/pylib/android_commands.py:539: |test_data_paths| that already > exist on the device with the same md5. > On 2012/08/30 12:45:14, Peter Beverloo wrote: > > nit: s/md5/hash? > > Done. > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... > build/android/pylib/android_commands.py:546: sys.exit(1) > On 2012/08/30 12:45:14, Peter Beverloo wrote: > > Should we do this check just once in the constructor (where the path is being > > determined as well) instead of during every push? > > This is what I did initially. But Isaac said that this class has a lot of > clients who don't care about pushing files. Therefore it would be a bit overkill > for them to exit(1) in the constructor. I agree with him. What do you think? > > https://chromiumcodereview.appspot.com/10867008/diff/24003/tools/android/md5s... > File tools/android/md5sum/md5sum.gyp (right): > > https://chromiumcodereview.appspot.com/10867008/diff/24003/tools/android/md5s... > tools/android/md5sum/md5sum.gyp:51: 'target_name': 'md5sum_host_bin', > On 2012/08/30 12:45:14, Peter Beverloo wrote: > > We can have a single target and include both host as target in the 'toolsets' > > option. > > Unfortunately I didn't manage to make it work that way. I have this kind of > warning when running make: > tools/android/md5sum/md5sum.target.mk:231: warning: overriding commands for > target `out/Debug/md5sum' > tools/android/md5sum/md5sum.host.mk:183: warning: ignoring old commands for > target `out/Debug/md5sum'. > > I tried many different approaches, one of them being having a single 'md5sum' > simple target with toolsets = ['host', 'target']. Nothing worked :/ I did a sync too.
On 2012/08/31 11:57:52, Philippe wrote: > On 2012/08/31 11:57:39, Philippe wrote: > > I also had to change the GYP file slightly. It wasn't working with Ninja due > to > > the fact that the 'strip_md5sum_device_bin' action's output had the same name > as > > the 'md5sum' target. > > > > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/all_android.gyp > > File build/all_android.gyp (right): > > > > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/all_android.... > > build/all_android.gyp:47: '../tools/android/md5sum/md5sum.gyp:md5sum', > > On 2012/08/30 12:45:14, Peter Beverloo wrote: > > > nit: unless you plan on writing tests for this, it probably shouldn't be > under > > > line 42. Same goes for the other tools. > > > > Done. > > > > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... > > File build/android/pylib/android_commands.py (right): > > > > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... > > build/android/pylib/android_commands.py:539: |test_data_paths| that already > > exist on the device with the same md5. > > On 2012/08/30 12:45:14, Peter Beverloo wrote: > > > nit: s/md5/hash? > > > > Done. > > > > > https://chromiumcodereview.appspot.com/10867008/diff/24003/build/android/pyli... > > build/android/pylib/android_commands.py:546: sys.exit(1) > > On 2012/08/30 12:45:14, Peter Beverloo wrote: > > > Should we do this check just once in the constructor (where the path is > being > > > determined as well) instead of during every push? > > > > This is what I did initially. But Isaac said that this class has a lot of > > clients who don't care about pushing files. Therefore it would be a bit > overkill > > for them to exit(1) in the constructor. I agree with him. What do you think? > > > > > https://chromiumcodereview.appspot.com/10867008/diff/24003/tools/android/md5s... > > File tools/android/md5sum/md5sum.gyp (right): > > > > > https://chromiumcodereview.appspot.com/10867008/diff/24003/tools/android/md5s... > > tools/android/md5sum/md5sum.gyp:51: 'target_name': 'md5sum_host_bin', > > On 2012/08/30 12:45:14, Peter Beverloo wrote: > > > We can have a single target and include both host as target in the > 'toolsets' > > > option. > > > > Unfortunately I didn't manage to make it work that way. I have this kind of > > warning when running make: > > tools/android/md5sum/md5sum.target.mk:231: warning: overriding commands for > > target `out/Debug/md5sum' > > tools/android/md5sum/md5sum.host.mk:183: warning: ignoring old commands for > > target `out/Debug/md5sum'. > > > > I tried many different approaches, one of them being having a single 'md5sum' > > simple target with toolsets = ['host', 'target']. Nothing worked :/ > > I did a sync too. I'm waiting for your final check Peter before I submit this.
Ok, thanks for trying. We can always iterate on the gyp optimizations later on. LGTM
On 2012/09/03 09:13:40, Peter Beverloo wrote: > Ok, thanks for trying. We can always iterate on the gyp optimizations later on. > > LGTM I uploaded a new patch set fixing some small issues introduced in android_commands.py during the review process. I'm running run_tests.py on my workstation, and will submit this CL as soon as it finishes.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10867008/25003
Presubmit check for 10867008-25003 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
tools/android
Presubmit checks took 3.9s to calculate.
+Marcus for tools/android/md5sum.
lgtm, just a couple of nits and one suggestion for pushing the new tool to the device: https://chromiumcodereview.appspot.com/10867008/diff/25003/build/android/pyli... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/25003/build/android/pyli... build/android/pylib/android_commands.py:212: self._md5sum_path = '%s/out/Debug/md5sum_bin' % (CHROME_SRC) nit: it'd be nice to test first for $BUILD_TYPE, then fallback to Debug then Release.. (otherwise, we could potentially get something stale here..). feel free to do it separately.. https://chromiumcodereview.appspot.com/10867008/diff/25003/build/android/pyli... build/android/pylib/android_commands.py:216: if self._md5sum_path and not self.FileExistsOnDevice(MD5SUM_DEVICE_PATH): hmm... AndroidCommands is instantiated a lot of times in many places... doing this file copy in init is probably a bit of an overhead... can we lazily copy this in PushIfNeeded? I mean, if we keep an extra flag to indicate whether or not this has been pushed and do it only once, it'll be no worse for PushIfNeeded.. and it'll be much better for everything else... thoughts? nit: self._md5sum_path can never be false, so probably simpler to just keep the second condition..
Thanks Marcus. Please take another look before I submit this. https://chromiumcodereview.appspot.com/10867008/diff/25003/build/android/pyli... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/25003/build/android/pyli... build/android/pylib/android_commands.py:212: self._md5sum_path = '%s/out/Debug/md5sum_bin' % (CHROME_SRC) On 2012/09/03 13:22:15, bulach wrote: > nit: it'd be nice to test first for $BUILD_TYPE, then fallback to Debug then > Release.. (otherwise, we could potentially get something stale here..). feel > free to do it separately.. I fixed the issue here. $BUILD_TYPE is only defined on buildbots IIRC. By the way, as I said in some earlier comments I think we should export it for everyone very early (e.g. in envsetup.sh) so that we can have a "universal" way to figure out what build type we are using. I'm surprised that this need wasn't raised earlier. We should not have for build bots a configuration differing from developer environments. It makes our life unnecessarily hard when trying to reproduce build errors locally. If we had this environment variable exported for everyone, this whole block of code (now moved to PushIfNeeded()) which probably exists in other places too could be replaced with a single line. What do you think? https://chromiumcodereview.appspot.com/10867008/diff/25003/build/android/pyli... build/android/pylib/android_commands.py:216: if self._md5sum_path and not self.FileExistsOnDevice(MD5SUM_DEVICE_PATH): On 2012/09/03 13:22:15, bulach wrote: > hmm... AndroidCommands is instantiated a lot of times in many places... doing > this file copy in init is probably a bit of an overhead... can we lazily copy > this in PushIfNeeded? I mean, if we keep an extra flag to indicate whether or > not this has been pushed and do it only once, it'll be no worse for > PushIfNeeded.. and it'll be much better for everything else... thoughts? > > nit: self._md5sum_path can never be false, so probably simpler to just keep the > second condition.. Good point.
lgtm, thanks! exporting BUILDTYPE seems a good idea, but I haven't participated in that change at all :) maybe ask wangxianzhu? other than that, just a couple of tiny nits, feel free to go ahead once you address them! https://chromiumcodereview.appspot.com/10867008/diff/36010/build/android/pyli... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/36010/build/android/pyli... build/android/pylib/android_commands.py:540: default_build_type = os.environ['BUILD_TYPE'] if 'BUILD_TYPE' in ( nit: default_build_type = os.environ.get('BUILD_TYPE', 'Debug') https://chromiumcodereview.appspot.com/10867008/diff/36010/build/android/pyli... build/android/pylib/android_commands.py:546: print >>sys.stderr, "Please build md5sum ('make md5sum')" nit: need to use ' consistently for string.. since we're transitioning to ninja anyways, maybe simpler to just keep: 'Please build md5sum'
https://chromiumcodereview.appspot.com/10867008/diff/36010/build/android/pyli... File build/android/pylib/android_commands.py (right): https://chromiumcodereview.appspot.com/10867008/diff/36010/build/android/pyli... build/android/pylib/android_commands.py:540: default_build_type = os.environ['BUILD_TYPE'] if 'BUILD_TYPE' in ( On 2012/09/03 14:20:55, bulach wrote: > nit: > default_build_type = os.environ.get('BUILD_TYPE', 'Debug') Nice! https://chromiumcodereview.appspot.com/10867008/diff/36010/build/android/pyli... build/android/pylib/android_commands.py:546: print >>sys.stderr, "Please build md5sum ('make md5sum')" On 2012/09/03 14:20:55, bulach wrote: > nit: > need to use ' consistently for string.. since we're transitioning to ninja > anyways, maybe simpler to just keep: > 'Please build md5sum' Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10867008/26004
I will commit this tomorrow. I prefer to be around in case something breaks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10867008/26004
On 2012/09/04 07:48:27, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/pliard%40chromium.org/10867008/26004 I'm adding Xianzhu to discuss build type-related aspects. As I said in some earlier comments, I had to add (again) some code dealing with the build type in android_commands.py. This is not the first time I have to write this kind of code. I guess this logic (checking $BUILD_TYPE, then looking at out/Release/... and out/Debug/.. or better but longer, compare the Debug/Release files timestamps) is duplicated in several places of the Python/Shell codebase. I suggested here that we generalize $BUILD_TYPE. Rather than exporting it only on buildbots, which makes our life a bit hard when trying to reproduce errors on our workstation, we could export it very early in envsetup.sh so that everyone can rely on it confidently. I remember that we already had a similar discussion Xianzhu but I don't remember the details exactly. What would prevent us from doing that? I think we need a universal and reliable way to figure out the build type regardless of the language the script is written in (Python vs Shell) and the environment the project is built in (buildbot vs developer workstation). Cheers, Philippe.
On 2012/09/04 08:53:04, Philippe wrote: > On 2012/09/04 07:48:27, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/pliard%2540chromium.org/10867008/26004 > > I'm adding Xianzhu to discuss build type-related aspects. > > As I said in some earlier comments, I had to add (again) some code dealing with > the build type in android_commands.py. This is not the first time I have to > write this kind of code. I guess this logic (checking $BUILD_TYPE, then looking > at out/Release/... and out/Debug/.. or better but longer, compare the > Debug/Release files timestamps) is duplicated in several places of the > Python/Shell codebase. > I suggested here that we generalize $BUILD_TYPE. Rather than exporting it only > on buildbots, which makes our life a bit hard when trying to reproduce errors on > our workstation, we could export it very early in envsetup.sh so that everyone > can rely on it confidently. > I remember that we already had a similar discussion Xianzhu but I don't remember > the details exactly. What would prevent us from doing that? > I think we need a universal and reliable way to figure out the build type > regardless of the language the script is written in (Python vs Shell) and the > environment the project is built in (buildbot vs developer workstation). > > Cheers, > Philippe. Exporting variables is something we shouldn't be doing.. See Issue 142642 and Issue 145755 for more context on the "why" here. Doing so for now seems fine, as I don't want you to be blocked on me, but if there is an alternative solution that'd be preferred.
On 2012/09/04 09:51:45, Peter Beverloo wrote: > On 2012/09/04 08:53:04, Philippe wrote: > > On 2012/09/04 07:48:27, I haz the power (commit-bot) wrote: > > > CQ is trying da patch. Follow status at > > > > https://chromium-status.appspot.com/cq/pliard%252540chromium.org/10867008/26004 > > > > I'm adding Xianzhu to discuss build type-related aspects. > > > > As I said in some earlier comments, I had to add (again) some code dealing > with > > the build type in android_commands.py. This is not the first time I have to > > write this kind of code. I guess this logic (checking $BUILD_TYPE, then > looking > > at out/Release/... and out/Debug/.. or better but longer, compare the > > Debug/Release files timestamps) is duplicated in several places of the > > Python/Shell codebase. > > I suggested here that we generalize $BUILD_TYPE. Rather than exporting it only > > on buildbots, which makes our life a bit hard when trying to reproduce errors > on > > our workstation, we could export it very early in envsetup.sh so that everyone > > can rely on it confidently. > > I remember that we already had a similar discussion Xianzhu but I don't > remember > > the details exactly. What would prevent us from doing that? > > I think we need a universal and reliable way to figure out the build type > > regardless of the language the script is written in (Python vs Shell) and the > > environment the project is built in (buildbot vs developer workstation). > > > > Cheers, > > Philippe. > > Exporting variables is something we shouldn't be doing.. See Issue 142642 and > Issue 145755 for more context on the "why" here. > > Doing so for now seems fine, as I don't want you to be blocked on me, but if > there is an alternative solution that'd be preferred. Glad to see we have a bug tracking that. I also suggested the file in out/ approach in the past. I think this is a good idea too.
List of reviewers changed. wangxianzhu@chromium.org did a drive-by without LGTM'ing!
On 2012/09/04 12:05:40, I haz the power (commit-bot) wrote: > List of reviewers changed. mailto:wangxianzhu@chromium.org did a drive-by without > LGTM'ing! Ah I didn't expect that. The commit-bot does a good job :) I will try to remove you Xianzhu from the reviewers and CC you instead.
On 2012/09/04 12:09:05, Philippe wrote: > On 2012/09/04 12:05:40, I haz the power (commit-bot) wrote: > > List of reviewers changed. mailto:wangxianzhu@chromium.org did a drive-by > without > > LGTM'ing! > > Ah I didn't expect that. The commit-bot does a good job :) I will try to remove > you Xianzhu from the reviewers and CC you instead. I shouldn't even need to do that actually. Clicking on 'commit' without changing the reviewer list while the bot is working should do the trick.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10867008/26004
Change committed as 154751 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
