Skip to content

Fixes for Windows native exec#6746

Merged
headius merged 6 commits intojruby:jruby-9.2from
headius:windows_exec_file_search
Jul 7, 2021
Merged

Fixes for Windows native exec#6746
headius merged 6 commits intojruby:jruby-9.2from
headius:windows_exec_file_search

Conversation

@headius
Copy link
Member

@headius headius commented Jul 6, 2021

This PR contains various small fixes for the native exec support on Windows, mostly relating to finding the full paths to cmd.exe and the .exe/.com/.bat/.cmd executables that go with a given command.

See #6745.

The exec logic calls through these methods to build a command line
that runs the windows cmd.exe to launch the desired executable.
Unfortunately this logic has two problems:

* The eventual jnr-posix `exec` implementation uses CreateProcess,
  which requires a full path to cmd.exe. Because all other paths
  use the JDK ProcessBuilder, we did not perform a search for this
  executable and the CreateProcess fails.
* The desired executable does run through the path and extension
  search logic, but the resulting discovered executable (such as
  .cmd or .bat) is not used in the cmd.exe command line.

This patch makes the following changes:

* Perform the executable search for cmd.exe, so we can provide a
  full path to CreateProcess
* Use the result of the executable search in place of the original
  desired executable name, so it will pick up non-exe files like
  .bat and .cmd.

See jruby#6745
@headius headius added this to the JRuby 9.2.20.0 milestone Jul 6, 2021
headius added 2 commits July 6, 2021 14:58
On Windows, the default logic (from rubygems/defaults/jruby) will
see that we are running the build from within the 9.1.17.0 jar
file, and produce .bat wrappers that attempt to run JRuby using
a `java` command line. This breaks those .bat files, as described
in jruby#6745, because the `java` command line gets completely wrapped
in quotes and no longer can be run as a single executable.

This patch un-patches the default Gem.ruby so that it goes back to
just using `jruby` as the command line to launch JRuby.

This is a pretty nasty hack only necessary because the build runs
the gem installation from within the 9.1.17.0 jar file. If the
installs were run from a normal JRuby command line (i.e. the
current working copy's bin/jruby) the wrappers are generated
correctly.
This argument to cmd.exe is passed directly as a self-contained
exec argv, so added quotes break cases where we are calling into
a native process-launching function like ProcessBuilder. Removing
the quoting does not appear to break non-native launching.
@headius headius force-pushed the windows_exec_file_search branch 2 times, most recently from a23e940 to 57aff6c Compare July 6, 2021 20:47
global_bin seems to not be readable from within the block under
some circumstances.
@headius headius force-pushed the windows_exec_file_search branch from 57aff6c to ed88810 Compare July 6, 2021 20:52
We parse out single-string commands along this path, which may
result in a single script with embedded spaces being broken into
individual args. Here we check that the original rawArgs length
matches the parsed args length so we know we will only replace the
argv[0] executable and not the entire script.
@headius headius force-pushed the windows_exec_file_search branch from 57092ef to 0184797 Compare July 6, 2021 21:51
@headius headius changed the base branch from master to jruby-9.2 July 7, 2021 16:20
@headius headius force-pushed the windows_exec_file_search branch from eb905a1 to f1ad815 Compare July 7, 2021 19:55
@headius
Copy link
Member Author

headius commented Jul 7, 2021

I have added a small test for this behavior, but it will only run in the master/9.3 builds. The 9.2 branch has not received @kares fixes for running test:jruby on Windows.

I did confirm the test works on 9.2 branch.

@headius headius merged commit 22c258f into jruby:jruby-9.2 Jul 7, 2021
@headius headius deleted the windows_exec_file_search branch July 7, 2021 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant