Fixes depot_tools presubmit

Fixes depot_tools presubmit

basic.sh expects "work: None", but "work: " is printed instead, which breaks
presubmit.  This fixes it.

BUG=
R=dpranke@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208799

[Dirk Pranke, 2013-06-25 22:39:49]

lgtm

lgtm

[commit-bot: I haz the power, 2013-06-25 22:41:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hinoka@chromium.org/17763004/2001

[commit-bot: I haz the power, 2013-06-25 22:44:03]

Presubmit check for 17763004-2001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
push-basic.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/push-basic.sh returned
non-zero exit status 1 in /b/commit-queue/workdir/tools/depot_tools/tests
Setting up test upstream git repo...
Setting up test git repo...
TESTING: git-cl upload wants a server
TESTING: git-cl status has no issue
TESTING: upload succeeds (needs a server running on localhost)
TESTING: git-cl status now knows the issue
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--    
0
100    77    0     0  100    77      0   1403 --:--:-- --:--:-- --:--:--  1425
TESTING: Base URL contains branch name
TESTING: git-cl push ok
Using 50% similarity for rename/copy detection. Override with --similarity.
Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies
Running presubmit commit checks ...

Presubmit checks passed.
Description: 'foo-quux\n\nReview URL: http://localhost:8080/5629499534213120'
Closing issue (you may be prompted for your codereview password)...
TESTING: committed code has proper description
TESTING: issue no longer has a branch
FAILURE: issue no longer has a branch


basic.sh failed
Command /b/commit-queue/workdir/tools/depot_tools/tests/basic.sh returned
non-zero exit status 1 in /b/commit-queue/workdir/tools/depot_tools/tests
Setting up test SVN repo...
Setting up test git-svn repo...
TESTING: git-cl upload wants a server
TESTING: git-cl status has no issue
TESTING: upload succeeds (needs a server running on localhost)
TESTING: git-cl status now knows the issue
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--    
0
100    77    0     0  100    77      0   1613 --:--:-- --:--:-- --:--:--  1638
TESTING: git-cl dcommits ok
Using 50% similarity for rename/copy detection. Override with --similarity.
Loaded authentication cookies from /home/chrome-bot/.codereview_upload_cookies
Running presubmit commit checks ...

Presubmit checks passed.
Description: 'foo-quux\n\nReview URL: http://localhost:8080/5840605766746112'
Closing issue (you may be prompted for your codereview password)...
TESTING: dcommitted code has proper description
TESTING: issue no longer has a branch
FAILURE: issue no longer has a branch


Presubmit checks took 163.4s to calculate.

[Ryan Tseng, 2013-06-26 01:12:17]

I think presubmit failed again because test.sh calls "git cl <args>", which
actually points to the slave's copy of depot_tools/git-cl, and not the testing
copy of depot_tools.

Can you take a look again?

[iannucci, 2013-06-26 01:18:23]

https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py#newcode64
PRESUBMIT.py:64: input_api.environ['PATH'] = '%s:%s' %
(input_api.PresubmitLocalPath(),
use os.pathsep instead of ':'

[Dirk Pranke, 2013-06-26 01:18:44]

The downside to this patch is that you could add a bad presubmit.py check that
turns off a bunch of checks, and we wouldn't catch that.

I think it may be better to just manually deal w/ changes to PRESUBMIT.py itself
and not rely on the CQ, but I could be convinced otherwise.

Chase, WDYT?

https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py#newcode57
PRESUBMIT.py:57: print input_api.PresubmitLocalPath()
I don't think you actually want this print statement ...

[Ryan Tseng, 2013-06-26 01:29:06]

Review changes made.
So the issue was, 7 days ago, this CL was landed:
https://chromiumcodereview.appspot.com/17379008

And as you can tell, it actually did go through the CQ and passed presubmit even
though one of the files it changed broke presubmit.  Why?  Probably because the
CQ presubmit is called the wrong version of depot_tools/git-cl.

https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py#newcode57
PRESUBMIT.py:57: print input_api.PresubmitLocalPath()
On 2013/06/26 01:18:44, Dirk Pranke wrote:
> I don't think you actually want this print statement ...

Oops, fixed.

https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py#newcode64
PRESUBMIT.py:64: input_api.environ['PATH'] = '%s:%s' %
(input_api.PresubmitLocalPath(),
On 2013/06/26 01:18:23, iannucci wrote:
> use os.pathsep instead of ':'

Done.

[Dirk Pranke, 2013-06-26 01:34:37]

On 2013/06/26 01:29:06, Ryan T. wrote:
> Review changes made.
> So the issue was, 7 days ago, this CL was landed:
> https://chromiumcodereview.appspot.com/17379008
> 
> And as you can tell, it actually did go through the CQ and passed presubmit
even
> though one of the files it changed broke presubmit.  Why?  Probably because
the
> CQ presubmit is called the wrong version of depot_tools/git-cl.
> 

Yes, I understand the tradeoff and what you're attempting to fix. Unfortunately,
both choices have drawbacks.

Theoretically, you could change things to run *both* presubmits. That would make
life more complicated, though, so I'm not sure that would be worth it.

-- Dirk

[Ryan Tseng, 2013-06-26 22:09:44]

I'll dcommit the original patch and move the "wrong depot_tools" issue into
another CL

[hinoka, 2013-06-26 22:13:34]

Committed patchset #5 manually as r208799 (presubmit successful).

[Dan Beam, 2013-06-27 03:36:39]

On 2013/06/26 01:29:06, Ryan T. wrote:
> Review changes made.
> So the issue was, 7 days ago, this CL was landed:
> https://chromiumcodereview.appspot.com/17379008
> 
> And as you can tell, it actually did go through the CQ and passed presubmit
even
> though one of the files it changed broke presubmit.  Why?  Probably because
the
> CQ presubmit is called the wrong version of depot_tools/git-cl.

Sorry for the breakage, didn't know something depended on this :(.

-- Dan

> 
> https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py
> File PRESUBMIT.py (right):
> 
> https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py#newcode57
> PRESUBMIT.py:57: print input_api.PresubmitLocalPath()
> On 2013/06/26 01:18:44, Dirk Pranke wrote:
> > I don't think you actually want this print statement ...
> 
> Oops, fixed.
> 
> https://codereview.chromium.org/17763004/diff/7002/PRESUBMIT.py#newcode64
> PRESUBMIT.py:64: input_api.environ['PATH'] = '%s:%s' %
> (input_api.PresubmitLocalPath(),
> On 2013/06/26 01:18:23, iannucci wrote:
> > use os.pathsep instead of ':'
> 
> Done.