gclient: in managed mode, warn if .gclient has a mismatched URL

gclient: in managed mode, warn if .gclient has a mismatched URL

BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=258617

[borenet, 2014-03-11 22:03:42]

This is pretty hacky right now, but it's just a proof of concept.

[iannucci, 2014-03-12 20:06:04]

What about having the SCM object provide a remote-v-local api which can return
"OK", "mismatched URL", "mismatched SCM", ... ?

[borenet, 2014-03-12 20:23:43]

On 2014/03/12 20:06:04, iannucci wrote:
> What about having the SCM object provide a remote-v-local api which can return
> "OK", "mismatched URL", "mismatched SCM", ... ?

Sure, I can do that.  This first round was just for demonstration; if this is a
reasonable thing to do, I'll move forward.  I'm thinking IsValidScm or
CheckoutIsValid (eg, can I run "svn info ." or "git config --local") plus
DoesRemoteURLMatch?  Then I essentially replace the code I've written here with:

for dep in self.dependencies:
  if dep.managed:
    scm = gclient_scm.CreateScm(d.url ...)
    if not scm.IsValidScm() or not scm.DoesRemoteURLMatch():
      raise gclient_utils.Error(...)

[borenet, 2014-03-13 13:53:35]

Uploaded patch set 2.

https://codereview.chromium.org/195913002/diff/20001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/195913002/diff/20001/gclient_scm.py#newcode144
gclient_scm.py:144: """Attempt to determine the remote URL for this
SCMWrapper."""
I just copied the new code here, because GitWrapper or SvnWrapper both need to
have the option of falling back on the other's method of finding the remote URL,
in case the actually-checked-out code doesn't match the desired URL.

[iannucci, 2014-03-13 18:27:26]

yeah, I think making it a loud warning (maybe with an explicit "continue" action
from the user?) before making it an error seems sensible.

https://codereview.chromium.org/195913002/diff/20001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/195913002/diff/20001/gclient.py#newcode1234
gclient.py:1234: Actual:   %s
I would also list the SCM which we would have selected in either case.

https://codereview.chromium.org/195913002/diff/20001/gclient.py#newcode1237
gclient.py:1237: an svn repository, you probably want to set 'managed': False in
.gclient.
do we also want to advise that they correct the url to be git?

https://codereview.chromium.org/195913002/diff/20001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/195913002/diff/20001/gclient_scm.py#newcode144
gclient_scm.py:144: """Attempt to determine the remote URL for this
SCMWrapper."""
On 2014/03/13 13:53:35, borenet wrote:
> I just copied the new code here, because GitWrapper or SvnWrapper both need to
> have the option of falling back on the other's method of finding the remote
URL,
> in case the actually-checked-out code doesn't match the desired URL.

yeah, that seems reasonable to me.

[borenet, 2014-03-13 18:57:25]

Changed to be a warning instead.  I'm not sure I like the idea of waiting for
user input, but I tried to make the message obnoxious so that it wouldn't be
missed.  Here's the output I get when I have my .gclient set incorrectly:


$ gclient sync --nohooks

################################################################################
################################### WARNING! ###################################
################################################################################

Your .gclient file seems to be broken. The requested URL is different from what
is actually checked out in /path/to/checkout/src. In the future this will cause
a
failure.

Expected: https://src.chromium.org/svn/trunk/src (svn)
Actual:   https://chromium.googlesource.com/chromium/src.git (git)

You should ensure that the URL listed in .gclient is correct and either change
it or fix the checkout. If you're managing your own git checkout in
/path/to/checkout/src but the URL in .gclient is for an svn repository, you
probably
want to set 'managed': False in .gclient.

################################################################################
################################################################################
################################################################################

________ found .git directory; skipping src


.... sync continues normally.

https://codereview.chromium.org/195913002/diff/20001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/195913002/diff/20001/gclient.py#newcode1234
gclient.py:1234: Actual:   %s
On 2014/03/13 18:27:27, iannucci wrote:
> I would also list the SCM which we would have selected in either case.

Done.

https://codereview.chromium.org/195913002/diff/20001/gclient.py#newcode1237
gclient.py:1237: an svn repository, you probably want to set 'managed': False in
.gclient.
On 2014/03/13 18:27:27, iannucci wrote:
> do we also want to advise that they correct the url to be git?

Sure, but the URL doesn't matter in unmanaged mode.

[iannucci, 2014-03-13 21:54:22]

