loman: tool for updating local_manifest (initial commit)

loman: tool for updating local_manifest (initial commit)

BUG=5787
TEST=Ran unittests.

Change-Id: I6b8cc37eb2bf5f81f803f61b886106b6f08f3315

[sosa, 2010-09-03 20:51:28]

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

http://codereview.chromium.org/3331013/diff/1/2#newcode15
bin/loman.py:15: def _FindRepoDir():
Is this a copy from repo.py?

http://codereview.chromium.org/3331013/diff/1/2#newcode42
bin/loman.py:42: def _AddProject(self, name, path, workon='False'):
Are you planning on making callers with workon=False?

http://codereview.chromium.org/3331013/diff/1/2#newcode56
bin/loman.py:56: help='Is this a workon package?')
Probably wanna set a default here for the boolean workon

http://codereview.chromium.org/3331013/diff/1/2#newcode69
bin/loman.py:69: print "Unable to find repo dir."
Nit:  I suggest importing cros_build_lib and using Info / Warning / Die similiar
to our bash scripts.

http://codereview.chromium.org/3331013/diff/1/2#newcode78
bin/loman.py:78: print >> sys.stderr, \
See nit above (would use die)

[sosa, 2010-09-03 20:54:49]

Missed your unittests.

Also, you should add a symlink of bin/loman -> bin/loman.py

http://codereview.chromium.org/3331013/diff/1/3
File bin/loman_unittest.py (right):

http://codereview.chromium.org/3331013/diff/1/3#newcode25
bin/loman_unittest.py:25: ptree.Parse()
Shouldn't you assert the no argument manifest?

[Mandeep Singh Baines, 2010-09-07 23:57:33]

Fixed. PTAL.

On 2010/09/03 20:51:28, sosa wrote:
> http://codereview.chromium.org/3331013/diff/1/2
> File bin/loman.py (right):
> 
> http://codereview.chromium.org/3331013/diff/1/2#newcode15
> bin/loman.py:15: def _FindRepoDir():
> Is this a copy from repo.py?
> 

No. What is repo.py?

> http://codereview.chromium.org/3331013/diff/1/2#newcode42
> bin/loman.py:42: def _AddProject(self, name, path, workon='False'):
> Are you planning on making callers with workon=False?
> 

At some point.

> http://codereview.chromium.org/3331013/diff/1/2#newcode56
> bin/loman.py:56: help='Is this a workon package?')
> Probably wanna set a default here for the boolean workon
> 

Done.

> http://codereview.chromium.org/3331013/diff/1/2#newcode69
> bin/loman.py:69: print "Unable to find repo dir."
> Nit:  I suggest importing cros_build_lib and using Info / Warning / Die
similiar
> to our bash scripts.
> 

Done.

> http://codereview.chromium.org/3331013/diff/1/2#newcode78
> bin/loman.py:78: print >> sys.stderr, \
> See nit above (would use die)

Done.

[Mandeep Singh Baines, 2010-09-08 22:49:46]

ping

[sosa, 2010-09-08 23:36:23]

Nits, o/w LGTM

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

http://codereview.chromium.org/3331013/diff/1/2#newcode15
bin/loman.py:15: def _FindRepoDir():
On 2010/09/03 20:51:28, sosa wrote:
> Is this a copy from repo.py?

I meant from the python code that is repo.

http://codereview.chromium.org/3331013/diff/7001/8002#newcode25
bin/loman.py:25: """Class which provides an abstraction for manipulating the
local manifest"""
nit: two lines between top-level defs

http://codereview.chromium.org/3331013/diff/7001/8002#newcode27
bin/loman.py:27: def __init__(self, text=None):
All your docstrings should end with a period

http://codereview.chromium.org/3331013/diff/7001/8002#newcode37
bin/loman.py:37: if project.attrib['path'] == path or project.attrib['name'] ==
name:
Since this is a public function, document the return (though it's always
returning true)

[kliegs, 2010-09-09 14:54:42]

LGTM with nits/comments

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

http://codereview.chromium.org/3331013/diff/7001/8002#newcode29
bin/loman.py:29: def __init__(self, text=None):
Probably not worth fixing for here, but it seems if there is no text leaving
text as None is the right thing to do.  And then have Parse check for None on
self._text and create the tree using XML constructors rather than a handroll of
<manifest>\n</manifest>.

http://codereview.chromium.org/3331013/diff/7001/8002#newcode44
bin/loman.py:44: def _AddProject(self, name, path, workon='False'):
Instead of always adding to the root element should it confirm that its adding
under a manifest tag?  If the xml doc gets extended in the future this would
keep the script working.

http://codereview.chromium.org/3331013/diff/7001/8002#newcode83
bin/loman.py:83: print >> open(local_manifest, 'w'), ptree.ToString()
Is it worth checking for an error on the write?  Python will likely stack trace
its own error but a more friendly/specific one might be useful.

[Mandeep Singh Baines, 2010-09-09 21:49:27]

Fixed. PTAL.

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

http://codereview.chromium.org/3331013/diff/7001/8002#newcode25
bin/loman.py:25: 
On 2010/09/08 23:36:23, sosa wrote:
> nit: two lines between top-level defs

Fixed.

http://codereview.chromium.org/3331013/diff/7001/8002#newcode27
bin/loman.py:27: """Class which provides an abstraction for manipulating the
local manifest"""
On 2010/09/08 23:36:23, sosa wrote:
> All your docstrings should end with a period

Fixed.

http://codereview.chromium.org/3331013/diff/7001/8002#newcode29
bin/loman.py:29: def __init__(self, text=None):
On 2010/09/09 14:54:42, kliegs wrote:
> Probably not worth fixing for here, but it seems if there is no text leaving
> text as None is the right thing to do.  And then have Parse check for None on
> self._text and create the tree using XML constructors rather than a handroll
of
> <manifest>\n</manifest>.

I'd rather have more common code. What you suggest seems more complex.

http://codereview.chromium.org/3331013/diff/7001/8002#newcode37
bin/loman.py:37: """Add a new workon project if it is not already in the
manifest."""
On 2010/09/08 23:36:23, sosa wrote:
> Since this is a public function, document the return (though it's always
> returning true)

Fixed.

http://codereview.chromium.org/3331013/diff/7001/8002#newcode44
bin/loman.py:44: def _AddProject(self, name, path, workon='False'):
On 2010/09/09 14:54:42, kliegs wrote:
> Instead of always adding to the root element should it confirm that its adding
> under a manifest tag?  If the xml doc gets extended in the future this would
> keep the script working.

Not sure I follow. If the DTD gets extended what you are suggesting MAY result
in this script still working. On the other hand, the current implementation MAY
also result in the script still working. It all depends on how the DTD gets
extended. The DTD is documented here:

http://android.git.kernel.org/?p=tools/repo.git;a=blob;f=docs/manifest_xml.tx...

http://codereview.chromium.org/3331013/diff/7001/8002#newcode83
bin/loman.py:83: print >> open(local_manifest, 'w'), ptree.ToString()
On 2010/09/09 14:54:42, kliegs wrote:
> Is it worth checking for an error on the write?  Python will likely stack
trace
> its own error but a more friendly/specific one might be useful.

Fixed.