loman: Add support for adding elements from default.xml

loman: Add support for adding elements from default.xml

This is needed in order to do the right thing when adding projects
which require a revision attribute.

BUG=6811
TEST=Updated unittests and verified they pass.
Verified that I can add chromeos-kernel-next and revision gets set.

Change-Id: I12818dad464094cd50098170da3fcf38b1e32f17

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=ee49ac0

[kliegs, 2010-09-22 21:41:02]

http://codereview.chromium.org/3400019/diff/1/2
File bin/loman.py (right):

http://codereview.chromium.org/3400019/diff/1/2#newcode49
bin/loman.py:49: """Add a new workon project element to the manifest tree.
Is this function still adding a workon project? Or with workon being a parameter
is it capable of adding any project?

http://codereview.chromium.org/3400019/diff/1/2#newcode63
bin/loman.py:63: element.attrib['workon'] = 'True'
Should this be set based on the value of the workon parameter?

http://codereview.chromium.org/3400019/diff/1/2#newcode112
bin/loman.py:112: default='%s/manifests/default.xml' % repo_dir,
Should this default to .../.repo/manifest.xml instead?

http://codereview.chromium.org/3400019/diff/1/2#newcode124
bin/loman.py:124: path = args[1]
The path variable does not appear to be used.  It looks like you've made it an
optional parameter (changing to len(args) < 1 above) but if its not used at all
then removing it from the usage line and parsing here would make this cleaner.

If its still meant to be used then make sure to pass/set it appropriately.

[Mandeep Singh Baines, 2010-09-22 22:24:41]

Fixed. PTAL.

On 2010/09/22 21:41:02, kliegs wrote:
> http://codereview.chromium.org/3400019/diff/1/2
> File bin/loman.py (right):
> 
> http://codereview.chromium.org/3400019/diff/1/2#newcode49
> bin/loman.py:49: """Add a new workon project element to the manifest tree.
> Is this function still adding a workon project? Or with workon being a
parameter
> is it capable of adding any project?
> 

Fixed.

> http://codereview.chromium.org/3400019/diff/1/2#newcode63
> bin/loman.py:63: element.attrib['workon'] = 'True'
> Should this be set based on the value of the workon parameter?
> 

Fixed.

> http://codereview.chromium.org/3400019/diff/1/2#newcode112
> bin/loman.py:112: default='%s/manifests/default.xml' % repo_dir,
> Should this default to .../.repo/manifest.xml instead?
> 

No. default.xml contains all projects. We want to copy from there.

> http://codereview.chromium.org/3400019/diff/1/2#newcode124
> bin/loman.py:124: path = args[1]
> The path variable does not appear to be used.  It looks like you've made it an
> optional parameter (changing to len(args) < 1 above) but if its not used at
all
> then removing it from the usage line and parsing here would make this cleaner.
> 
> If its still meant to be used then make sure to pass/set it appropriately.

Fixed.

[Mandeep Singh Baines, 2010-09-23 17:06:46]

ping

[zbehan, 2010-09-23 18:02:19]

Communicated offline. You should add a very brief paragraph in the Description
of the commit what does it do, not just what it's needed for. Otherwise, LGTM.

On 2010/09/23 17:06:46, Mandeep Singh Baines wrote:
> ping

[kliegs, 2010-09-23 18:15:37]

Is the default symlink something we've committed to maintaining as a link to the
full manifest?  That seems counter-intuitive to me (but I'm not as familiar with
mid and long-term repo plans).    Would adding  a 'manifest-full.xml' link or
file make more sense?  I'm worried sometime in the next month its decided that
the minimal manifest makes sense as a default (it is whats in our instructions
for checkout after all) and the link gets changed which breaks loman.

One minor text nit otherwise.

http://codereview.chromium.org/3400019/diff/7001/8001
File bin/loman.py (right):

http://codereview.chromium.org/3400019/diff/7001/8001#newcode130
bin/loman.py:130: Die('Path name "%s" already exits with a different path.' %
name)
Should just be 'name "%s" ...

[anush, 2010-09-23 19:57:32]

On Thu, Sep 23, 2010 at 11:15 AM, <kliegs@chromium.org> wrote:

> Is the default symlink something we've committed to maintaining as a link
> to the
> full manifest?  That seems counter-intuitive to me (but I'm not as familiar
> with
> mid and long-term repo plans).    Would adding  a 'manifest-full.xml' link
> or
> file make more sense?  I'm worried sometime in the next month its decided
> that
> the minimal manifest makes sense as a default (it is whats in our
> instructions
> for checkout after all) and the link gets changed which breaks loman.
>
>
Long term we would want to switch to default.xml pointing to a
"newlayout.xml" which will reorganize the source tree a little bit, but
since loman parses the XML nodes only the path should change. I agree if we
do choose to make default point to minilayout then this would require a
change to loman. But I dont think there are any plans right now to switch
default to minilayout and that will anyway require much more in terms of
developer buy in etc than only CLs . LGTM w/ your nit below.

Thanks



