Issue 10942098: Make sure subprocess calls don`t ignore result codes indicating errors. Fix JS docs generation whil… (Closed)
|
Created:
July 4, 2013, 11:23 a.m. by Wladimir Palant Modified:
July 8, 2013, 9:11 a.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionMake sure subprocess calls don`t ignore result codes indicating errors. Fix JS docs generation whil…
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed wrong argument format #
Total comments: 12
Patch Set 3 : Addressed issues #
Total comments: 1
Patch Set 4 : #MessagesTotal messages: 8
http://codereview.adblockplus.org/10942098/diff/1/sitescripts/extensions/bin/... File sitescripts/extensions/bin/createNightlies.py (right): http://codereview.adblockplus.org/10942098/diff/1/sitescripts/extensions/bin/... sitescripts/extensions/bin/createNightlies.py:353: subprocess.check_call(command) If you actually don't have to check the return code, you should use subprocess.call() instead. http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/extensions/b... File sitescripts/extensions/bin/createNightlies.py (right): http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/extensions/b... sitescripts/extensions/bin/createNightlies.py:268: except ConfigParser.NoOptionError: Will still with NoSectionError, when the section 'extensions' doesn't exist at all. http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/extensions/b... sitescripts/extensions/bin/createNightlies.py:271: buildCommand += map(pipes.quote, ['/home/android/bin/makedebugbuild.py', '--revision', self.revision, '--version', self.version, '--stdout']) It would be better to use, .extend() instead of +=. The former will inject additional elements to the original list, in place. While the latter (as used here) will copy the list, which is unnecessary. Also, the proper and platform-independent way to build that path would be: os.path.join(os.path.expanduser('~android'), 'bin' 'makedebugbuild.py') http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/extensions/b... File sitescripts/extensions/bin/updateDownloadLinks.py (right): http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/extensions/b... sitescripts/extensions/bin/updateDownloadLinks.py:196: except: It would be better to check whether the platform-specific metadata file exists instead of catching every possible exception. There might be other reasons why the command fails, and I would rather not silently ignore them. http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... File sitescripts/subscriptions/bin/updateSubscriptionDownloads.py (right): http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... sitescripts/subscriptions/bin/updateSubscriptionDownloads.py:39: subprocess.check_call(['rsync', '-a', '--delete', destDir + '/', destTemp]) There is no need to hard-code path separators. Use os.path.sep instead (you might also want to have a look at os.path.join). http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... sitescripts/subscriptions/bin/updateSubscriptionDownloads.py:41: subprocess.check_call(['rsync', '-au', '--delete', destTemp + '/', destDir]) See above. http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... File sitescripts/subscriptions/bin/updateSubscriptionDownloadsCVS.py (right): http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... sitescripts/subscriptions/bin/updateSubscriptionDownloadsCVS.py:32: result = subprocess.check_output(['rsync', '-a', '--delete', '--out-format=%o %n', '--exclude=CVS', source + '/', dest]) There is no need to hard-code path separators. Use os.path.sep instead (you might also want to have a look at os.path.join). http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... File sitescripts/subscriptions/knownIssuesParser.py (right): http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... sitescripts/subscriptions/knownIssuesParser.py:153: data = subprocess.check_output(['hg', '-q', '-R', repoPath, 'cat', '-r', 'default', os.path.join(repoPath, 'knownIssues')]) The added -q parameter doesn't seem to have any affect, because of neither the ouput of the content of the given file, nor error messages are affected by hg's -q parameter. http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... File sitescripts/subscriptions/subscriptionParser.py (right): http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... sitescripts/subscriptions/subscriptionParser.py:228: settingsData = subprocess.check_output(['hg', '-q', '-R', repo, 'cat', '-r', 'default', os.path.join(repo, 'settings')]) The added -q parameter doesn't seem to have any affect, because of neither the ouput of the content of the given file, nor error messages are affected by hg's -q parameter.
http://codereview.adblockplus.org/10942098/diff/1/sitescripts/extensions/bin/... File sitescripts/extensions/bin/createNightlies.py (right): http://codereview.adblockplus.org/10942098/diff/1/sitescripts/extensions/bin/... sitescripts/extensions/bin/createNightlies.py:353: subprocess.check_call(command) Ignore that one. I thought I had discarded this comment, bebore publishing.
All comments should be addressed unless otherwise noted. I also switched to a cleaner approach whenever we need to commit and push a change - we check the result of all calls but run "hg stat" first to verify that a commit is necessary. http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/extensions/b... File sitescripts/extensions/bin/createNightlies.py (right): http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/extensions/b... sitescripts/extensions/bin/createNightlies.py:268: except ConfigParser.NoOptionError: On 2013/07/04 13:57:51, sebastian wrote: > Will still with NoSectionError, when the section 'extensions' doesn't exist at > all. Given that androidBuildHost is mandatory in the extensions section (we'll error out below otherwise) that should be fine. Not that I'm completely happy with how we deal with missing settings... http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/extensions/b... sitescripts/extensions/bin/createNightlies.py:271: buildCommand += map(pipes.quote, ['/home/android/bin/makedebugbuild.py', '--revision', self.revision, '--version', self.version, '--stdout']) Good point about list.extend(), changed that. os.path.expanduser() won't work because that user doesn't exist on the current machine - we are passing that command to SSH. I'm not really happy with the way Andrey hardcoded the command here but then again, I guess it does the job. http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... File sitescripts/subscriptions/bin/updateSubscriptionDownloads.py (right): http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... sitescripts/subscriptions/bin/updateSubscriptionDownloads.py:39: subprocess.check_call(['rsync', '-a', '--delete', destDir + '/', destTemp]) On 2013/07/04 13:57:51, sebastian wrote: > There is no need to hard-code path separators. Use os.path.sep instead (you > might also want to have a look at os.path.join). os.path.join() won't help here - rsync requires that the source path ends with a slash, otherwise it won't work correctly. But - yes, using os.path.sep for that is probably cleaner. http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... File sitescripts/subscriptions/knownIssuesParser.py (right): http://codereview.adblockplus.org/10942098/diff/3001/sitescripts/subscription... sitescripts/subscriptions/knownIssuesParser.py:153: data = subprocess.check_output(['hg', '-q', '-R', repoPath, 'cat', '-r', 'default', os.path.join(repoPath, 'knownIssues')]) On 2013/07/04 13:57:51, sebastian wrote: > The added -q parameter doesn't seem to have any affect, because of neither the > ouput of the content of the given file, nor error messages are affected by hg's > -q parameter. Oops, I was a bit over-eager adding -q to all hg commands - check_output() calls obviously don't need it.
http://codereview.adblockplus.org/10942098/diff/14001/sitescripts/subscriptio... File sitescripts/subscriptions/bin/updateMalwareDomainsList.py (right): http://codereview.adblockplus.org/10942098/diff/14001/sitescripts/subscriptio... sitescripts/subscriptions/bin/updateMalwareDomainsList.py:52: subprocess.call(['hg', '-q', 'push', '-R', tempdir]) If I understand it correctly, you check with hg stat whether there is something to push. So you don't have to silently ignore failure of hg push, anymore.
lgtm 
|
