This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
Summary: | API changes to support the Compile on Save feature | ||
---|---|---|---|
Product: | java | Reporter: | Jan Lahoda <jlahoda> |
Component: | Source | Assignee: | apireviews <apireviews> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | dkonecny, jglick, mkleint, phejl, tzezula |
Priority: | P2 | Keywords: | API, API_REVIEW_FAST |
Version: | 6.x | ||
Hardware: | All | ||
OS: | All | ||
URL: | http://wiki.netbeans.org/CompileOnSave | ||
Issue Type: | TASK | Exception Reporter: | |
Bug Depends on: | 160649 | ||
Bug Blocks: | 139404 | ||
Attachments: |
API diff.
Proposed change. |
Description
Jan Lahoda
2008-06-27 16:56:28 UTC
Created attachment 63610 [details]
API diff.
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 environment variables. 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 accessed here: http://hg.netbeans.org/jet-cos/file/tip/java.source/antsrc/ (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. JG05: OK. JG06: OK. MK1: answered above I hope MK2: e.g. bypassAntBuildScript in: http://hg.netbeans.org/jet-cos/file/b2aed9c80e88/apisupport.project/src/org/netbeans/modules/apisupport/project/ui/ModuleActions.java 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: http://hg.netbeans.org/jet-cos/file/tip/java.source.ant/ JG03: There is api.java.project.runner.ProjectRunner and spi.java.project.runner.ProjectRunnerImplementation now: http://hg.netbeans.org/jet-cos/file/tip/java.project/src/org/netbeans/api/java/project/runner/ProjectRunner.java http://hg.netbeans.org/jet-cos/file/tip/java.project/src/org/netbeans/spi/java/project/runner/ProjectRunnerImplementation.java 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 ProjectRunner, etc. Any opinions? Thanks. Created attachment 69426 [details]
Proposed change.
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 args. Good. [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) Changeset: http://hg.netbeans.org/main/rev/3ca62d75a91b User: Jan Lahoda <jlahoda@netbeans.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) Changeset: http://hg.netbeans.org/main/rev/a015df57a7e0 User: Jesse Glick <jglick@netbeans.org> Log: apichanges update for #138504. http://deadlock.netbeans.org/hudson/job/nbms-and-javadoc/2180/testReport/org.netbeans.nbbuild/CheckLinks/testBrokenLinks/ 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. |