I would like to propose a few API changes needed for the Compile on Save feature, notably:
-org.netbeans.spi.java.project.runner.ProjectRunner that allows direct execution of Java classes, using the compile on
-ability to listen on .class file writes done by the Java infrastructure to the output folder of projects
Please see the attached patch.
Created attachment 63610 [details]
Asking for review.
There seem to be two unrelated API changes here, which makes it harder to review. Please use separate issues in the
future for independent changes. Also I think this is too significant to qualify as a fast-track review.
[JG01] Fix <issue number="999999"/>, and add to java.source/apichanges.xml.
[JG02] I don't like the idea of java.source having a dep on Ant. Anyway why are you doing this? I thought Ant builds
would be left alone. What about e.g. Maven projects? Where is java.source/antsrc? There is no explanation of this change.
[JG03] What is ProjectRunner doing in an "spi" package? It is clearly an API, not an SPI.
[JG04] Why are you using Ant to run classes? Seems like a poor decision. <java> is really not good at running
interactive Java programs; it is designed for batch build scripts, with at best line-oriented ("cooked") I/O. Anyway
even if the implementation of a given command happens to use Ant as a quick-and-dirty implementation for 6.5, this
should certainly not be exposed in the SPI; it should be a detail of the SPI provider, which should be given a proper
interface (thus fixing the hacky checkSupported attribute as well).
[JG05] toString(ClassPath) can just be replaced with ClassPath.toString().
[JG06] Quick test actions need to support JVM arguments. All actions probably need to support a CWD, and perhaps
JG05 - rather, toString(PathConversionMode.FAIL), I think.
[MK1] +1 on jesse's suggestion not using ant.
[MK2] do you have an example code on how the ProjectRunner is to be used?
[MK3] the SPI providers for ProjectRunner (those who provide build script snippets) seem to register their impls at
"executor-snippets/" + actionName + ".xml". Will this work for all different project types that might have a different
idea about what "run" means?
[MK4] BuildArtifactMapper.addArtifactsUpdatedListener comment states that there is this contract: "The files in the
output folder are updated only if file <code>.netbeans_automatic_build</code> exists inside the output folder.". Is this
piece of information/API stated someplace else as well. It seems a bit hidden here.
Thanks for the comments.
Regarding full vs. fast review, I propose the following: we will integrate on Monday, and we will do full review
afterwards. Any objections? I would like to ask Jesse, Milos, Tomas Z. and Petr H. to be the reviewers. Please speak up
if you do not wish to be a reviewer.
JG01: will do for java.source. 999999 was intentionally used as a placeholder.
JG02: if we make a Java-level SPI for ProjectRunner, and if we use ant as a backend for actual execution of the projects
(which we will need at least for NB6.5, I think), we will need to place the implementation of the SPI of the
ProjectRunner into a module that sees both java.source internals and ant - I do not see any problem with using
java.source for this purpose. All J2SE-like projects should use ProjectRunner to quick-run the project (which will run
an ant script internally). Web-like projects are currently supposed to use their own ant script. The java.source/antsrc
contains two ant tasks: private TranslateClassPath (used by the ant code snippets) and override for Javac task for
Web-like projects (this task will copy .class files from the java.source internal caches into the target folder or run
the original javac task depending on the ensure.built.source.roots property). The java.source/antsrc directory can be
(sorry I did not include this link at the time I filled the issue, but the repository was broken at that time, and it
was not possible to push into it, so the content of the repository was obsolete).
JG03: spi looked slightly better for me, as it should be used by the project types (which usually use the SPI part of
the java.project module). But I have no problem with moving it into an API package.
JG04: ant module currently provides a lot of services (Rerun/Stop actions in the menu, correct handling of I/O tabs,
stack trace hyperlinking, binding to JUnit and debugger). Without ant, we would either need to duplicate a lot of code,
or have an API to share this code (which is out-of-scope for the Compile-on-Save feature, IMO). So, at least for NB6.5,
I propose to stick with the ant approach. I am fine and agree with separating the API/SPI and hiding this as an impl.
detail and rewriting the SPI impl to direct execution when possible.
MK1: answered above I hope
MK2: e.g. bypassAntBuildScript in:
MK3: probably depends on what is the exact meaning of "run" for the given project. Should be roughly equivalent to
direct execution of the given class with given parameters.
MK4: I will add a note into java.source's arch.xml.
Thanks again for the comments.
I am fine with being a reviewer.
To JG02: I am still -1 on having java.source depend on Ant. This would be an unwanted architectural dependency. Not an
issue for the standard NB IDE but may be an issue for others. Would suggest a new module containing just this SPI impl,
though adding to java.project may also be a possibility.
To JG03: you are offering methods people call - that is an API. If you offer interfaces people can implement and
register, that would be an SPI. If you offer convenience impls of an SPI, that would be a support SPI. If you offer
convenience utility methods around the raw API, that would be a support API.
To JG04: the more generic parts of execution should be handled by the extexecution API going forward; starting the
debugger directly is already possible using debugger-specific APIs; stack trace hyperlinking and JUnit window
integration should in the future be handled by a Java-specific execution API, I guess. Direct use of java.lang.Process
will in the future not only greatly simplify standard I/O stream handling, but also offer us new feature possibilities
which are impossible when using <java> (e.g. direct connection of JConsole or similar Visual VM features to a running
program). So I agree with using Ant as a backend for 6.5 so long as this is concealed from the APIs.
JG02: Ok. I have introduced java.source.ant to bind java.source and ant:
JG03: There is api.java.project.runner.ProjectRunner and spi.java.project.runner.ProjectRunnerImplementation now:
Based on the experiences with the ProjectRunner so far, I would like to adjust the API by changing the signatures of the
ProjectRunner methods to:
public static boolean isSupported(String command, Map<String, ?> properties);
public static void execute(String command, Map<String, ?> properties);
This mainly means that the FileObject parameter was removed, and that the values of properties can be of any type (have
to be Strings in the current API). This would allow (among others) allow to execute a library class from the main
project (that depends on the library) - issue #139404.
I am attaching minimal patch showing only the API changes and the changes in the implementation of the
ProjectRunnerImplementation. Full patch would of course adjust apichanges, inter-project dependencies, all callers of
Any opinions? Thanks.
Created attachment 69426 [details]
If you make this change I guess I will be unable to release one version of autoproject.java which works on both the beta
and RC, but oh well.
[JG07] - the PRI impl could report that a command is not supported if it is passed unusable params, e.g.
applications.args for test. Not very important though.
[JG08] So test-sys-prop.* are gone from the API? Meaning java.j2seproject etc. will need to convert these to -Dk=v JVM
[JG09] I would like to see constants for the supported properties. Even better, replace Map<String,?> with a final class
containing properly typed and documented setters. Friendlier for code completion, and setters could throw IAE if you
e.g. attempted to set both a file to run and a class name to run. This class could even implement some of the generic
logic now in PRI but specified in the API, such as extracting a default class name and run CP from a FileObject.
JG09 seems partially implemented - there are now PROP_* constants, but they have no Javadoc: descriptions need to be
moved out of the class header. Same for the three new QUICK_* constants.
There is no constant for "methodname" (cf. SingleMethod, as used in e.g. autoproject.java.actions.ActionProviderImpl).
[JG10] Besides "methodname", apparently "stopclassname" is an undocumented property which is recognized by the debug action.
Integrated into 'main-golden', will be available in build *200809301401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
User: Jan Lahoda <firstname.lastname@example.org>
Log: #138504: implementing most comments from the review.
Integrated into 'main-golden', will be available in build *200810010201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
User: Jesse Glick <email@example.com>
Log: apichanges update for #138504.
apisupport.apidocs/build/javadoc/org-netbeans-modules-java-project/apichanges.html:313:78: Broken link: file:apisupport.apidocs/build/javadoc/org-netbeans-modules-java-project/org/netbeans/api/java/project/runner/ProjectRunner.html
isn't this one already integrated? can it be closed?
I think it is.