build_nexe.py can now apply tls_edit

build_nexe.py can now apply tls_edit

Instead of custom gyp rules to apply tls_edit to IRT nexe's, build_nexe
can now apply tls_edit as an option. The gyp rule to build IRTs using
build_nexe.py can simply specify this option making all the custom
gyp rules irrelevent.

Note this change does not actually remove the extra tls_edit gyp rules,
that will be done as a separate step.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3127
TEST= trybots
R=bradnelson@google.com, mcgrathr@chromium.org, noelallen@chromium.org

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=12738

[David Yen, 2014-02-08 01:32:16]

I've added an option to use tls_edit inside of build_nexe.py. This simplifies
all of our gyp rules when building variants of the IRT.

If I've done everything correctly, we should be able to revert all the old gyp
rule changes for post processing tls_edit on IRT nexe's and it should "just
work".

[bradn, 2014-02-08 22:32:29]

lgtm

https://codereview.chromium.org/137803016/diff/1/build/untrusted.gypi
File build/untrusted.gypi (right):

https://codereview.chromium.org/137803016/diff/1/build/untrusted.gypi#newcode905
build/untrusted.gypi:905: '--tls-edit=<(PRODUCT_DIR)/tls_edit',
So one minor wrinkle with this is that while there's a intra-target dependency
on tls_edit (ie if the target has dependencies that may cause it to be rebuilt,
it will be rebuilt if tls_edit changes), there is no automatic inter-target
depedency.
So two things to watch for:
- if there depdencies aren't right, steps requiring tls_edit may be attempted
before it's ready
- if tls_edit changes, targets that have already been built using a different
version of it won't notice the change.

As you've only got 2 targets (irt, irt_core) that need to be gotten right, this
is probably ok. But be careful.

[Roland McGrath, 2014-02-10 17:18:20]

build_nexe.py doesn't know or need to know what the thing is doing.
It just happens to be tls_edit.  So I'd call the option --binary-editor or
something more generic like that.

[David Yen, 2014-02-10 17:38:36]

On 2014/02/08 22:32:29, bradn wrote:
> lgtm
> 
> https://codereview.chromium.org/137803016/diff/1/build/untrusted.gypi
> File build/untrusted.gypi (right):
> 
>
https://codereview.chromium.org/137803016/diff/1/build/untrusted.gypi#newcode905
> build/untrusted.gypi:905: '--tls-edit=<(PRODUCT_DIR)/tls_edit',
> So one minor wrinkle with this is that while there's a intra-target dependency
> on tls_edit (ie if the target has dependencies that may cause it to be
rebuilt,
> it will be rebuilt if tls_edit changes), there is no automatic inter-target
> depedency.
> So two things to watch for:
> - if there depdencies aren't right, steps requiring tls_edit may be attempted
> before it's ready
> - if tls_edit changes, targets that have already been built using a different
> version of it won't notice the change.
> 
> As you've only got 2 targets (irt, irt_core) that need to be gotten right,
this
> is probably ok. But be careful.

Yes this really bothered me too. I was going to ask you if there was any way to
inject or
mix dependencies from gyp and gypi rules so we could automatically depend on
tls_edit if it is specified. I looked around at other gypi files for a bit and
didn't see
anyone else doing anything like that.

On 2014/02/10 17:18:20, Roland McGrath wrote:
> build_nexe.py doesn't know or need to know what the thing is doing.
> It just happens to be tls_edit.  So I'd call the option --binary-editor or
> something more generic like that.

I played around with this idea (and even thought about making it generic enough
where the stripping step could be another process we pass in as an argument),
but there are certain restrictions that makes it hard to be a completely generic
process:

1. The caller would have to know that the executable will be called between
linker and debug stripper, which makes a generic process really far less useful.
2. The arguments of the caller would either have to be specified on the command
line somehow, or they would have to know its hard coded to something like
"$(EDITOR) $(LINKFILE) $(OUTPUTFILE)"
3. The intermediate file name should probably be specified on the command line
if this were a generic process.
4. If this were generic multiple post process executables should probably be
supported?

Given these restrictions I figured making it generic would have limited value,
while specifying that it is tls_edit makes it clear that calling build_nexe with
that argument will build a nexe with the tls modified for the IRT. It would be
even clearer if the option was named "-irt-tls" or something like that, but then
it would be less clear that we need to pass in tls_edit as an argument.

[David Yen, 2014-02-10 21:22:26]

I added a patch that removes the tls_edit rules from irt.gyp.

[noelallen1, 2014-02-10 21:38:36]

LGTM except that tls_edit is an executable?  the gyp change isn't here so can't
tell.  If it is, doesn't tls_edit listed as in input in untrusted.gypi need a
.exe suffix for Windows?

https://codereview.chromium.org/137803016/diff/290001/build/untrusted.gypi
File build/untrusted.gypi (right):

https://codereview.chromium.org/137803016/diff/290001/build/untrusted.gypi#ne...
build/untrusted.gypi:338: '<(PRODUCT_DIR)/tls_edit',
does this need to be tls_edit.exe on Windows?

[Roland McGrath, 2014-02-11 18:13:49]

lgtm

[David Yen, 2014-02-11 18:14:26]

I added the gyp executable suffix to the end as Noel suggested, ptal

[David Yen, 2014-02-11 18:15:50]

Committed patchset #3 manually as r12738 (presubmit successful).

[Mark Seaborn, 2014-02-12 16:07:04]

This seems to cause the Chromium Windows build to fail with:

'..\..\out\Release' is not recognized as an internal or external command,

operable program or batch file.

FAILED with 0: ..\..\out\Release/tls_edit.exe
..\..\out\Release/gen/nacl_irt_x86_64_raw.nexe.debug.raw
..\..\out\Release/gen/nacl_irt_x86_64_raw.nexe.debug

See
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/262332...
(from https://codereview.chromium.org/153823011/)

[dyen_google.com, 2014-02-12 18:27:22]

It looks like a slash issue in windows, I've fixed it here:

https://codereview.chromium.org/160613003/


On Wed, Feb 12, 2014 at 8:07 AM, <mseaborn@chromium.org> wrote:

> This seems to cause the Chromium Windows build to fail with:
>
> '..\..\out\Release' is not recognized as an internal or external command,
>
> operable program or batch file.
>
> FAILED with 0: ..\..\out\Release/tls_edit.exe
> ..\..\out\Release/gen/nacl_irt_x86_64_raw.nexe.debug.raw
> ..\..\out\Release/gen/nacl_irt_x86_64_raw.nexe.debug
>
> See
> http://build.chromium.org/p/tryserver.chromium/builders/
> win_rel/builds/262332/steps/compile/logs/stdio
> (from https://codereview.chromium.org/153823011/)
>
>
> https://codereview.chromium.org/137803016/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/groups/opt_out.