<exec> is broken in several ways, here's a patch to fix

[GoogleCodeExporter]

What steps will reproduce the problem?

  1. create a hamakefile that attempts to use <exec> (try binary="/bin/echo")
  2. run it :-)

What is the expected output? What do you see instead?

  There are several issues:

  1. The binary= attribute value is interpreted as an HDFS path and so the shell ends up seeing something like
     "hdfs://<namenode>/bin/echo" instead of exactly what is specified (modulo simple variable substitutions).

  2. The standard output and error of the subprocess are ignored and therefore lost.

  3. The I/O streams of the subprocess are never explicitly closed, leading eventually to a "too many open
     files" condition (esp. in a <foreach> over many inputs).

  4. In "dry-run" mode, the extra check in Exec.execute() prevents Utils.execute() from even logging what command
     would have been run.

What version of the product are you using? On what operating system?

  These issues all exist in trunk (2.0b-4) as of this filing, and at least most exist in the 2.0b-3 release.
  OS doesn't matter for these issues, but I'm running under Linux w/ Oracle JDK 1.6.0_17.

Please provide any additional information below.

  The attached patch (against trunk@372) addresses all of the above issues:

  1. The binary field of Exec is now a String rather than a Path, and SyntaxParser.parseExecTask() no longer calls Utils.resolvePath().

  2. Utils.execute() now creates threads to copy the subprocesses standard output and error to System.out and System.err, respectively.

  3. Utils.execute() now explicitly closes all subprocess I/O streams.

  4. The extra check for dry-run mode has been removed from Exec.execute().

-peter

Original issue reported on code.google.com by petenewc...@gmail.com on 17 Jul 2011 at 3:47

Attachments:

• fixexec.patch

[GoogleCodeExporter]

Peter, thank you for the patch! We will review and apply.

Original comment by kroko...@gmail.com on 17 Jul 2011 at 4:24

[GoogleCodeExporter]

Peter, thank you for the patch! We will review and apply.

Original comment by kroko...@gmail.com on 17 Jul 2011 at 4:24

[GoogleCodeExporter]

Vladimir, please review the patch and apply if you have not objections.

Original comment by kroko...@gmail.com on 17 Jul 2011 at 4:25

[GoogleCodeExporter]

Original comment by kroko...@gmail.com on 18 Jul 2011 at 1:52

• Changed state: Accepted

[GoogleCodeExporter]

I have reviewed and applied this patch towards trunk. Thanks!

Original comment by kroko...@gmail.com on 20 Jul 2011 at 1:12

• Changed state: Fixed

[GoogleCodeExporter]

On second thought - looks like it is removed DRY_RUN support.

Original comment by kroko...@gmail.com on 20 Jul 2011 at 1:14

• Changed state: New

[GoogleCodeExporter]

fixed now

Original comment by kroko...@gmail.com on 20 Jul 2011 at 1:18

• Changed state: Fixed

[GoogleCodeExporter]

I don't believe that dry-run support is actually removed by the original patch, 
because even though the dry-run if statement is removed from Exec.execute(), 
there is still one in Utils.execute().

The reason to remove the statement from Exec.execute() is because it is useful 
for dry-run mode to log the commands that would have been executed.  Since that 
logging is performed by Utils.execute() just before its own dry-run check, the 
redundant dry-run check in Exec.execute() causes even that logging to be 
skipped, severely reducing the utility of dry-run mode to begin with (at least 
with <exec>).

-peter

Original comment by petenewc...@gmail.com on 20 Jul 2011 at 2:52

[GoogleCodeExporter]

yes, i have realized this bit later. thanks.

Original comment by kroko...@gmail.com on 20 Jul 2011 at 2:55

[GoogleCodeExporter]

Ah, just read the commit logs... I see that you figured that out already.

Thanks!

-peter

Original comment by petenewc...@gmail.com on 20 Jul 2011 at 2:57