Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On Windows, -Dpljava.libjvmlocation packager-convenience option breaks build #190

Closed
jcflack opened this issue Oct 5, 2018 · 1 comment

Comments

@jcflack
Copy link
Contributor

jcflack commented Oct 5, 2018

Thanks to Ken Olson for reporting this.

Workaround

Do not use the -Dpljava.libjvmlocation option on the mvn command line when building for Windows. The option is intended as a convenience for package maintainers, but it is easily worked around. A Windows package with a customized default JVM location can be made by including a small patch file to apply to pljava-so/src/main/c/Backend.c and hardcode the desired value (as a C string literal) for the PLJAVA_LIBJVMDEFAULT macro.

Because of low impact, ease of workaround, and practical obstacles to a general fix, the workaround for this issue is recommended for the foreseeable future.

A possible future fix that generates the macro definition into an extra file to include could be cleaner than getting it passed intact on the compiler command line.

Analysis

PL/Java's build constructs a <define> element for the nar-maven-plugin to cause PLJAVA_LIBJVMDEFAULT to be predefined in the C compiler with the given value (in the form of a C string literal). The nar-maven-plugin puts the macro name and value in the form of a /D compiler option for the MSVC compiler and (in the version of the plugin currently selected) passes that to the Ant Execute task(1). The Ant task passes it to java.lang.Runtime.exec(2). So far, so good.

The problem comes in at the Windows platform-specific variant of the JDK's internal java.lang.ProcessImpl, which has the responsibility of quoting/escaping the discrete Runtime.exec parameters to form a single command-line string for the Windows CreateProcess call, such that the new process will correctly parse the string and recover the parameters intact.

The ProcessImpl code does not implement correct rules for doing so, and a parameter value in the form of a C literal (with double quotes that are significant and other escaping potentially needed inside) is a test case that proves the point.

In fairness to ProcessImpl, the task it is faced with doing is practically impossible. Windows does not have one common set of rules for how the command line is parsed(3). To prepare a properly-quoted/escaped command line requires knowledge of what program the created process will run, its language and perhaps even what version of the language's runtime library the target program is linked to.(4). The Java Runtime.exec and ProcessBuilder APIs have no way for the caller to supply this information (even if it could be known), and neither does the implementation find and examine the target executable to divine the information (though it does make a partial effort by checking whether the target name ends in .CMD or .BAT and applying the different rules that apply to those).(5)

The difficulties with the JDK implementation do not stop there. The exact quoting rules applied also depend on whether a SecurityManager is in use and, if not, on the value of a system property.(6) Related JDK bug reports tend to get "resolved" as "won't fix", on the ground that any existing code using these APIs must be fiercely applying workarounds already, and would break if they were fixed.(7) (The claim that "there is no place to put a method" with better behavior seems dubious, but then the past bug reports seem to have labored under the belief that one correct behavior could be found. By realizing that no generally-correct solution is possible for Windows, and that the rules to apply require knowledge of the runtime library linked into the specific program to be launched and whether it will be launched through CMD or not, there might be a fresh argument for extending ProcessBuilder with new methods with a parameter for the quoting rule (or stack of rules) to apply, and deprecating the legacy ones.)

As it is, Java code that needs to use the Runtime.exec or ProcessBuilder APIs on Windows, and work around their behavior, would need to work quite hard to do so. For the general case, it would need the runtime library information about the program to be executed, and whether it will be executed through CMD, and would also have to understand which of the different almost-right quoting algorithms will be applied in ProcessImpl, and somehow transform the original string so that the composition of those rules would leave it correct. I'll be surprised if anyone has implemented workaround code that doesn't fail in cases slightly different than it was designed for.

jcflack added a commit that referenced this issue Oct 6, 2018
Java 11 is out now, so replace the "expected to work in" with "works in".

Call attention to Oracle's licensing change in Java 11.

Clarify the considerations around building from unreleased snapshot
sources, and how to determine the branch with most recent activity.

Add a note on the SQL syntax needed to call the Saxon-based example
functions as one travels back through PostgreSQL versions.

Mention issue #190 and suggest a workaround for builders of packages
for Windows.
@jcflack
Copy link
Contributor Author

jcflack commented Oct 19, 2020

Believed resolved in 1.6.0.

@jcflack jcflack closed this as completed Oct 19, 2020
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

No branches or pull requests

1 participant