On 2014/03/13 18:57:25, borenet wrote:
> Changed to be a warning instead.  I'm not sure I like the idea of waiting for
> user input, but I tried to make the message obnoxious so that it wouldn't be
> missed.  Here's the output I get when I have my .gclient set incorrectly:
> 
> 
> $ gclient sync --nohooks
> 
>
################################################################################
> ################################### WARNING!
###################################
>
################################################################################
> 
> Your .gclient file seems to be broken. The requested URL is different from
what
> is actually checked out in /path/to/checkout/src. In the future this will
cause
> a
> failure.
> 
> Expected: https://src.chromium.org/svn/trunk/src (svn)
> Actual:   https://chromium.googlesource.com/chromium/src.git (git)
> 
> You should ensure that the URL listed in .gclient is correct and either change
> it or fix the checkout. If you're managing your own git checkout in
> /path/to/checkout/src but the URL in .gclient is for an svn repository, you
> probably
> want to set 'managed': False in .gclient.
> 
>
################################################################################
>
################################################################################
>
################################################################################

If we're not going to block, could we maybe display the warning at the very end?
I'm just worried about it zipping off the top of the screen.

> 
> ________ found .git directory; skipping src
> 
> 
> .... sync continues normally.
> 
> https://codereview.chromium.org/195913002/diff/20001/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/195913002/diff/20001/gclient.py#newcode1234
> gclient.py:1234: Actual:   %s
> On 2014/03/13 18:27:27, iannucci wrote:
> > I would also list the SCM which we would have selected in either case.
> 
> Done.
> 
> https://codereview.chromium.org/195913002/diff/20001/gclient.py#newcode1237
> gclient.py:1237: an svn repository, you probably want to set 'managed': False
in
> .gclient.
> On 2014/03/13 18:27:27, iannucci wrote:
> > do we also want to advise that they correct the url to be git?
> 
> Sure, but the URL doesn't matter in unmanaged mode.

[borenet, 2014-03-14 13:30:56]

Uploaded patch set 4.

https://codereview.chromium.org/195913002/diff/60001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/195913002/diff/60001/gclient.py#newcode81
gclient.py:81: import atexit
Is atexit acceptable for this? It would be nice to have a way to gather
warnings/errors as we run and dump them all out at the end. That'd be especially
good for https://codereview.chromium.org/189913020/, to warn more loudly that
we've moved directories around.

[borenet, 2014-03-17 16:54:20]

On 2014/03/14 13:30:56, borenet wrote:
> Uploaded patch set 4.
> 
> https://codereview.chromium.org/195913002/diff/60001/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/195913002/diff/60001/gclient.py#newcode81
> gclient.py:81: import atexit
> Is atexit acceptable for this? It would be nice to have a way to gather
> warnings/errors as we run and dump them all out at the end. That'd be
especially
> good for https://codereview.chromium.org/189913020/, to warn more loudly that
> we've moved directories around.

Friendly ping.

[iannucci, 2014-03-17 17:38:40]

https://codereview.chromium.org/195913002/diff/60001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/195913002/diff/60001/gclient.py#newcode81
gclient.py:81: import atexit
On 2014/03/14 13:30:57, borenet wrote:
> Is atexit acceptable for this? It would be nice to have a way to gather
> warnings/errors as we run and dump them all out at the end. That'd be
especially
> good for https://codereview.chromium.org/189913020/, to warn more loudly that
> we've moved directories around.

Hm. I would probably wrap atexit instead of directly registering callbacks, if
only because atexit works in LIFO order, which could be weird. Having a
print_on_exit(...) function could register a single atexit and then keep a queue
internally for messages to print.

[borenet, 2014-03-17 17:41:33]

https://codereview.chromium.org/195913002/diff/60001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/195913002/diff/60001/gclient.py#newcode81
gclient.py:81: import atexit
On 2014/03/17 17:38:40, iannucci wrote:
> On 2014/03/14 13:30:57, borenet wrote:
> > Is atexit acceptable for this? It would be nice to have a way to gather
> > warnings/errors as we run and dump them all out at the end. That'd be
> especially
> > good for https://codereview.chromium.org/189913020/, to warn more loudly
that
> > we've moved directories around.
> 
> Hm. I would probably wrap atexit instead of directly registering callbacks, if
> only because atexit works in LIFO order, which could be weird. Having a
> print_on_exit(...) function could register a single atexit and then keep a
queue
> internally for messages to print.

Alternatively, I can just call the _CheckConfig method right before finishing,
rather than at the beginning.

[iannucci, 2014-03-17 18:55:01]

