Make javac run in a subprocess. Fixes #809. #886

Merged
merged 2 commits into from Dec 14, 2012

3 participants

@hyPiRion
Collaborator

If we're not making javac run in a subprocess, libraries the project
depends on which leiningen has another version of will clash. This is
because leiningen append its standalone to java's bootclasspath, which
makes the ToolProvider add these classpaths automatically into the java
compiler it returns. By starting a subprocess without leiningen added to
the bootclasspath, we avoid this library clashing.

UI changes as a result of this patch: Whenever javac fails, the task one
wanted to run will be responsible for aborting Leiningen, whereas javac
did this itself when it discovered that the compilation failed. As we're
running in a subprocess, an abort message such as

Uberjar aborting because jar/compilation failed: Subprocess failed

will appear, though this will only be appended to the javac error
message and the "Compilation of Java sources (lein javac) failed."
message.

This commit also adds the overhead of starting a subprocess when one
have to compile java sources, but it will not start one if no
.class-files are outdated/not existing.

Not tested on Windows, though unless leiningen.core.eval does some unknown magic with subprocesses, this shouldn't be an issue.

@hyPiRion hyPiRion Make javac run in a subprocess. Fixes #809.
If we're not making javac run in a subprocess, libraries the project
depends on which leiningen has another version of will clash. This is
because leiningen append its standalone to java's bootclasspath, which
makes the ToolProvider add these classpaths automatically into the java
compiler it returns. By starting a subprocess without leiningen added to
the bootclasspath, we avoid this library clashing.

UI changes as a result of this patch: Whenever javac fails, the task one
wanted to run will be responsible for aborting Leiningen, whereas javac
did this itself when it discovered that the compilation failed. As we're
running in a subprocess, an abort message such as

    Uberjar aborting because jar/compilation failed: Subprocess failed

will appear, though this will only be appended to the javac error
message and the "Compilation of Java sources (lein javac) failed."
message.

This commit also adds the overhead of starting a subprocess when one
have to compile java sources, but it will not start one if no
.class-files are outdated/not existing.

Not tested on Windows.
377a98f
@technomancy
Owner

Thanks. I agree that using a subprocess is the right thing to do here; I didn't understand the interaction with the bootclasspath when this was first implemented.

A couple notes:

0) On line 102 it checks to make sure we have the Java compiler in Leiningen's process. This isn't quite right since JAVA_CMD and LEIN_JAVA_CMD can point to different executables, meaning Leiningen can be run with a JRE and the project run with a JDK and things should still work. So this check should be moved inside the eval-in-project call.

1) The codebase currently doesn't use any prefix-style :require clauses; for consistency could you replace those with full namespace names? This makes it easier to use grep to see where a namespace is being consumed.

2) If you add ^:displace metadata to the Clojure dependency inside the :dependencies vector it will ensure that the project's specified version of Clojure isn't overridden by this profile.

Thanks for submitting this change; would be happy to merge it once the above considerations are addressed.

@technomancy
Owner

Though it occurs to me that since I don't actually use the javac task it might be helpful to get a review from @Raynes or @michaelklishin as well to see if they have thoughts.

@Raynes
Collaborator

I think this is fine, but I hate having to do the subprocess stuff. Might want to look into just shelling out at some point and see how hard/reliable it would be to do it with conch instead. It'd certainly be faster.

@technomancy
Owner

You mean it would be faster to shell out to javac in a way that didn't require loading Clojure code? That's probably true. Still, this approach is much better than what we currently have.

If we do decide to hit javac directly we can probably do it with the same subprocess functions that eval-in-project is built upon; we already roll our own sh function.

@hyPiRion hyPiRion Check compiler existence within javac subprocess.
Also added ^:displace to avoid adding multiple clojure-versions onto the
classpath, and fully expanded non-`clojure.core` namespaces.
d38ecd3
@hyPiRion
Collaborator

Re: 1) - not entirely sure how thorough you wanted me to be there. I've not expanded clojure.core functions and macros into clojure.core/foo, but every other namespace is now fully expanded.

The other issues have been resolved.

@technomancy technomancy merged commit 3a1f84f into technomancy:master Dec 14, 2012
@technomancy
Owner

Never mind; easier for me to fix it than explain what I'm talking about. =)

Thanks.

@technomancy
Owner

Prefix list removal in :require: 9cceba8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment