NaCl: Allow download_nacl_irt.py to be usefully invoked with no arguments

NaCl: Allow download_nacl_irt.py to be usefully invoked with no arguments

Extend the script to read the NaCl revisions/hashes directly from DEPS
in this case.

This makes the script usable in cases where you don't want to run
"gclient runhooks", such as when creating a tarball of the checkout,
in which case it is not desirable to run Gyp and have it generate
makefiles.

Admittedly this is redundant, because it adds a second way of getting
information from DEPS to the download_nacl_irt.py script.

The implementation of EvalDepsFile() is borrowed from:
http://src.chromium.org/viewvc/native_client/trunk/src/native_client/build/chromebinaries.py?revision=4577&view=markup

BUG=http://bugs.gentoo.org/show_bug.cgi?id=366413
TEST=run "python src/build/download_nacl_irt.py"

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

[Paweł Hajdan Jr., 2011-05-09 14:08:48]

Thank you for quick reaction. Please make sure to get this reviewed by someone
else from the NaCl team.

http://codereview.chromium.org/6966010/diff/1/build/download_nacl_irt.py
File build/download_nacl_irt.py (right):

http://codereview.chromium.org/6966010/diff/1/build/download_nacl_irt.py#newc...
build/download_nacl_irt.py:157: deps_data = EvalDepsFile(os.path.join('src',
'DEPS'))
This is duplicating some gclient code and it's relying on something that may be
considered an implementation detail of gclient. How about switching things
entirely to this file, i.e. remove the revision numbers and hashes from DEPS and
moving it so some other file where you can trivially parse them (it might even
be this file).

Then you can get rid of the conditional logic, and make the gclient hook run
this script without parameters.

[Mark Seaborn, 2011-05-09 14:19:42]

On 9 May 2011 15:08, <phajdan.jr@chromium.org> wrote:

> Thank you for quick reaction. Please make sure to get this reviewed by
> someone
> else from the NaCl team.
>
>
> http://codereview.chromium.org/6966010/diff/1/build/download_nacl_irt.py
> File build/download_nacl_irt.py (right):
>
>
>
http://codereview.chromium.org/6966010/diff/1/build/download_nacl_irt.py#newc...
> build/download_nacl_irt.py:157: deps_data =
>
> EvalDepsFile(os.path.join('src', 'DEPS'))
> This is duplicating some gclient code and it's relying on something that
> may be considered an implementation detail of gclient.


I'm not sure about that:  the DEPS file format is part of gclient's
interface, not its implementation.  I suppose gclient could gain a reader
that doesn't rely on Python's "eval" and then extend the format in a manner
incompatible with Python, but it seems unlikely.


> How about switching things entirely to this file,
> i.e. remove the revision numbers
> and hashes from DEPS and moving it so some other file where you can
> trivially parse them (it might even be this file).
>

We can't remove nacl_revision from DEPS because it is used for checking out
native_client from SVN.

So although I could remove the hashes, it wouldn't gain much if the script
still has to get nacl_revision from DEPS.

FWIW, in NaCl's standalone build we use the same EvalDepsFile() function in
order to download Chrome binaries to test NaCl against.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2011-05-09 14:19:45]

On 9 May 2011 15:08, <phajdan.jr@chromium.org> wrote:

> Thank you for quick reaction. Please make sure to get this reviewed by
> someone
> else from the NaCl team.
>
>
> http://codereview.chromium.org/6966010/diff/1/build/download_nacl_irt.py
> File build/download_nacl_irt.py (right):
>
>
>
http://codereview.chromium.org/6966010/diff/1/build/download_nacl_irt.py#newc...
> build/download_nacl_irt.py:157: deps_data =
>
> EvalDepsFile(os.path.join('src', 'DEPS'))
> This is duplicating some gclient code and it's relying on something that
> may be considered an implementation detail of gclient.


I'm not sure about that:  the DEPS file format is part of gclient's
interface, not its implementation.  I suppose gclient could gain a reader
that doesn't rely on Python's "eval" and then extend the format in a manner
incompatible with Python, but it seems unlikely.


> How about switching things entirely to this file,
> i.e. remove the revision numbers
> and hashes from DEPS and moving it so some other file where you can
> trivially parse them (it might even be this file).
>

We can't remove nacl_revision from DEPS because it is used for checking out
native_client from SVN.

So although I could remove the hashes, it wouldn't gain much if the script
still has to get nacl_revision from DEPS.

FWIW, in NaCl's standalone build we use the same EvalDepsFile() function in
order to download Chrome binaries to test NaCl against.

Cheers,
Mark

[Paweł Hajdan Jr., 2011-05-09 14:28:11]

+M-A

If this is fine for you, it's also fine for me. I think there might be some risk
of this script and gclient getting out of sync, but I'm not sure how likely is
that and what are other options.

[M-A Ruel, 2011-05-09 19:07:00]

I don't have much opinion.

[Paweł Hajdan Jr., 2011-05-09 19:30:57]

Rubber-stamp LGTM then. Thanks!

[bradn, 2011-05-10 19:52:56]

LGTM