On 2014/03/17 17:41:33, borenet wrote:
> https://codereview.chromium.org/195913002/diff/60001/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/195913002/diff/60001/gclient.py#newcode81
> gclient.py:81: import atexit
> On 2014/03/17 17:38:40, iannucci wrote:
> > On 2014/03/14 13:30:57, borenet wrote:
> > > Is atexit acceptable for this? It would be nice to have a way to gather
> > > warnings/errors as we run and dump them all out at the end. That'd be
> > especially
> > > good for https://codereview.chromium.org/189913020/, to warn more loudly
> that
> > > we've moved directories around.
> > 
> > Hm. I would probably wrap atexit instead of directly registering callbacks,
if
> > only because atexit works in LIFO order, which could be weird. Having a
> > print_on_exit(...) function could register a single atexit and then keep a
> queue
> > internally for messages to print.
> 
> Alternatively, I can just call the _CheckConfig method right before finishing,
> rather than at the beginning.

sgtm, just to make sure devs actually see it

[borenet, 2014-03-17 19:07:27]

On 2014/03/17 18:55:01, iannucci wrote:
> On 2014/03/17 17:41:33, borenet wrote:
> > https://codereview.chromium.org/195913002/diff/60001/gclient.py
> > File gclient.py (right):
> > 
> > https://codereview.chromium.org/195913002/diff/60001/gclient.py#newcode81
> > gclient.py:81: import atexit
> > On 2014/03/17 17:38:40, iannucci wrote:
> > > On 2014/03/14 13:30:57, borenet wrote:
> > > > Is atexit acceptable for this? It would be nice to have a way to gather
> > > > warnings/errors as we run and dump them all out at the end. That'd be
> > > especially
> > > > good for https://codereview.chromium.org/189913020/, to warn more loudly
> > that
> > > > we've moved directories around.
> > > 
> > > Hm. I would probably wrap atexit instead of directly registering
callbacks,
> if
> > > only because atexit works in LIFO order, which could be weird. Having a
> > > print_on_exit(...) function could register a single atexit and then keep a
> > queue
> > > internally for messages to print.
> > 
> > Alternatively, I can just call the _CheckConfig method right before
finishing,
> > rather than at the beginning.
> 
> sgtm, just to make sure devs actually see it

Done in patch set 5.

[iannucci, 2014-03-18 20:20:55]

lgtm :)

As mentioned offline, it would be good to have a general facility to batch
warnings to the end of the command. Right now they get swallowed when they're
printed inline during the run.

https://codereview.chromium.org/195913002/diff/80001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/195913002/diff/80001/gclient.py#newcode1048
gclient.py:1048: """Verify that the config matches any existing checkout."""
"""Verify that the .gclient file matches the state of the existing checked out
solutions."""

?

[borenet, 2014-03-18 20:30:08]

Agreed.  Per our discussion in chat, having a way to list all warnings at the
end would be helpful in https://codereview.chromium.org/189913020/ so that the
"I moved your checkout" warnings don't get lost in log spew.

https://codereview.chromium.org/195913002/diff/80001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/195913002/diff/80001/gclient.py#newcode1048
gclient.py:1048: """Verify that the config matches any existing checkout."""
On 2014/03/18 20:20:56, iannucci wrote:
> """Verify that the .gclient file matches the state of the existing checked out
> solutions."""
> 
> ?

Done.

[borenet, 2014-03-20 18:21:22]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-03-20 18:21:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/borenet@google.com/195913002/100001

[commit-bot: I haz the power, 2014-03-20 18:23:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:23:33]

Presubmit check for 195913002-100001 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 **
tests/gclient_smoketest.py (55.22s) failed
....F......F.FFF...........................
======================================================================
FAIL: testSolutionNone (__main__.GClientSmoke)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 274, in testSolutionNone
    self.check(('', '', 0), results)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
797, in check
    self.checkString(expected[1], results[1])
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
792, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: No SCM found for url None\n'

======================================================================
FAIL: testRest (__main__.GClientSmokeFromCheckout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1488, in testRest
    [('running', join(self.root_dir, 'foo', 'bar'))])
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
792, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: No SCM found for url None\n'

