Minor regression: Kernel.system('non-existing-file') produces some output but shouldn't

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: JRuby 1.4.0RC3
  • Fix Version/s: JRuby 1.5
  • Component/s: Core Classes/Modules
  • Labels:
    None
  • Environment:
    Latest JRuby 1.1RC2, rev 6111, Ubuntu Linux

Description

A bugfix for JRUBY-1557, revision 6111, introduced a minor regression on Kernel#system behavior:

system('non-existing-file')

returns false on MRI and no output, while it returns false on JRuby, but with additional output:
/bin/sh: non-existing-file: not found

This is visible when running the rubyspecs, with dotted formatter, and not only dots are printed, but something like that:
..../bin/sh: sad: not found
..................

Issue Links

is related to

Bug - A problem which impairs or prevents the functions of the product. JRUBY-1557 Using backquotes to run commands with shell redirects sometimes passes the redirects to the called program

  • Blocker - Blocks development and/or testing work, production could not run
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Riley Lynch added a comment -

This is because MRI looks up the executable on the PATH before attempting to run it, whereas jruby passes the argument array directly to the shell for execution, without first verifying whether a matching executable can be found on the PATH.

This can be fixed at the same time as JRUBY-2253 by making it so that jruby also searches the PATH before executing, raising Error::ENOENT if no matching executable is found.

Show
Riley Lynch added a comment - This is because MRI looks up the executable on the PATH before attempting to run it, whereas jruby passes the argument array directly to the shell for execution, without first verifying whether a matching executable can be found on the PATH. This can be fixed at the same time as JRUBY-2253 by making it so that jruby also searches the PATH before executing, raising Error::ENOENT if no matching executable is found.
Hide
Permalink
Riley Lynch added a comment -

Clarification: MRI doesn't raise Error::ENOENT for system() in the case that no executable is found – only in the case of exec().

Show
Riley Lynch added a comment - Clarification: MRI doesn't raise Error::ENOENT for system() in the case that no executable is found – only in the case of exec().
Hide
Permalink
Riley Lynch added a comment -

See patch attached to JRUBY-2253.

Show
Riley Lynch added a comment - See patch attached to JRUBY-2253.
Hide
Permalink
Charles Oliver Nutter added a comment -

This would be great to fix, if only to eliminate the annoying "sad" message when running rubyspecs. Waiting for follow-up on the JRUBY-2253 patch, which is excellent but introduces a testEnv failure.

Show
Charles Oliver Nutter added a comment - This would be great to fix, if only to eliminate the annoying "sad" message when running rubyspecs. Waiting for follow-up on the JRUBY-2253 patch, which is excellent but introduces a testEnv failure.
Hide
Permalink
Riley Lynch added a comment -

The JRUBY-2253 patch was updated to resolve the testEnv failure.

Show
Riley Lynch added a comment - The JRUBY-2253 patch was updated to resolve the testEnv failure.
Hide
Permalink
Charles Oliver Nutter added a comment -

I fixed this recently because I was sick of the extra output during spec runs.

Show
Charles Oliver Nutter added a comment - I fixed this recently because I was sick of the extra output during spec runs.
Hide
Permalink
Vladimir Sizikov added a comment -

Unfortunately, the bug is still present in 1.4 and master branches.

The reason is that the Charlie's fix has been reverted in rev. 5ad739088d9971b93b5f94cea64a832777a20f86, with the following comment:

"This reverts commit 14f69b67b7710129a0fa099be611360df1f78016. Reverted for 1.3 series only."

Show
Vladimir Sizikov added a comment - Unfortunately, the bug is still present in 1.4 and master branches. The reason is that the Charlie's fix has been reverted in rev. 5ad739088d9971b93b5f94cea64a832777a20f86, with the following comment: "This reverts commit 14f69b67b7710129a0fa099be611360df1f78016. Reverted for 1.3 series only."
Hide
Permalink
Vladimir Sizikov added a comment -

Target for 1.4?

Show
Vladimir Sizikov added a comment - Target for 1.4?
Hide
Permalink
Charles Oliver Nutter added a comment -

The fix I made was too much...it caused valid stderr output to be swallowed as well as output we did not want to see. We would need to investigate how MRI chooses to handle stderr streams from missing programs when calling Kernel#system.

Show
Charles Oliver Nutter added a comment - The fix I made was too much...it caused valid stderr output to be swallowed as well as output we did not want to see. We would need to investigate how MRI chooses to handle stderr streams from missing programs when calling Kernel#system.
Hide
Permalink
Charles Oliver Nutter added a comment -

This is complicated. What I know is this:

  • We should not swallow stderr output from a successfully-started subprocess. That's the original reason we reverted the commit.
  • MRI does a full manual search of PATH with the command specified to see in advance whether it will fail or succeed. This allows them to avoid trying to run it (usually through sh or cmd) and having an error message result.

The code for this searching appears to be in dln.c/dln.h in MRI's source, with most entry points starting at dln_find_exe.

There appears to be no simple out-of-the-box way to determine if an exec will find a binary to run on Java, so this is not going to make 1.4.

Show
Charles Oliver Nutter added a comment - This is complicated. What I know is this:
  • We should not swallow stderr output from a successfully-started subprocess. That's the original reason we reverted the commit.
  • MRI does a full manual search of PATH with the command specified to see in advance whether it will fail or succeed. This allows them to avoid trying to run it (usually through sh or cmd) and having an error message result.
The code for this searching appears to be in dln.c/dln.h in MRI's source, with most entry points starting at dln_find_exe. There appears to be no simple out-of-the-box way to determine if an exec will find a binary to run on Java, so this is not going to make 1.4.
Hide
Permalink
Vladimir Sizikov added a comment -

Will take a look into this in more depth, since Charlie is busy with other things

Show
Vladimir Sizikov added a comment - Will take a look into this in more depth, since Charlie is busy with other things
Hide
Permalink
Thomas E Enebo added a comment -

Not making 1.4 bumping out of 1.4...

Show
Thomas E Enebo added a comment - Not making 1.4 bumping out of 1.4...
Hide
Permalink
Vladimir Sizikov added a comment -

Resolved in cc4b619, along with the major refactoring of ShellLauncher and with the great help of Riley Lynch's patch.

Show
Vladimir Sizikov added a comment - Resolved in cc4b619, along with the major refactoring of ShellLauncher and with the great help of Riley Lynch's patch.

People

  • Assignee:
    Vladimir Sizikov
    Reporter:
    Vladimir Sizikov
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: