Add a --unmanaged flag to gclient config to allow the main solution to be unmanaged by the scm.

Add a --unmanaged flag to gclient config to allow the main solution to be unmanaged by the scm.

The dependencies will continue to be managed. A new flag is present in the .gclient file to control this behavior.

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

[cmp, 2011-09-16 18:46:37]

Here's http://codereview.chromium.org/7777002/, updated against latest
depot_tools.  No changes from Nicolas's code except for a modified gclient_test
so tests keep working.  I verified presubmit tests pass (I don't have python2.5,
though).

Spoke with M-A offline.  Tried 'src': None and the other attempted solution
(avoid CreateSCM with no gclient_scm change), both caused gclient to not parse
the DEPS for src, so they were unworkable.  This approach works well and is
minimally invasive, and I'm convinced of the flag's utility.  Please take a look
and let me know what you think.

[nsylvain, 2011-09-16 18:51:53]

LGTM, nice looking code.

[M-A Ruel, 2011-09-16 18:58:23]

You should have at least a test that confirms that a scm will checkout but not
update an unmanaged object generated with CreateSCM().

http://codereview.chromium.org/7918027/diff/1003/gclient.py
File gclient.py (right):

http://codereview.chromium.org/7918027/diff/1003/gclient.py#newcode1069
gclient.py:1069: client.SetDefaultConfig(name, deps_file, base_url,
safesync_url, managed)
s/managed/not options.unmanaged/ and replace lines 1064-1065.

http://codereview.chromium.org/7918027/diff/1003/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/7918027/diff/1003/gclient_scm.py#newcode226
gclient_scm.py:226: print ('________ unmanaged solution; skipping %s' %
self.relpath)
It should be described somewhere that unmanaged deps will be checked out but
never synced.

[Dirk Pranke, 2011-09-16 19:06:34]

Change looks good. As an aside, I've found in practice that I am often flipping
between "chrome" branches that are managed, "chrome+webkit branches" that don't
track webkit in DEPS, and dev branches that are branched off of one of those two
options.

In this case I've found 'gclient sync -r src@HEAD' when not on a managed branch
works fine. Granted, I have to remember to pass the right argument depending on
the branch, and I need to write a script to automate this ...

Since I think I was one of the people first asking about this, I thought I'd
pass this along insofar as I'm not likely to remember to use this switch. It
does seem useful, though.

http://codereview.chromium.org/7918027/diff/1003/gclient.py
File gclient.py (right):

http://codereview.chromium.org/7918027/diff/1003/gclient.py#newcode395
gclient.py:395: None, self.deps_file, should_process))
style nit ... at some point w/ all these None arguments it'd be good to start
using keyword names.

http://codereview.chromium.org/7918027/diff/1003/gclient.py#newcode1065
gclient.py:1065: managed = False
Why not just use default=False above when you define the option? Same could be
done for options.git_deps.

http://codereview.chromium.org/7918027/diff/1003/gclient.py#newcode1069
gclient.py:1069: client.SetDefaultConfig(name, deps_file, base_url,
safesync_url, managed)
On 2011/09/16 18:58:23, Marc-Antoine Ruel wrote:
> s/managed/not options.unmanaged/ and replace lines 1064-1065.

Actually, I would specify default=False and just delete 1063-1065 (but also use
not options.unmanaged).

[cmp, 2011-09-20 01:12:28]

I added two unit tests to gclient_scm_test.py to track unmanaged Git checkouts.

http://codereview.chromium.org/7918027/diff/1003/gclient.py
File gclient.py (right):

http://codereview.chromium.org/7918027/diff/1003/gclient.py#newcode395
gclient.py:395: None, self.deps_file, should_process))
I agree, yet I don't want to make that change here while adding this
functionality.  Will look into a follow-up CL that makes that change if M-A
agrees.

http://codereview.chromium.org/7918027/diff/1003/gclient.py#newcode1065
gclient.py:1065: managed = False
On 2011/09/16 19:06:35, Dirk Pranke wrote:
> Why not just use default=False above when you define the option?

Good idea, done.

> Same could be done for options.git_deps.

options.git_deps is a different situation, so I don't plan to change it.

http://codereview.chromium.org/7918027/diff/1003/gclient.py#newcode1069
gclient.py:1069: client.SetDefaultConfig(name, deps_file, base_url,
safesync_url, managed)
On 2011/09/16 18:58:23, Marc-Antoine Ruel wrote:
> s/managed/not options.unmanaged/ and replace lines 1064-1065.

I implemented Dirk's suggestion (which is like this).

http://codereview.chromium.org/7918027/diff/1003/gclient.py#newcode1069
gclient.py:1069: client.SetDefaultConfig(name, deps_file, base_url,
safesync_url, managed)
On 2011/09/16 19:06:35, Dirk Pranke wrote:
> On 2011/09/16 18:58:23, Marc-Antoine Ruel wrote:
> > s/managed/not options.unmanaged/ and replace lines 1064-1065.
> 
> Actually, I would specify default=False and just delete 1063-1065 (but also
use
> not options.unmanaged).

Done.

http://codereview.chromium.org/7918027/diff/1003/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/7918027/diff/1003/gclient_scm.py#newcode226
gclient_scm.py:226: print ('________ unmanaged solution; skipping %s' %
self.relpath)
Done, updated unmanaged text in gclient config --help.

[nsylvain, 2011-09-20 16:47:01]

http://codereview.chromium.org/7918027/diff/8005/tests/gclient_scm_test.py
File tests/gclient_scm_test.py (right):

http://codereview.chromium.org/7918027/diff/8005/tests/gclient_scm_test.py#ne...
tests/gclient_scm_test.py:868: self.assertTrue(out in (msg1, msg2), (out, msg1,
msg2))
i dont think i understand this. Can you explain what this does?  And the 2 git
clones above, are they the root and a deps?

[cmp, 2011-09-20 17:15:37]

http://codereview.chromium.org/7918027/diff/8005/tests/gclient_scm_test.py
File tests/gclient_scm_test.py (right):

http://codereview.chromium.org/7918027/diff/8005/tests/gclient_scm_test.py#ne...
tests/gclient_scm_test.py:868: self.assertTrue(out in (msg1, msg2), (out, msg1,
msg2))
On 2011/09/20 16:47:02, nsylvain wrote:
> i dont think i understand this. Can you explain what
> this does?  And the 2 git clones above, are they the
> root and a deps?

First note that these methods (testUpdateCheckout and testUpdateUpdate) are
taken from the ManagedGitWrapperTestCase class above.  The only difference is
that options.revision is set on them.

I hate to duplicate the code.  I can move both of these methods out of here and
ManagedGit... into the Base, then set a class variable that toggles whether or
not it's testing a managed repo.

Re: what line 868 does, assertTrue verifies that all of its arguments are true. 
So in this case one of msg1 or msg2 should be printed to stdout, and all three
should be true (not None or null).  I believe this is necessary because there
may not be a guarantee about the test method ordering and its possible different
messages are printed depending on which ordering of method executions occurs.

[nsylvain, 2011-09-20 17:24:31]

LGTM

[cmp, 2011-09-20 17:30:08]

On 2011/09/20 17:15:37, cmp wrote:
> I hate to duplicate the code.  I can move both of these methods
> out of here and  ManagedGit... into the Base, then set a class
> variable that toggles whether or not it's testing a managed repo.

Still prefer not to duplicate the code, but my suggestion doesn't work as easily
as I thought it would.  The solution seems worse than the current patch, so I'll
leave it as-is.

[M-A Ruel, 2011-09-20 17:39:34]

http://codereview.chromium.org/7918027/diff/8005/gclient.py
File gclient.py (right):

http://codereview.chromium.org/7918027/diff/8005/gclient.py#newcode167
gclient.py:167: self.should_process = should_process
It's sad that 'managed' has a meaning so near to 'should_process' but in a
slightly different way. Please document the difference.

http://codereview.chromium.org/7918027/diff/8005/gclient.py#newcode677
gclient.py:677: Dependency.__init__(self, None, None, None, None, None, None,
None,
True instead of None?

http://codereview.chromium.org/7918027/diff/8005/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/7918027/diff/8005/gclient_scm.py#newcode181
gclient_scm.py:181: if revision == 'unmanaged':
remove 2 indents.

[cmp, 2011-09-20 18:52:13]

http://codereview.chromium.org/7918027/diff/8005/gclient.py
File gclient.py (right):

http://codereview.chromium.org/7918027/diff/8005/gclient.py#newcode167
gclient.py:167: self.should_process = should_process
On 2011/09/20 17:39:34, Marc-Antoine Ruel wrote:
> It's sad that 'managed' has a meaning so near to 'should_process' but in a
> slightly different way. Please document the difference.

Done.

http://codereview.chromium.org/7918027/diff/8005/gclient.py#newcode677
gclient.py:677: Dependency.__init__(self, None, None, None, None, None, None,
None,
On 2011/09/20 17:39:34, Marc-Antoine Ruel wrote:
> True instead of None?

Done.

http://codereview.chromium.org/7918027/diff/8005/gclient_scm.py
File gclient_scm.py (right):

http://codereview.chromium.org/7918027/diff/8005/gclient_scm.py#newcode181
gclient_scm.py:181: if revision == 'unmanaged':
On 2011/09/20 17:39:34, Marc-Antoine Ruel wrote:
> remove 2 indents.

Done.

[M-A Ruel, 2011-09-20 19:15:46]

lgtm

[cmp, 2011-09-20 20:00:09]

Added gclient_smoketest.py fixes so those presubmit tests pass.

[M-A Ruel, 2011-09-20 20:11:00]

lgtm

[commit-bot: I haz the power, 2011-09-20 20:17:52]

Change committed as 102002