GN: Avoid nontrivial shell commands in gcc_toolchain tool("link")

GN: Avoid nontrivial shell commands in gcc_toolchain tool("link")

The gcc_toolchain template is used for NaCl toolchains on every host
OS, as well as for the native toolchains on a POSIXy OS.  When the
host is not POSIXy, the toolchain commands cannot use sh syntax (not
even '&&').  The "link" tool does more than a simple command when
'strip' or 'postlink' variables are set, and used POSIX sh syntax to
in these cases.  To make this tool portable, make it use a Python
script instead of those complex shell command lines.  The 'postlink'
variable was used only for the pnacl toolchain's "finalize" step and
it can just as well use the 'strip' variable for that purpose, so
simplify things by eliminating 'postlink' entirely.

BUG=512869
R=dpranke@chromium.org, dschuff@chromium.org

Committed: https://crrev.com/46f8e4a291285770abb2357402fe8e662e00df2e
Cr-Commit-Position: refs/heads/master@{#358684}

[Dirk Pranke, 2015-11-03 23:49:13]

lgtm w/ a couple of nits.

Also, your CL description isn't quite right, because Mac is POSIXy but doesn't
use gcc_toolchain.

https://codereview.chromium.org/1432603003/diff/1/build/toolchain/gcc_link_wr...
File build/toolchain/gcc_link_wrapper.py (right):

https://codereview.chromium.org/1432603003/diff/1/build/toolchain/gcc_link_wr...
build/toolchain/gcc_link_wrapper.py:1: #!/usr/bin/env python
nit: probably should remove the shebang line.

https://codereview.chromium.org/1432603003/diff/1/build/toolchain/nacl/BUILD.gn
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/1432603003/diff/1/build/toolchain/nacl/BUILD....
build/toolchain/nacl/BUILD.gn:40: strip = toolprefix + "finalize"
maybe add a comment here about why we're using the strip variable for finalize?

[Dirk Pranke, 2015-11-03 23:50:13]

dpranke@chromium.org changed reviewers:
- brettw@chromium.org, scottmg@chromium.org, thakis@chromium.org

[Dirk Pranke, 2015-11-03 23:50:13]

also, simplifying the reviewer list and moving more people to CC:

[Dirk Pranke, 2015-11-03 23:50:30]

Description was changed from

==========
GN: Avoid nontrivial shell commands in gcc_toolchain tool("link")

The gcc_toolchain template is used for NaCl toolchains on every host
OS, as well as for the native toolchains on a POSIXy OS.  When the
host is not POSIXy, the toolchain commands cannot use sh syntax (not
even '&&').  The "link" tool does more than a simple command when
'strip' or 'postlink' variables are set, and used POSIX sh syntax to
in these cases.  To make this tool portable, make it use a Python
script instead of those complex shell command lines.  The 'postlink'
variable was used only for the pnacl toolchain's "finalize" step and
it can just as well use the 'strip' variable for that purpose, so
simplify things by eliminating 'postlink' entirely.

BUG= 512869
R=brettw@chromium.org, dpranke@chromium.org, dschuff@chromium.org,
scottmg@chromium.org, thakis@chromium.org
==========

to

==========
GN: Avoid nontrivial shell commands in gcc_toolchain tool("link")

The gcc_toolchain template is used for NaCl toolchains on every host
OS, as well as for the native toolchains on a POSIXy OS.  When the
host is not POSIXy, the toolchain commands cannot use sh syntax (not
even '&&').  The "link" tool does more than a simple command when
'strip' or 'postlink' variables are set, and used POSIX sh syntax to
in these cases.  To make this tool portable, make it use a Python
script instead of those complex shell command lines.  The 'postlink'
variable was used only for the pnacl toolchain's "finalize" step and
it can just as well use the 'strip' variable for that purpose, so
simplify things by eliminating 'postlink' entirely.

BUG= 512869
R=dpranke@chromium.org, dschuff@chromium.org
==========

[scottmg, 2015-11-03 23:54:23]

On 2015/11/03 23:49:13, Dirk Pranke wrote:
> lgtm w/ a couple of nits.
> 
> Also, your CL description isn't quite right, because Mac is POSIXy but doesn't
> use gcc_toolchain.
> 
>
https://codereview.chromium.org/1432603003/diff/1/build/toolchain/gcc_link_wr...
> File build/toolchain/gcc_link_wrapper.py (right):
> 
>
https://codereview.chromium.org/1432603003/diff/1/build/toolchain/gcc_link_wr...
> build/toolchain/gcc_link_wrapper.py:1: #!/usr/bin/env python
> nit: probably should remove the shebang line.

I think there's a presubmit that complains if you do that (because I often get
whacked by it).

