Review request: 8004658: Add internal smart javac wrapper to solve JEP 139 (original) (raw)

Jonathan Gibbons jonathan.gibbons at oracle.com
Wed Jan 9 17:37:45 PST 2013

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

More information about the compiler-dev mailing list