Bug 179749 - API changes in (generic) Java and Java project APIs to support annotation processors
API changes in (generic) Java and Java project APIs to support annotation pro...
Status: RESOLVED FIXED
Product: java
Classification: Unclassified
Component: Classpath
6.x
All All
: P3 (vote)
: 6.x
Assigned To: Tomas Zezula
issues@java
http://lahoda.info/hg/nb-patches/raw-...
:
Depends on: 179896 179859 180033 180034 181419 181421 181423
Blocks: 111065
  Show dependency treegraph
 
Reported: 2010-01-21 06:35 UTC by Jan Lahoda
Modified: 2010-12-07 13:23 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
API diff. (37.76 KB, patch)
2010-01-21 21:48 UTC, Jan Lahoda
Details | Diff
Updated API patch. (38.66 KB, patch)
2010-01-25 03:29 UTC, Jan Lahoda
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Lahoda 2010-01-21 06:35:31 UTC
I would like to propose enhancements to the generic Java and Java project APIs to support running annotation processors during initial scan and inside editor. API patch is here:
https://lahoda.info/hg/nb-patches/raw-file/tip/apt-api
Current implementation (non-API) patches are:
https://lahoda.info/hg/nb-patches/raw-file/tip/apt-j2se-project
https://lahoda.info/hg/nb-patches/raw-file/tip/apt-dbaleks-patch
(in this order).
Comment 1 Jan Lahoda 2010-01-21 07:01:47 UTC
Thanks for review.
Comment 2 Jesse Glick 2010-01-21 15:22:42 UTC
I get a 401 - Unauthorized from your server.
Comment 3 Jan Lahoda 2010-01-21 21:48:19 UTC
Created attachment 93471 [details]
API diff.
Comment 4 Jan Lahoda 2010-01-21 21:51:25 UTC
Sorry for that - the links should use "http" instead of "https":
API patch:
http://lahoda.info/hg/nb-patches/raw-file/tip/apt-api
Current implementation for J2SE Project:
http://lahoda.info/hg/nb-patches/raw-file/tip/apt-j2se-project

I have also attached the current version of the API patch to this bug.
Comment 5 Jesse Glick 2010-01-22 13:02:39 UTC
[JG01] apichanges neglects to mention AptQueryImplementation.


[JG02] annotationProcessorsToRun Javadoc should clarify that it is referring to PROCESSOR_PATH when it says "on the ClassPath".


[JG03] Return type of annotationProcessorsToRun is not clearly documented. Canonical names of processors? Binary names? Any reason this is not a Set<String>?


[JG04] Usage of the term "APT" in various APIs may be confusing since I guess this applies only to JSR 269 annotation processors, not APT which was introduced in JDK 5 and deprecated in JDK 6.


[JG05] APT_RUN_ALL_PROCESSORS seems unnecessary; couldn't you just check for APT_PROCESSORS_LIST being empty?


[JG06] j2seproject should pass '-s build/generated-sources/apt' (or whatever name you pick for the last path component), as it is useful to be able to see generated source files.


[JG07] AptUtils.getProcessorNames could probably be replaced by a call to Lookups.metaInfServices in case aptOptions.annotationProcessorsToRun() == null.


[JG08] Consider using ExecutionEngine.createPermissions in your class loader as it will ensure that any stdio (e.g. Throwable.printStackTrace) from annotation processors will be sent to an InputOutput rather than the NB log. (You can make the InputOutput be hidden unless and until something is printed to it.)

Currently ExecutionEngine.createPermissions is not public, though this can be worked around using a dummy subclass of NbClassLoader, or it could just be made public.

