[NaCl SDK] Refactor sdk_update*.

[NaCl SDK] Refactor sdk_update*.

Still need to add tests and documentation, but this CL is getting large already.

BUG=156766
R=sbc@chromium.org
NOTRY=true

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

[Sam Clegg, 2012-10-22 22:55:30]

Is it worth avoiding the conflict with the builtin commands.py module?  I'm not
sure ...

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/build_updater.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/build_updater.py:45:
('build_tools/sdk_tools/third_party/fancy_urllib/*.py',
I guess there is a tradeoff here between listing explicitly
exactly what goes in the package, and the cost of maintaining the list.

I guess the risk to random other .py files being here at
package build time is close to zero?

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/sdk_tools/commands/__init__.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/commands/__init__.py:10: from update
import *
Are wild card imports ok in __init__.py files?

I find them scary in all cases.

How many things to do you really want to pull in here?

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/sdk_tools/sdk_update_common.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_common.py:26: 
I'm still not completely sold on this pattern.

I prefer an early return if os.path.exists() but that is
really a style thing I guess.

The argument I suppose is that there could be another processes racing with up. 
 However, if some other process
was to create the folder between the two calls I'd argue that its better for
this program to just crash here.  i.e. externally messing with the tree while
this program is not worth defending against.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:28: import
manifest_util
Why not just always add to sys.path and avoid the try block?

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:39: import cygtar
Yuck.. is this so the script can work in SCM as well as when packaged?

Could the import not happen at the top of commands/__init__.py?

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:103: raise
Error('Json encoding error writing config "%s".\n  %s' % (path, e))
Maybe use two different try blocks here.   Generate json first, then write it to
a file.

Is it worth catching IOError here?  Is it likely that the file is not writable?

We should probably always be using the the "with open() as.." pattern for file
I/O.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:138:
logging.error('Error encoding manifest "%s" to JSON.\n  %s' % (path, e))
same as above.

Why are you logging here rather than raising Error for the
user to see?

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:265: print
"Native Client SDK Updater, version r%s" % (REVISION,)
no need to pass tuple here.   Just "% REVISION"

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:279: return
getattr(sys.modules[__name__], 'CMD' + name, None)
Hmm... I think globals() might be nicer here.  I really don't like using
sys.modules like this.. but I'm open to change.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:370:
logging.error(str(e))
Does this basically just print the error?  Or does it decorate in some way?

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/sdk_tools/third_party/fancy_urllib/README
(right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/third_party/fancy_urllib/README:36:
+
Maybe better just to add "sdk_tools/third_party" to the list of excludes for the
presubmit?

We don't want to be be making third party code lint free right?

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/tests/test_sdktools.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/tests/test_sdktools.py:117: cmd.extend(['-U',
self.server.GetURL(MANIFEST_BASENAME)])
So does '-U' first no longer work?

[binji, 2012-10-23 00:14:09]

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/build_updater.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/build_updater.py:45:
('build_tools/sdk_tools/third_party/fancy_urllib/*.py',
On 2012/10/22 22:55:30, Sam Clegg wrote:
> I guess there is a tradeoff here between listing explicitly
> exactly what goes in the package, and the cost of maintaining the list.
> 
> I guess the risk to random other .py files being here at
> package build time is close to zero?

Yes, I think this is clearer as well -- and less tedious to modify. Ultimately,
most of the stuff comes from the sdk_tools/ directory -- we should try to keep
cruft out of it.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/sdk_tools/commands/__init__.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/commands/__init__.py:10: from update
import *
On 2012/10/22 22:55:30, Sam Clegg wrote:
> Are wild card imports ok in __init__.py files?
> 
> I find them scary in all cases.
> 
> How many things to do you really want to pull in here?

Done.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/sdk_tools/sdk_update_common.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_common.py:26: 
On 2012/10/22 22:55:30, Sam Clegg wrote:
> I'm still not completely sold on this pattern.
> 
> I prefer an early return if os.path.exists() but that is
> really a style thing I guess.
> 
> The argument I suppose is that there could be another processes racing with
up. 
>  However, if some other process
> was to create the folder between the two calls I'd argue that its better for
> this program to just crash here.  i.e. externally messing with the tree while
> this program is not worth defending against.

Fair enough, we use os.path.exists elsewhere -- may as well be consistent.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:28: import
manifest_util
On 2012/10/22 22:55:30, Sam Clegg wrote:
> Why not just always add to sys.path and avoid the try block?

Done.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:39: import cygtar
On 2012/10/22 22:55:30, Sam Clegg wrote:
> Yuck.. is this so the script can work in SCM as well as when packaged?
> 
> Could the import not happen at the top of commands/__init__.py?

It seems that only update is using cygtar anyway, I just moved the import there.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:103: raise
Error('Json encoding error writing config "%s".\n  %s' % (path, e))
On 2012/10/22 22:55:30, Sam Clegg wrote:
> Maybe use two different try blocks here.   Generate json first, then write it
to
> a file.
> 
> Is it worth catching IOError here?  Is it likely that the file is not
writable?
> 
> We should probably always be using the the "with open() as.." pattern for file
> I/O.

Done.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:138:
logging.error('Error encoding manifest "%s" to JSON.\n  %s' % (path, e))
On 2012/10/22 22:55:30, Sam Clegg wrote:
> same as above.
> 
> Why are you logging here rather than raising Error for the
> user to see?

Ah, it was because I was calling this from the finally block, but when this
function raised, it was killing the original raiser -- the better solution is to
handle that below.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:265: print
"Native Client SDK Updater, version r%s" % (REVISION,)
On 2012/10/22 22:55:30, Sam Clegg wrote:
> no need to pass tuple here.   Just "% REVISION"

It's habit for me, I always make sure to pass a tuple to % for one element,
because if that element is a tuple, it will be expanded. But yes, not necessary
here.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:279: return
getattr(sys.modules[__name__], 'CMD' + name, None)
On 2012/10/22 22:55:30, Sam Clegg wrote:
> Hmm... I think globals() might be nicer here.  I really don't like using
> sys.modules like this.. but I'm open to change.

Haha, this is stolen from git_cl.py...
Done.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:370:
logging.error(str(e))
On 2012/10/22 22:55:30, Sam Clegg wrote:
> Does this basically just print the error?  Or does it decorate in some way?

it adds 'ERROR:', see fmt = '%(levelname)s:%(message)s' above.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/sdk_tools/third_party/fancy_urllib/README
(right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/third_party/fancy_urllib/README:36:
+
On 2012/10/22 22:55:30, Sam Clegg wrote:
> Maybe better just to add "sdk_tools/third_party" to the list of excludes for
the
> presubmit?
> 
> We don't want to be be making third party code lint free right?

Done.

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/tests/test_sdktools.py (right):

http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
native_client_sdk/src/build_tools/tests/test_sdktools.py:117: cmd.extend(['-U',
self.server.GetURL(MANIFEST_BASENAME)])
On 2012/10/22 22:55:30, Sam Clegg wrote:
> So does '-U' first no longer work? 

No, it is a breaking change. But in some ways I think it is clearer than having
a separate set of options for the "naclsdk" and the command that it runs.

[Sam Clegg, 2012-10-23 00:39:51]

lgtm, altough WRT to the wildcard imports I was thinking it would
be better to remove them. e.g.

from info import Info

Rather than defining __all__ in the commands files.

IIUC whole wildcard import thing is a bad idea.  However I don't
often write multi-file python modules so it might be one of the
times where it is not so bad to use them?


On 2012/10/23 00:14:09, binji wrote:
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> File native_client_sdk/src/build_tools/build_updater.py (right):
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/build_updater.py:45:
> ('build_tools/sdk_tools/third_party/fancy_urllib/*.py',
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > I guess there is a tradeoff here between listing explicitly
> > exactly what goes in the package, and the cost of maintaining the list.
> > 
> > I guess the risk to random other .py files being here at
> > package build time is close to zero?
> 
> Yes, I think this is clearer as well -- and less tedious to modify.
Ultimately,
> most of the stuff comes from the sdk_tools/ directory -- we should try to keep
> cruft out of it.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> File native_client_sdk/src/build_tools/sdk_tools/commands/__init__.py (right):
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/sdk_tools/commands/__init__.py:10: from
update
> import *
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > Are wild card imports ok in __init__.py files?
> > 
> > I find them scary in all cases.
> > 
> > How many things to do you really want to pull in here?
> 
> Done.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> File native_client_sdk/src/build_tools/sdk_tools/sdk_update_common.py (right):
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/sdk_tools/sdk_update_common.py:26: 
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > I'm still not completely sold on this pattern.
> > 
> > I prefer an early return if os.path.exists() but that is
> > really a style thing I guess.
> > 
> > The argument I suppose is that there could be another processes racing with
> up. 
> >  However, if some other process
> > was to create the folder between the two calls I'd argue that its better for
> > this program to just crash here.  i.e. externally messing with the tree
while
> > this program is not worth defending against.
> 
> Fair enough, we use os.path.exists elsewhere -- may as well be consistent.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> File native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py (right):
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:28: import
> manifest_util
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > Why not just always add to sys.path and avoid the try block?
> 
> Done.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:39: import
cygtar
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > Yuck.. is this so the script can work in SCM as well as when packaged?
> > 
> > Could the import not happen at the top of commands/__init__.py?
> 
> It seems that only update is using cygtar anyway, I just moved the import
there.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:103: raise
> Error('Json encoding error writing config "%s".\n  %s' % (path, e))
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > Maybe use two different try blocks here.   Generate json first, then write
it
> to
> > a file.
> > 
> > Is it worth catching IOError here?  Is it likely that the file is not
> writable?
> > 
> > We should probably always be using the the "with open() as.." pattern for
file
> > I/O.
> 
> Done.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:138:
> logging.error('Error encoding manifest "%s" to JSON.\n  %s' % (path, e))
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > same as above.
> > 
> > Why are you logging here rather than raising Error for the
> > user to see?
> 
> Ah, it was because I was calling this from the finally block, but when this
> function raised, it was killing the original raiser -- the better solution is
to
> handle that below.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:265: print
> "Native Client SDK Updater, version r%s" % (REVISION,)
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > no need to pass tuple here.   Just "% REVISION"
> 
> It's habit for me, I always make sure to pass a tuple to % for one element,
> because if that element is a tuple, it will be expanded. But yes, not
necessary
> here.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:279: return
> getattr(sys.modules[__name__], 'CMD' + name, None)
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > Hmm... I think globals() might be nicer here.  I really don't like using
> > sys.modules like this.. but I'm open to change.
> 
> Haha, this is stolen from git_cl.py...
> Done.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/sdk_tools/sdk_update_main.py:370:
> logging.error(str(e))
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > Does this basically just print the error?  Or does it decorate in some way?
> 
> it adds 'ERROR:', see fmt = '%(levelname)s:%(message)s' above.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> File
native_client_sdk/src/build_tools/sdk_tools/third_party/fancy_urllib/README
> (right):
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
>
native_client_sdk/src/build_tools/sdk_tools/third_party/fancy_urllib/README:36:
> +
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > Maybe better just to add "sdk_tools/third_party" to the list of excludes for
> the
> > presubmit?
> > 
> > We don't want to be be making third party code lint free right?
> 
> Done.
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> File native_client_sdk/src/build_tools/tests/test_sdktools.py (right):
> 
>
http://codereview.chromium.org/11228013/diff/5001/native_client_sdk/src/build...
> native_client_sdk/src/build_tools/tests/test_sdktools.py:117:
cmd.extend(['-U',
> self.server.GetURL(MANIFEST_BASENAME)])
> On 2012/10/22 22:55:30, Sam Clegg wrote:
> > So does '-U' first no longer work? 
> 
> No, it is a breaking change. But in some ways I think it is clearer than
having
> a separate set of options for the "naclsdk" and the command that it runs.

[binji, 2012-10-23 22:10:56]

Significant changes to fix windows. PTAL.

WRT "from foo import *", I think in this case it makes it easier to not have to
duplicate changes: add a new function in a command file, add it to the
__init__...

There's not really any danger to importing all -- it will all be namespaced
under commands anyway.

(Also, it doesn't seem to be forbidden by the style guide).

[Sam Clegg, 2012-10-23 22:17:25]

lgtm

http://codereview.chromium.org/11228013/diff/5007/native_client_sdk/src/build...
File native_client_sdk/src/build_tools/sdk_tools/sdk_update_common.py (right):

http://codereview.chromium.org/11228013/diff/5007/native_client_sdk/src/build...
native_client_sdk/src/build_tools/sdk_tools/sdk_update_common.py:43: max_tries =
1
If this is hardcoded is 1 is there any need to leave this extra loop in here?

[binji, 2012-10-25 23:24:53]

+noelallen

Any concerns, Noel? I'm working on tests for this now as a separate CL.

[noelallen1, 2012-10-29 23:11:30]

A few nits otherwise LGTM

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
File native_client_sdk/src/build_tools/sdk_tools/commands/info.py (right):

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/sdk_tools/commands/info.py:8: __all__ =
['Info']
What is this exactly?

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/sdk_tools/commands/info.py:24: for key, value
in bundle.iteritems():
Shouldn't these be sorted somehow?

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/sdk_tools/commands/info.py:29: for
archive_key, archive_value in archive.iteritems():
You print all archives if one is found for this host, but just print no archive
if there isn't one for this host.   Would printing all available archives and
then an error if no match make more sense?

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
File native_client_sdk/src/build_tools/sdk_tools/commands/list.py (right):

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/sdk_tools/commands/list.py:48: bundle.name,
What about a note for missing recommended bundles, or deprecated bundles?

[binji, 2012-10-29 23:51:18]

I've also renamed commands/ -> command/ (to get rid of pylint errors, etc.)

I also import command.foo directly rather that relying on "from import foo *".

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
File native_client_sdk/src/build_tools/sdk_tools/commands/info.py (right):

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/sdk_tools/commands/info.py:8: __all__ =
['Info']
On 2012/10/29 23:11:30, noelallen1 wrote:
> What is this exactly?

Removed.

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/sdk_tools/commands/info.py:24: for key, value
in bundle.iteritems():
On 2012/10/29 23:11:30, noelallen1 wrote:
> Shouldn't these be sorted somehow?

Done.

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/sdk_tools/commands/info.py:29: for
archive_key, archive_value in archive.iteritems():
On 2012/10/29 23:11:30, noelallen1 wrote:
> You print all archives if one is found for this host, but just print no
archive
> if there isn't one for this host.   Would printing all available archives and
> then an error if no match make more sense?

That's not quite right. I print all members of the host archive, or an error. I
think this is reasonable -- you can only install the HostOS archive. If there is
a bundle available only for another host os, do you really need info about it?

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
File native_client_sdk/src/build_tools/sdk_tools/commands/list.py (right):

http://codereview.chromium.org/11228013/diff/18001/native_client_sdk/src/buil...
native_client_sdk/src/build_tools/sdk_tools/commands/list.py:48: bundle.name,
On 2012/10/29 23:11:30, noelallen1 wrote:
> What about a note for missing recommended bundles, or deprecated bundles?

That's a good idea, but I'd like to do that in another CL.