>
https://codereview.chromium.org/1432603003/diff/1/build/toolchain/nacl/BUILD.gn
> File build/toolchain/nacl/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1432603003/diff/1/build/toolchain/nacl/BUILD....
> build/toolchain/nacl/BUILD.gn:40: strip = toolprefix + "finalize"
> maybe add a comment here about why we're using the strip variable for
finalize?

[Dirk Pranke, 2015-11-04 00:03:30]

On 2015/11/03 23:54:23, scottmg wrote:
> On 2015/11/03 23:49:13, Dirk Pranke wrote:
> > nit: probably should remove the shebang line.
> 
> I think there's a presubmit that complains if you do that (because I often get
> whacked by it).

IIRC, the presubmit complains if there's a mismatch between the presence of
the shebang and whether the script is executable (i.e., you need either both or
neither). In this case, it should be neither.

[Roland McGrath, 2015-11-04 00:06:44]

https://codereview.chromium.org/1432603003/diff/1/build/toolchain/gcc_link_wr...
File build/toolchain/gcc_link_wrapper.py (right):

https://codereview.chromium.org/1432603003/diff/1/build/toolchain/gcc_link_wr...
build/toolchain/gcc_link_wrapper.py:1: #!/usr/bin/env python
On 2015/11/03 23:49:13, Dirk Pranke wrote:
> nit: probably should remove the shebang line.

I'm following the example of all the .py files I've seen in the repo.

https://codereview.chromium.org/1432603003/diff/1/build/toolchain/nacl/BUILD.gn
File build/toolchain/nacl/BUILD.gn (right):

https://codereview.chromium.org/1432603003/diff/1/build/toolchain/nacl/BUILD....
build/toolchain/nacl/BUILD.gn:40: strip = toolprefix + "finalize"
On 2015/11/03 23:49:13, Dirk Pranke wrote:
> maybe add a comment here about why we're using the strip variable for
finalize?

Done.

[Dirk Pranke, 2015-11-04 00:11:37]

On 2015/11/04 00:06:44, Roland McGrath wrote:
>
https://codereview.chromium.org/1432603003/diff/1/build/toolchain/gcc_link_wr...
> File build/toolchain/gcc_link_wrapper.py (right):
> 
>
https://codereview.chromium.org/1432603003/diff/1/build/toolchain/gcc_link_wr...
> build/toolchain/gcc_link_wrapper.py:1: #!/usr/bin/env python
> On 2015/11/03 23:49:13, Dirk Pranke wrote:
> > nit: probably should remove the shebang line.
> 
> I'm following the example of all the .py files I've seen in the repo.

Hmm. Okay, looking at this now, I can see that the style guide (which I think I
wrote
at some point) is ambiguous, so I'll let you slide for now. Though the file
should be executable per the presubmit comments ... :).

[Roland McGrath, 2015-11-04 00:52:35]

The CQ bit was checked by mcgrathr@chromium.org

[Roland McGrath, 2015-11-04 00:52:35]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1432603003/#ps40001
(title: "add comment")

[commit-bot: I haz the power, 2015-11-04 00:55:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432603003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432603003/40001

[Roland McGrath, 2015-11-04 01:10:11]

mcgrathr@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Roland McGrath, 2015-11-04 01:10:11]

+sergeyu for remoting/OWNERS review

[commit-bot: I haz the power, 2015-11-04 01:24:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-04 01:24:04]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Sergey Ulanov, 2015-11-04 07:36:23]

https://codereview.chromium.org/1432603003/diff/40001/remoting/webapp/build_t...
File remoting/webapp/build_template.gni (right):