======================================================================
FAIL: testRevertAndStatus (__main__.GClientSmokeFromCheckout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1430, in testRevertAndStatus
    out = self.parseGclient(['status', '--deps', 'mac', '--jobs', '1'], [])
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
792, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: No SCM found for url None\n'

======================================================================
FAIL: testRunHooks (__main__.GClientSmokeFromCheckout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1455, in testRunHooks
    out = self.parseGclient(['runhooks', '--deps', 'mac'], ['running'])
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
792, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: No SCM found for url None\n'

======================================================================
FAIL: testSync (__main__.GClientSmokeFromCheckout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/gclient_smoketest.py", line 1418, in testSync
    ['running', 'running'])
  File "tests/gclient_smoketest.py", line 82, in parseGclient
    self.checkString(expected_stderr, stderr)
  File
"/b/commit-queue/workdir/tools/depot_tools/testing_support/fake_repos.py", line
792, in checkString
    self.assertEquals(expected, result, msg)
AssertionError: '' != 'Error: No SCM found for url None\n'

----------------------------------------------------------------------
Ran 43 tests in 54.959s

FAILED (failures=5)
Error: No SCM found for url None

Error: No SCM found for url None

Error: No SCM found for url None

Error: No SCM found for url None

Error: No SCM found for url None



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   1787 --:--:-- --:--:-- --:--:--  1833
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

Review URL: http://localhost:10000/5629499534213120
Closing issue (you may be prompted for your codereview password)...
Traceback (most recent call last):
  File "/b/commit-queue/workdir/tools/depot_tools/tests/../git_cl.py", line
2544, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/b/commit-queue/workdir/tools/depot_tools/tests/../git_cl.py", line
2530, in main
    return dispatcher.execute(OptionParser(), argv)
  File "/b/commit-queue/workdir/tools/depot_tools/subcommand.py", line 245, in
execute
    return command(parser, args[1:])
  File "/b/commit-queue/workdir/tools/depot_tools/tests/../git_cl.py", line
1988, in CMDpush
    return SendUpstream(parser, args, 'push')
  File "/b/commit-queue/workdir/tools/depot_tools/tests/../git_cl.py", line
1954, in SendUpstream
    cl.RpcServer().add_comment(cl.GetIssue(), comment)
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 255, in
add_comment
    ('no_redirect', 'True')])
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 385, in
post
    return self._send(request_path, payload=body, content_type=ctype, **kwargs)
  File "/b/commit-queue/workdir/tools/depot_tools/rietveld.py", line 411, in
_send
    result = self.rpc_server.Send(request_path, **kwargs)
  File "/b/commit-queue/workdir/tools/depot_tools/third_party/upload.py", line
463, in Send
    f = self.opener.open(req)
  File "/usr/lib/python2.7/urllib2.py", line 406, in open
    response = meth(req, response)
  File "/usr/lib/python2.7/urllib2.py", line 519, in http_response
    'http', request, response, code, msg, hdrs)
  File "/usr/lib/python2.7/urllib2.py", line 444, in error
    return self._call_chain(*args)
  File "/usr/lib/python2.7/urllib2.py", line 378, in _call_chain
    result = func(*args)
  File "/usr/lib/python2.7/urllib2.py", line 527, in http_error_default
    raise HTTPError(req.get_full_url(), code, msg, hdrs, fp)
urllib2.HTTPError: HTTP Error 403: Forbidden
FAILURE: git-cl push ok


Presubmit checks took 110.9s to calculate.

[borenet, 2014-03-20 18:52:29]

Uploaded patch set 6 to fix the smoketests, which failed because of the
following spec in testing_support.fake_repos:

    # WebKit abuses this.
    fs['trunk/webkit/.gclient'] = """
solutions = [
  {
    'name': './',
    'url': None,
  },
]
"""

For now, my "fix" just skips over specs like this, but it seems like it
shouldn't be valid, or should be marked as not managed.  I didn't mess with it
because I didn't know in what way "WebKit abuses this."

[iannucci, 2014-03-21 17:20:40]

lgtm

[borenet, 2014-03-21 17:22:27]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-03-21 17:22:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/borenet@google.com/195913002/120001

[commit-bot: I haz the power, 2014-03-21 17:24:50]

Change committed as 258617

[ahe, 2014-03-24 15:07:36]

https://codereview.chromium.org/195913002/diff/120001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/195913002/diff/120001/gclient_scm.py#newcode146
gclient_scm.py:146: return shlex.split(scm.GIT.Capture(
I have a layout where a Subversion directory is nested in a git repository. This
method incorrectly reports that the Subversion directory is a git repository.
See https://code.google.com/p/dart/issues/detail?id=17731

I think that if you do the Subversion check first, it would work.

[borenet, 2014-03-24 15:08:48]

On 2014/03/24 15:07:36, ahe wrote:
> https://codereview.chromium.org/195913002/diff/120001/gclient_scm.py
> File gclient_scm.py (right):
> 
>
https://codereview.chromium.org/195913002/diff/120001/gclient_scm.py#newcode146
> gclient_scm.py:146: return shlex.split(scm.GIT.Capture(
> I have a layout where a Subversion directory is nested in a git repository.
This
> method incorrectly reports that the Subversion directory is a git repository.
> See https://code.google.com/p/dart/issues/detail?id=17731
> 
> I think that if you do the Subversion check first, it would work.

Yes - someone else reported this as well.  The fix is uploaded for review here:
https://codereview.chromium.org/209963002/

[ahe, 2014-03-24 15:13:22]

On 2014/03/24 15:08:48, borenet wrote:
> Yes - someone else reported this as well.  The fix is uploaded for review
here:
> https://codereview.chromium.org/209963002/

Awesome! I'll ignore the warning for now.