> One minor text nit otherwise.
>
>
> http://codereview.chromium.org/3400019/diff/7001/8001
>
> File bin/loman.py (right):
>
> http://codereview.chromium.org/3400019/diff/7001/8001#newcode130
> bin/loman.py:130: Die('Path name "%s" already exits with a different
> path.' % name)
> Should just be 'name "%s" ...
>
>
> http://codereview.chromium.org/3400019/show
>

[kliegs, 2010-09-23 20:15:59]

(don't hold up the CL on this, but it might save confusion later)

Is it possible to just add a new symlink to the repo directory now?  So we
could have a 'manifest-all.xml' link that just points to default.xml?  this
way we don't need to update loman in the future?  If this is easy to do (on
the order of a couple hours max including testing) I'd like to see this
done.  Just have no clue on what it takes - haven't done any repo hacking.

It'd also help anyone who was developing a similar tool be able to easily
distinguish which manifests they should use (full vs. default vs. active)

-Jon

On Thu, Sep 23, 2010 at 3:57 PM, Anush Elangovan(அனுஷ்)
<anush@chromium.org>wrote:

> On Thu, Sep 23, 2010 at 11:15 AM, <kliegs@chromium.org> wrote:
>
>> Is the default symlink something we've committed to maintaining as a link
>> to the
>> full manifest?  That seems counter-intuitive to me (but I'm not as
>> familiar with
>> mid and long-term repo plans).    Would adding  a 'manifest-full.xml' link
>> or
>> file make more sense?  I'm worried sometime in the next month its decided
>> that
>> the minimal manifest makes sense as a default (it is whats in our
>> instructions
>> for checkout after all) and the link gets changed which breaks loman.
>>
>>
> Long term we would want to switch to default.xml pointing to a
> "newlayout.xml" which will reorganize the source tree a little bit, but
> since loman parses the XML nodes only the path should change. I agree if we
> do choose to make default point to minilayout then this would require a
> change to loman. But I dont think there are any plans right now to switch
> default to minilayout and that will anyway require much more in terms of
> developer buy in etc than only CLs . LGTM w/ your nit below.
>
> Thanks
>
>
>
>> One minor text nit otherwise.
>>
>>
>> http://codereview.chromium.org/3400019/diff/7001/8001
>>
>> File bin/loman.py (right):
>>
>> http://codereview.chromium.org/3400019/diff/7001/8001#newcode130
>> bin/loman.py:130: Die('Path name "%s" already exits with a different
>> path.' % name)
>> Should just be 'name "%s" ...
>>
>>
>> http://codereview.chromium.org/3400019/show
>>
>
>

[Mandeep Singh Baines, 2010-09-24 00:44:53]

Fixed. PTAL.

Reran unittests and verified that I can workon chromeos-kernel-next and the
manifest is correct.

On 2010/09/23 20:15:59, kliegs wrote:
> (don't hold up the CL on this, but it might save confusion later)
> 
> Is it possible to just add a new symlink to the repo directory now?  So we
> could have a 'manifest-all.xml' link that just points to default.xml?  this
> way we don't need to update loman in the future?  If this is easy to do (on
> the order of a couple hours max including testing) I'd like to see this
> done.  Just have no clue on what it takes - haven't done any repo hacking.
> 
> It'd also help anyone who was developing a similar tool be able to easily
> distinguish which manifests they should use (full vs. default vs. active)
> 
> -Jon
> 
> On Thu, Sep 23, 2010 at 3:57 PM, Anush Elangovan(அனுஷ்)
> <anush@chromium.org>wrote:
> 
> > On Thu, Sep 23, 2010 at 11:15 AM, <mailto:kliegs@chromium.org> wrote:
> >
> >> Is the default symlink something we've committed to maintaining as a link
> >> to the
> >> full manifest?  That seems counter-intuitive to me (but I'm not as
> >> familiar with
> >> mid and long-term repo plans).    Would adding  a 'manifest-full.xml' link
> >> or
> >> file make more sense?  I'm worried sometime in the next month its decided
> >> that
> >> the minimal manifest makes sense as a default (it is whats in our
> >> instructions
> >> for checkout after all) and the link gets changed which breaks loman.
> >>
> >>
> > Long term we would want to switch to default.xml pointing to a
> > "newlayout.xml" which will reorganize the source tree a little bit, but
> > since loman parses the XML nodes only the path should change. I agree if we
> > do choose to make default point to minilayout then this would require a
> > change to loman. But I dont think there are any plans right now to switch
> > default to minilayout and that will anyway require much more in terms of
> > developer buy in etc than only CLs . LGTM w/ your nit below.
> >
> > Thanks
> >
> >
> >
> >> One minor text nit otherwise.
> >>
> >>
> >> http://codereview.chromium.org/3400019/diff/7001/8001
> >>
> >> File bin/loman.py (right):
> >>
> >> http://codereview.chromium.org/3400019/diff/7001/8001#newcode130
> >> bin/loman.py:130: Die('Path name "%s" already exits with a different
> >> path.' % name)
> >> Should just be 'name "%s" ...
> >>
> >>
> >> http://codereview.chromium.org/3400019/show
> >>
> >
> >
>

[kliegs, 2010-09-24 15:04:29]

LGTM