https://codereview.chromium.org/1432603003/diff/40001/remoting/webapp/build_t...
remoting/webapp/build_template.gni:207: [
"$pexe_dir/exe.unstripped/remoting_client_plugin_newlib.pexe" ]
The build-webapp.py just copies all files into the target directory. There is
also .nmf file that expects that the debug version has .pexe.debug suffix. So if
the file name is the same this is not going to produce correct build because
.debug file will replace non-debug one. Can we have .pexe.debug extension in the
exe.unstripped directory? Otherwise you either need to modify build-webapp.py to
rename the file (and specify that in the arguments) or add a copy action here to
copy the file to give it .pexe.debug name.

[Roland McGrath, 2015-11-04 20:26:47]

https://codereview.chromium.org/1432603003/diff/40001/remoting/webapp/build_t...
File remoting/webapp/build_template.gni (right):

https://codereview.chromium.org/1432603003/diff/40001/remoting/webapp/build_t...
remoting/webapp/build_template.gni:207: [
"$pexe_dir/exe.unstripped/remoting_client_plugin_newlib.pexe" ]
On 2015/11/04 07:36:23, Sergey Ulanov wrote:
> The build-webapp.py just copies all files into the target directory. There is
> also .nmf file that expects that the debug version has .pexe.debug suffix. So
if
> the file name is the same this is not going to produce correct build because
> .debug file will replace non-debug one. Can we have .pexe.debug extension in
the
> exe.unstripped directory? Otherwise you either need to modify build-webapp.py
to
> rename the file (and specify that in the arguments) or add a copy action here
to
> copy the file to give it .pexe.debug name.

I think it's best that the toolchain rule use the exe.unstripped subdirectory
rather than the .debug suffix, because that makes it consistent with the other
GN toolchains.

It's probably best to use a copy rule for what build-webapp.py expects, because
that will make it consistent with the GYP build.  I'll take a crack at that.

Can you give me a direct pointer to a bot or a local testing command that will
verify that I haven't broken this part of the build?


Thanks,
Roland

[Sergey Ulanov, 2015-11-04 22:52:50]

On 2015/11/04 20:26:47, Roland McGrath wrote:
>
https://codereview.chromium.org/1432603003/diff/40001/remoting/webapp/build_t...
> File remoting/webapp/build_template.gni (right):
> 
>
https://codereview.chromium.org/1432603003/diff/40001/remoting/webapp/build_t...
> remoting/webapp/build_template.gni:207: [
> "$pexe_dir/exe.unstripped/remoting_client_plugin_newlib.pexe" ]
> On 2015/11/04 07:36:23, Sergey Ulanov wrote:
> > The build-webapp.py just copies all files into the target directory. There
is
> > also .nmf file that expects that the debug version has .pexe.debug suffix.
So
> if
> > the file name is the same this is not going to produce correct build because
> > .debug file will replace non-debug one. Can we have .pexe.debug extension in
> the
> > exe.unstripped directory? Otherwise you either need to modify
build-webapp.py
> to
> > rename the file (and specify that in the arguments) or add a copy action
here
> to
> > copy the file to give it .pexe.debug name.
> 
> I think it's best that the toolchain rule use the exe.unstripped subdirectory
> rather than the .debug suffix, because that makes it consistent with the other
> GN toolchains.
> 
> It's probably best to use a copy rule for what build-webapp.py expects,
because
> that will make it consistent with the GYP build.  I'll take a crack at that.

If you add .unstripped suffix for unstripped binaries then you can just need to
update remoting/webapp/crd/remoting_client_pnacl.nmf.jinja2 , no need to update
build-webapp.py or copy the file. It's only the problem when the unstripped
binary has the same name  


> 
> Can you give me a direct pointer to a bot or a local testing command that will
> verify that I haven't broken this part of the build?

Just build //remoting/webapp with is_debug=true and then verify that
//out/.../remoting/remoting.webapp.v2/ contains two .pexe files (stripped and
unstripped) and the *.nmf file contains correct names 

> 
> 
> Thanks,
> Roland

[Roland McGrath, 2015-11-04 23:00:34]

On 2015/11/04 22:52:50, Sergey Ulanov wrote:
> If you add .unstripped suffix for unstripped binaries then you can just need
to
> update remoting/webapp/crd/remoting_client_pnacl.nmf.jinja2 , no need to
update
> build-webapp.py or copy the file. It's only the problem when the unstripped
> binary has the same name  

