Bug 50781 - javac task creates long command line
Summary: javac task creates long command line
Status: NEW
Alias: None
Product: Ant
Classification: Unclassified
Component: Core tasks (show other bugs)
Version: 1.8.3
Hardware: PC Windows XP
: P2 critical (vote)
Target Milestone: ---
Assignee: Ant Notifications List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-15 07:40 UTC by kunszabo67
Modified: 2017-07-07 20:05 UTC (History)
4 users (show)



Attachments
Patch to fix Bug 50781 (1.55 KB, patch)
2011-12-22 04:51 UTC, Justin Ryan
Details | Diff
Patch for discussion (not yet tested) (7.82 KB, patch)
2012-05-22 18:53 UTC, Jesse Glick
Details | Diff
Jesse Glick's patch with default value as 1 (7.82 KB, patch)
2012-07-05 03:31 UTC, Justin Ryan
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description kunszabo67 2011-02-15 07:40:00 UTC
The javac task does not put all long list-like command line arguments (classpath, etc.) into a response file, just the list of source files to be compiled.

This can create command lines when fork="true" that are too long for the host OS if there are too many entries in 
-classpath,
-sourcepath,
-bootclasspath,
-extdirs and
-endorseddirs.
Comment 1 kunszabo67 2011-02-15 08:31:01 UTC
The same is true for java task, too.
Comment 2 willem.verstraeten 2011-10-05 12:45:30 UTC
also true for the JUnit task
Comment 3 Justin Ryan 2011-12-22 04:51:14 UTC
Created attachment 28096 [details]
Patch to fix Bug 50781

The trouble maker for this bug is the -classpath argument which can grow without control and surpass the 32K buffer allowed by Windows. The current logic only protect a long source file list. The fix includes all parameters into the @ temp file. I've tested locally and it works fine.
Comment 4 Stefan Bodewig 2011-12-28 07:48:11 UTC
The reason the code works the way it does is that many implementations of javac (the code is shared) do not support arbitrary arguments in the response file, only source files.  IIRC this has been true for Sun's javac until 1.3 or 1.4 as well.

In order to make use of the patch it has to be guarded by some kind of conditional that ensures we don't feed additional args to jikes or Microsoft's jvc.  We may even need an attribute to control it for the JDK's javac as people may be using older javacs in forked mode.
Comment 5 Justin Ryan 2012-05-22 17:33:36 UTC
Does javac 1.3 and 1.4 have to be supported? This code only runs if the command line is too long, which means they've had this error for ages and haven't filed any bugs.  I'm not familiar enough with the Ant codebase to know how to conditionalize this code to only with with javac > 1.4. If someone could tell me that, I could submit another patch.
Comment 6 Stefan Bodewig 2012-05-22 17:52:11 UTC
DefaultCompilerAdapter has a combo of assumeJava1x methods you can use, something like !assumeJava13 && !assumeJava14
Comment 7 Jesse Glick 2012-05-22 18:18:52 UTC
Jikes's home page has not been updated in seven years, and jvc is ancient history. We should optimize for the problem affecting users of current tools; @args works for me with javac 1.4.2b19 and later (*), and also with ecj 3.5.1. And remember that you can use a current javac and pass -source 1.2 -bootclasspath ... to build for an old platform without actually using a compiler binary from the previous millenium.

If there is a way to automatically detect that an obsolete compiler is in use, great, but I am not sure what that would be, especially if it is just known that C:\something\bin\javac.exe is to be invoked. If we need to retain compatibility here, we should default to the behavior in the patch, but make it possible to restore the prior behavior using a project property (or, if necessary, task attribute).

(In reply to comment #5)
> This code only runs if the
> command line is too long, which means they've had this error for ages

My understanding is that if you are compiling a large number of source files with a short classpath using a very old compiler, the 1.8.3 task will work but the task with the proposed patch will fail.

(*) JDK 1.3 can no longer be made to run on Linux without great effort.
Comment 8 Jesse Glick 2012-05-22 18:28:51 UTC
(In reply to comment #6)
> DefaultCompilerAdapter has a combo of assumeJava1x methods

These are useless for this purpose: if you run <javac fork="true" executable="..."/>, DCA.assumeJavaXY will just assume whatever version of Java Ant is running on, which for 1.8.3 will necessarily be 1.4+.
Comment 9 Jesse Glick 2012-05-22 18:39:52 UTC
BTW the patch as it stands is wrong, since it ignores the documented firstFileName parameter. What you meant to do is adjust the doc for this parameter to indicate that it could in fact be zero even when there are some options; and change callers (esp. JavacExternal but also perhaps AptExternalCompilerAdapter) to pass zero by default rather than cmd.size().

(The patch also, curiously, behaves as if firstFileName=1 rather than =0: it copies over the first real arg unchanged, then adds the @rest. This could fail in case the first arg happened to be something very long, such as -J-Xbootclasspath/p:/some/long/path/to/compiler/patch.jar.)
Comment 10 Jesse Glick 2012-05-22 18:53:06 UTC
Created attachment 28819 [details]
Patch for discussion (not yet tested)
Comment 11 Jesse Glick 2012-05-22 18:54:32 UTC
<java> and <junit> (comment #1, comment #2) would also need to be investigated.
Comment 12 Justin Ryan 2012-07-05 02:35:28 UTC
(In reply to comment #9)
> BTW the patch as it stands is wrong, since it ignores the documented
> firstFileName parameter.

I agree that my origin patch wasn't ideal and is broken in respect to firstFileName. 

> (The patch also, curiously, behaves as if firstFileName=1 rather than =0: it
> copies over the first real arg unchanged, then adds the @rest. This could
> fail in case the first arg happened to be something very long, such as
> -J-Xbootclasspath/p:/some/long/path/to/compiler/patch.jar.)

Your patch while much better it is lacking in one respect, it could potentially create commandArray of size 1 with just the name of the @tmpfile, which isn't an executable. You need to maintain index 0, since it is pointing to the actual javac.exe (that's one thing my patch got right). Keep in mind that commandArray  is cmd in CommandLaucher.exec and eventually becomes Runtime.getRuntime().exec(cmd, env). I believe you're assuming the first element in the array is the first command line argument.

Previous to this change, firstFileName would never practically been 0, so the lurking bug of setting commandArray with a size of 1 would never happen. I think if you change this:

Project.toBoolean(project.getProperty("pre-50781")) ? cmd.size() : 0;

to:

Project.toBoolean(project.getProperty("pre-50781")) ? cmd.size() : 1;

You'll avoid the problem, and the it will fix this defect.
Comment 13 Justin Ryan 2012-07-05 03:31:53 UTC
Created attachment 29031 [details]
Jesse Glick's patch with default value as 1

Default to a value which will skip the executable at index zero.
Comment 14 Trejkaz (pen name) 2015-03-10 04:55:07 UTC
We're seeing this break our builds at <junit>, but I noticed after only a bit of investigation that java.exe doesn't have a way to move command-line arguments to a file anyway, so unless Ant builds its own alternative launcher for Java, this is going to be difficult to work around.
Comment 15 David Turner (Two Sigma) 2017-07-07 20:05:11 UTC
I hit this bug recently.  Unfortunately, the patches won't work for me; my arguments are "-Xblahblahblah", "MyCompilerClass", "-classpath", "[too long]".  As I understand it, for javac, the class argument may not be read from an @file.  As other comments imply, this problem is not solvable in general; arbitrary compilers may have arbitrary spilling rules.  But one possibility is to only start spilling at 4096 minus N characters, where N is the length of the temp filename; this is the best we can do, and if it breaks, then there was probably no way to salvage the situation.