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

[Fredrik Öhrström]

2013/1/10 Jonathan Gibbons <jonathan.gibbons at oracle.com>:
> 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.

Fixed.


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

Fixed.

> 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?

sjavac specific arguments are filtered away before the command line
is sent to the JavaCompilers. Thus any misspellt option will be caught.
A better and common option mechanism, that can be shared between
javac,jdeps and sjavac would be nice.

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

The Module is there to anticipate Modules in jdk9. The reason is that it is
alot of work retrofitting another level of abstraction on top of Package.
The current sjavac uses only a single module.

> Main.319 spelling subtelity

Fixed.

> Main 344, reference to System.err

Fixed.

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

Fixed.

> Module.java
> unsorted imports

Fixed.

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

The path component separator in an URI is always a /.
To make -sourcepath work, we have to expect that the uri path
ends with something that matches the package name.
This might work even when the URI begins with something else
than file:, however this is not tested.

> ResolveWithDeps, 60   don't understand comment

Fixed. (I hope.)

> SmartFileObject: 97
> what about windows newlines?

It seems like it has been working on Windows before. Will double check.

> 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.

Fixed.

> CompilerThread
> unsorted imports; strange mixture of javac imports and fully qualified names
> in the code.

Fixed, but I cannot find the qualified names. Please point to them.

> 205, 211, 217, etc
> These strings look like they should be constants

Fixed.

> 266 ugly subtle test for Windows -- why not be explicit and test os.name?

Fixed.

> JavacServer
> Lots of explicit references to System.err
> 436, 436, etc

Fixed.

> More strings that should be constants

Fixed.

> 589 spelling error disappeard

Fixed.

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

Fixed.

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

Because I do not want the stream from stderr/stdout to be piped back
to the Java program that created the ProcessBuilder. I need to have
stdout/stderr to end up on disk when the sjavac client terminates.
I cannot see that ProcessBuilder has this feature. Please correct me
if I am wrong.

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

Fixed. (Configurable enough, I hope.)

//Fredrik