Review request: 8004658: Add internal smart javac wrapper to solve JEP 139

[Jonathan Gibbons]

More feedback,

Looks ok, but ...

Log explicitly uses System.err and System.out.   Better to create and 
use an instance of Log that contains references to the streams (or 
writers) to be used.  Allow the top level method to specify these 
streams.  If nothing else, this makes it easier to integrate this 
package into other systems, including tests.

Main.main:line 177
No need to create Main with --startserver:

Messages should eventually be localized

Main:221,225, etc -- use of undocumented option. Would be nice to see a 
better arg decoding mechanism. If particular, with the haphazard "find" 
mechanisms you have, do unrecognized options get detected anywhere?

Confusing to see names like Module -- you gotta know there will be name 
classes in JDK 9.

Main.319 spelling subtelity

Main 344, reference to System.err

Main 389, 401 -- cut n paste error -- comment needs fixing

Module.java
unsorted imports

JavaCompilerWithDeps
95-97  What if it's not a fie: URI?

ResolveWithDeps, 60   don't understand comment

SmartFileObject: 97
what about windows newlines?

SmartWriter:70
In this and other places, what about exceptions before close() is called 
-- this will leave the file open on Windows, which is bad. In general, 
in all places in sjavac where you call FileWriter.close(), consider 
using try-with-resources.

CompilerThread
unsorted imports; strange mixture of javac imports and fully qualified 
names in the code.
205, 211, 217, etc
These strings look like they should be constants
266 ugly subtle test for Windows -- why not be explicit and test os.name?

JavacServer
Lots of explicit references to System.err
436, 436, etc
More strings that should be constants
589 spelling error disappeard

Main 678
Why only a directory after -*path -- why not a path, as per javac

  JavacServer, in fork, why not use ProcessBuilder and 
setErrorRedirect(true) to handle redirection, rather than using /bin/sh 
or cmd?

JavacServer
I see a bunch of numbers that would be better as constants.  5, 125, 
1000, 2000, 120000, etc -- and maybe configurable.

test.sh
I saw a reference somewhere that sjavac was in tools.jar, is that right?
test.sh does not run with a standard "langtools only" build -- compile 
langtools (incl sjavac) then run with classes on bootclasspath -- this 
is another problem with test.sh as a script, not as a Java program.
This is sort-of a showstopper.  To fit in with standard langtools dev 
workflow, it must be possible to
     * build all langtools classes
     * jtreg -jdk:/recent/build -Xbootclasspath/p:build/classes 
-ignore:quiet -samevm test/

-- Jon

On 01/08/2013 06:32 AM, Fredrik Öhrström wrote:
> And here is an updated webrev, with cleanups and more tests.
>
> http://cr.openjdk.java.net/~ohrstrom/webrev-8004658-sjavac-v2/
>
> //Fredrik
>
> 2012/12/6 Fredrik Öhrström <fredrik.ohrstrom at oracle.com>:
>> This the smart javac wrapper that makes use of the javac hooks for reporting
>> dependencies.
>> The easiest way to test it, is to clone build-infra and use --enable-sjavac
>> with configure.
>>
>> After building, do "export PATH=build/.../jdk/bin:$PATH"
>> then you can do:
>> sjavac src -d bin --server:portfile=/tmp/myport
>>
>> The more sources you have under the src directory, the merrier. With enough
>> sources,
>> it will actually use multiple cores. Run it again, and nothing will happen,
>> since it
>> quickly determines that all artifacts are up to date. (add -h headers if you
>> expect
>> native method headers to be generated.)
>>
>> Try changing public methods, and recompile, dependent packages will be
>> recompiled.
>> Try changing private or package private methods, and recompile, only that
>> package will be recompiled.
>> Try changing a native method, the header file will be regenerated.
>>
>> The smart javac wrapper is not part of the final image. (The sjavac launcher
>> and its classes
>> in the tools.jar are explicitly stripped from the image.) It will remain a
>> build tool, while we
>> experiment and improve its multi core performance and evaluate how it should
>> integrated into the full product.
>>
>> The real test is of course, to add "public void helloWorld() {}" to
>> Object.java and
>> do "make LOG=info". Or add a native method to Adler32.java and watch
>> how the build system recompiles libzip.so (remember to do "make LOG=info")
>> to see the incremental build properly.
>>
>> http://cr.openjdk.java.net/~ohrstrom/webrev-8004658-sjavac/
>>
>> //Fredrik