As I said before I don't think it's a good idea to have the newlib_pnacl
toolchain do this, because it would be inconsistent with all the other
toolchains.

> > Can you give me a direct pointer to a bot or a local testing command that
will
> > verify that I haven't broken this part of the build?
> 
> Just build //remoting/webapp with is_debug=true and then verify that
> //out/.../remoting/remoting.webapp.v2/ contains two .pexe files (stripped and
> unstripped) and the *.nmf file contains correct names 

'ninja -C ... remoting/webapp' with a GN build on Linux/x86-64 is currently
broken out of the box, unrelated to my change.
Dirk is looking into it but perhaps you could help him.

[Dirk Pranke, 2015-11-05 01:21:28]

On 2015/11/04 23:00:34, Roland McGrath wrote:
> 'ninja -C ... remoting/webapp' with a GN build on Linux/x86-64 is currently
> broken out of the box, unrelated to my change.
> Dirk is looking into it but perhaps you could help him.

Sergey fixed the webrtc issue that was breaking this, and I've posted a fix for
another issue w/ net/, so this 
should start working again relatively soon.

[Roland McGrath, 2015-11-05 19:00:18]

PTAL

New version has a copy rule for remoting/webapp's .pexe.debug file.
After the unrelated fixes Dirk and Sergey posted, building the remoting/webapp
target works with either the old or new version of my change.
The eyeball test shows me the .pexe.debug file exists where you want it.  But
I'm not aware of any build target or test to run that would verify it--i.e.,
something that was broken with my previous version of this change.

[Sergey Ulanov, 2015-11-09 21:14:47]

https://codereview.chromium.org/1432603003/diff/80001/remoting/client/plugin/...
File remoting/client/plugin/BUILD.gn (right):

https://codereview.chromium.org/1432603003/diff/80001/remoting/client/plugin/...
remoting/client/plugin/BUILD.gn:45: if (enable_pnacl && is_debug) {
Duplicating the target like this makes it harder to maintain that file. To make
it simpler maybe always copy the file?
Or perhaps a better solution is to add remoting_client_plugin_debug copy()
target and depend on it in remoting/webapp/build_template.gni only in debug
builds.

[Roland McGrath, 2015-11-09 21:30:21]

PTAL

https://codereview.chromium.org/1432603003/diff/80001/remoting/client/plugin/...
File remoting/client/plugin/BUILD.gn (right):

https://codereview.chromium.org/1432603003/diff/80001/remoting/client/plugin/...
remoting/client/plugin/BUILD.gn:45: if (enable_pnacl && is_debug) {
On 2015/11/09 21:14:46, Sergey Ulanov wrote:
> Duplicating the target like this makes it harder to maintain that file. To
make
> it simpler maybe always copy the file?
> Or perhaps a better solution is to add remoting_client_plugin_debug copy()
> target and depend on it in remoting/webapp/build_template.gni only in debug
> builds.

The "unstripped" (pre-finalized) file doesn't get saved anywhere if !is_debug,
so there is nothing to copy in that case.

I've changed the name of the copy rule as you suggested.

[Sergey Ulanov, 2015-11-09 21:32:59]

lgtm

[Roland McGrath, 2015-11-09 21:33:47]

The CQ bit was checked by mcgrathr@chromium.org

[Roland McGrath, 2015-11-09 21:33:47]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1432603003/#ps100001
(title: "change name of remoting copy rule")

[commit-bot: I haz the power, 2015-11-09 21:37:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432603003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432603003/100001

[commit-bot: I haz the power, 2015-11-09 21:44:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-09 21:44:23]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Roland McGrath, 2015-11-09 21:49:28]

The CQ bit was checked by mcgrathr@chromium.org

[Roland McGrath, 2015-11-09 21:49:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1432603003/#ps120001
(title: "gn set-but-unused nit")

[commit-bot: I haz the power, 2015-11-09 21:51:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432603003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432603003/120001

[commit-bot: I haz the power, 2015-11-09 22:28:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-09 22:29:43]

Patchset 7 (id:??) landed as
https://crrev.com/46f8e4a291285770abb2357402fe8e662e00df2e
Cr-Commit-Position: refs/heads/master@{#358684}