Alternately, improve ClassPath.getClassLoader to accept a ClassLoader, InputOutput, and PermissionCollection (you do not want AllPermissions for this purpose). No cache param in this case, and no listener in ClassLoaderSupport. It would also be best if a new constructor were added to NbClassLoader using URL[] rather than FileObject[]; FileObject is not used internally (or does not need to be).
Comment 6 Jan Lahoda 2010-01-25 03:27:50 UTC
(In reply to comment #5)
Thanks for the comments.

Re JG01, JG02, JG03: I tried to improve the documentation.

> [JG03] Return type of annotationProcessorsToRun is not clearly documented.
> Canonical names of processors? Binary names? Any reason this is not a
> Set<String>?

Iterable<? extends String> should be enough for the clients. But if there is a strong opinion that is should be Set<? extends String>, I have nothing against that.

> 
> 
> [JG04] Usage of the term "APT" in various APIs may be confusing since I guess
> this applies only to JSR 269 annotation processors, not APT which was
> introduced in JDK 5 and deprecated in JDK 6.

Would AnnotationProcessingQuery be better?

> 
> 
> [JG05] APT_RUN_ALL_PROCESSORS seems unnecessary; couldn't you just check for
> APT_PROCESSORS_LIST being empty?

Would be possible, but I think that having two properties will better match the UI. Consider that the user has manually selected a few annotation processors, and wants to try running all APs. With two properties, the manually selected processors will be kept, and if the user will at any time choose to run only selected processors, he/she will not need to select them again. With only one property is used, the list of selected processors would be lost when the user would decide to run all APs.

> 
> 
> [JG06] j2seproject should pass '-s build/generated-sources/apt' (or whatever
> name you pick for the last path component), as it is useful to be able to see
> generated source files.

I added "-s ${build.generated.sources.dir}/apt", but we might need to filter that out from the internal source path - with the current patch, scanning would be triggered after each rebuild, which is not desirable.

> 
> 
> [JG07] AptUtils.getProcessorNames could probably be replaced by a call to
> Lookups.metaInfServices in case aptOptions.annotationProcessorsToRun() == null.

Lookups.metaInfServices (IIRC) does not handle "broken" services very well - when one service throws an exception during loading/instantiation, no services are provided and an exception is thrown (bug #173202). If that is changed in Lookups.metaInfServices, we may be able to use it.

> 
> 
> [JG08] Consider using ExecutionEngine.createPermissions in your class loader as
> it will ensure that any stdio (e.g. Throwable.printStackTrace) from annotation
> processors will be sent to an InputOutput rather than the NB log. (You can make
> the InputOutput be hidden unless and until something is printed to it.)

Thanks, I will take a look at that. I am not sure that opening an Output tab for output from annotation processors is appropriate (writing to such tab could happen at strange moments) though.
Comment 7 Jan Lahoda 2010-01-25 03:29:30 UTC
Created attachment 93520 [details]
Updated API patch.
Comment 8 Jesse Glick 2010-01-25 13:04:39 UTC
(In reply to comment #6)
> if there is a
> strong opinion that is should be Set<? extends String>

Not a strong opinion, just seems clearer.

>> Usage of the term "APT" in various APIs may be confusing
> 
> Would AnnotationProcessingQuery be better?

Sounds good.

> With two properties, the manually selected
> processors will be kept, and if the user will at any time choose to run only
> selected processors, he/she will not need to select them again.

OK. Not sure whether this use case outweighs the possible confusion caused by redundancy in properties.

> I added "-s ${build.generated.sources.dir}/apt", but we might need to filter
> that out from the internal source path - with the current patch, scanning would
> be triggered after each rebuild, which is not desirable.

Right. Perhaps java.api.common could display this dir in the logical view just like any other ${build.generated.sources.dir}/*, and add a Java SourceGroup for it, but exclude it from the ClassPath.SOURCE of ${src.dir}?

Or APQ could be extended to let the project specify a -s directory, in which case java.source would know to ignore changes in it?

> Lookups.metaInfServices (IIRC) does not handle "broken" services very well -
> when one service throws an exception during loading/instantiation, no services
> are provided and an exception is thrown (bug #173202).

As far as I know, MISL correctly isolates errors coming from particular services and continues to load others. If you can reproduce otherwise, please file a bug in platform/lookup and have it block this. (I don't see anything on the topic mentioned in #173202.)

> I am not sure that opening an Output tab
> for output from annotation processors is appropriate

Probably messages going to Filer should be managed right in the editor: as errors, warnings, or (for INFO) warnings on the current line. But if an AP accidentally or incorrectly prints to stdout or stderr, e.g. because of a call to Throwable.printStackTrace, you probably want to see that, and the OW is probably more appropriate than the NB console (since there is no bug in NB). Of course you could just decline to display such messages at all and leave it to an Ant build (or whatever).
Comment 9 Jan Lahoda 2010-01-27 08:02:36 UTC
I have incorporated the API:
http://hg.netbeans.org/jet-main/rev/da5a766e5aa2
(APTQuery renamed to AnnotationProcessingQuery, and corresponding classes/interfaces/methods renamed similarly)
and basic implementations for apisupport.project:
http://hg.netbeans.org/jet-main/rev/188e113764a5
and J2SE Project:
http://hg.netbeans.org/jet-main/rev/bf8dcf793fb4

I have filled two new bugs for the last two comments (Lookups.metaInfServices and capturing output printed to System.{err|out} from annotations processors).
I tried to do something with the source path and ${build.generated.sources.dir}/ap-source-output (formerly "apt"), but that probably will need more work.

Dusan will integrate the changes to java.source and GUI for J2SE Project.
Comment 10 Jan Lahoda 2010-01-27 08:27:04 UTC
One more Apt->AnnotationProcessing change.
http://hg.netbeans.org/jet-main/rev/c845b7f66098
Comment 11 Jesse Glick 2010-01-27 09:34:08 UTC
The apisupport.project query impl is not quite right, I guess. It should return build/classes-generated from sourceOutputDirectory for src, and build/test/$type/classes-generated for test/$type/src.
Comment 12 Quality Engineering 2010-01-27 22:21:08 UTC
Integrated into 'main-golden', will be available in build *201001280200* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/da5a766e5aa2
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #179749: introducing AnnotationProcessingQuery and related APIs/generic implementations.
Comment 13 Jesse Glick 2010-03-02 07:30:35 UTC
(In reply to comment #11)
> The apisupport.project query impl is not quite right

bug #181419


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo