GN: Avoid nontrivial shell commands in gcc_toolchain tools

GN: Avoid nontrivial shell commands in gcc_toolchain tools

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
nontrivial sh syntax or other POSIX utilities (such as 'cut').
The "ar" and "solink" tools do more than a simple command and
used POSIX sh syntax to do their work.  To make these tools
portable, make them use Python scripts instead of those complex
shell command lines.

BUG=512869
R=dpranke@chromium.org

Committed: https://crrev.com/0b367cc8cf4b27037e5c709adf37271c0d90d392
Cr-Commit-Position: refs/heads/master@{#356699}

[Dirk Pranke, 2015-10-28 01:51:31]

I haven't had a chance to look at this yet, sorry! I'll try to get to it
tomorrow.

[Dirk Pranke, 2015-10-28 22:13:03]

mostly lgtm, just one possible bug and some nits.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
File build/toolchain/gcc_ar_wrapper.py (right):

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
build/toolchain/gcc_ar_wrapper.py:33: help='Load plugin')
nit: I'd reorder --plugin before operation.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
build/toolchain/gcc_ar_wrapper.py:48: pass
I suspect you shouldn't be ignoring the error here ...

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
File build/toolchain/gcc_solink_wrapper.py (right):

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
build/toolchain/gcc_solink_wrapper.py:21: """
nit: format the comments on a single line here.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
build/toolchain/gcc_solink_wrapper.py:27: if soname_re.search(line):
nit: you could skip the re here and just do:

  if 'SONAME' in line:
    toc += line

I'd probably also do:

  stdout, _ = readelf.communicate()
  for line in stdout:
    ...
  return readelf.returncode, toc

instead, but it's roughly equivalent.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
build/toolchain/gcc_solink_wrapper.py:30: 
nit two blank lines between top-level functions.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
build/toolchain/gcc_solink_wrapper.py:32: """Replaces: nm --format=posix -g -D
$sofile | cut -f1-2 -d
nit: cut -f1-2 -d' '

[Roland McGrath, 2015-10-28 22:25:16]

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
File build/toolchain/gcc_ar_wrapper.py (right):

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
build/toolchain/gcc_ar_wrapper.py:33: help='Load plugin')
On 2015/10/28 22:13:03, Dirk Pranke wrote:
> nit: I'd reorder --plugin before operation.

Done.  I originally wrote them in the order they are actually used on the
command line.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
build/toolchain/gcc_ar_wrapper.py:48: pass
On 2015/10/28 22:13:03, Dirk Pranke wrote:
> I suspect you shouldn't be ignoring the error here ...

Ignoring the error is the equivalent of 'rm -f', which is what this is
replacing.
It could do if os.path.exists: os.remove, but that is costlier and racy.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
File build/toolchain/gcc_solink_wrapper.py (right):

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
build/toolchain/gcc_solink_wrapper.py:21: """
On 2015/10/28 22:13:03, Dirk Pranke wrote:
> nit: format the comments on a single line here.

Done.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
build/toolchain/gcc_solink_wrapper.py:27: if soname_re.search(line):
On 2015/10/28 22:13:03, Dirk Pranke wrote:
> nit: you could skip the re here and just do:
> 
>   if 'SONAME' in line:
>     toc += line
> 
> I'd probably also do:
> 
>   stdout, _ = readelf.communicate()
>   for line in stdout:
>     ...
>   return readelf.returncode, toc
> 
> instead, but it's roughly equivalent.

Done.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
build/toolchain/gcc_solink_wrapper.py:30: 
On 2015/10/28 22:13:03, Dirk Pranke wrote:
> nit two blank lines between top-level functions.

Done.

https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_solink_...
build/toolchain/gcc_solink_wrapper.py:32: """Replaces: nm --format=posix -g -D
$sofile | cut -f1-2 -d
On 2015/10/28 22:13:03, Dirk Pranke wrote:
> nit: cut -f1-2 -d' '

Done.

[Roland McGrath, 2015-10-28 22:25:37]

The CQ bit was checked by mcgrathr@chromium.org

[Roland McGrath, 2015-10-28 22:25:38]

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/1413073005/#ps20001
(title: "review nits")

[commit-bot: I haz the power, 2015-10-28 22:26:26]

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

[Dirk Pranke, 2015-10-28 22:40:32]

On 2015/10/28 22:25:16, Roland McGrath wrote:
>
https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
> File build/toolchain/gcc_ar_wrapper.py (right):
> 
>
https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
> build/toolchain/gcc_ar_wrapper.py:33: help='Load plugin')
> On 2015/10/28 22:13:03, Dirk Pranke wrote:
> > nit: I'd reorder --plugin before operation.
> 
> Done.  I originally wrote them in the order they are actually used on the
> command line.
> 
>
https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
> build/toolchain/gcc_ar_wrapper.py:48: pass
> On 2015/10/28 22:13:03, Dirk Pranke wrote:
> > I suspect you shouldn't be ignoring the error here ...
> 
> Ignoring the error is the equivalent of 'rm -f', which is what this is
> replacing.

I don't think that's right:

  % rm -f /etc/passwd && echo hello
  rm: /etc/passwd: Permission denied
  %

The rm command still fails in this case and so we don't go on to execute the
second command. 

rm -f will not fail on a missing file, so you can check for errno.ENOENT in the
catch clause and
ignore that one, I think.

[Roland McGrath, 2015-10-28 22:55:51]

The CQ bit was unchecked by mcgrathr@chromium.org

[Roland McGrath, 2015-10-28 23:04:20]

On 2015/10/28 22:40:32, Dirk Pranke wrote:
> On 2015/10/28 22:25:16, Roland McGrath wrote:
> >
>
https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
> > File build/toolchain/gcc_ar_wrapper.py (right):
> > 
> >
>
https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
> > build/toolchain/gcc_ar_wrapper.py:33: help='Load plugin')
> > On 2015/10/28 22:13:03, Dirk Pranke wrote:
> > > nit: I'd reorder --plugin before operation.
> > 
> > Done.  I originally wrote them in the order they are actually used on the
> > command line.
> > 
> >
>
https://codereview.chromium.org/1413073005/diff/1/build/toolchain/gcc_ar_wrap...
> > build/toolchain/gcc_ar_wrapper.py:48: pass
> > On 2015/10/28 22:13:03, Dirk Pranke wrote:
> > > I suspect you shouldn't be ignoring the error here ...
> > 
> > Ignoring the error is the equivalent of 'rm -f', which is what this is
> > replacing.
> 
> I don't think that's right:
> 
>   % rm -f /etc/passwd && echo hello
>   rm: /etc/passwd: Permission denied
>   %
> 
> The rm command still fails in this case and so we don't go on to execute the
> second command. 
> 
> rm -f will not fail on a missing file, so you can check for errno.ENOENT in
the
> catch clause and
> ignore that one, I think.

Done.

[Roland McGrath, 2015-10-28 23:04:40]

The CQ bit was checked by mcgrathr@chromium.org

[Roland McGrath, 2015-10-28 23:04:41]

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/1413073005/#ps40001
(title: "pedantically correct os.remove error handling")

[commit-bot: I haz the power, 2015-10-28 23:05:04]

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

[commit-bot: I haz the power, 2015-10-29 00:24:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-29 00:25:20]

Patchset 3 (id:??) landed as
https://crrev.com/0b367cc8cf4b27037e5c709adf37271c0d90d392
Cr-Commit-Position: refs/heads/master@{#356699}