Bug 179749

Thanks for review.

I get a 401 - Unauthorized from your server.

Created attachment 93471 [details]
API diff.

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.

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

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

Created attachment 93520 [details]
Updated API patch.

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

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.

One more Apt->AnnotationProcessing change.
http://hg.netbeans.org/jet-main/rev/c845b7f66098

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.

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.

(In reply to comment #11)
> The apisupport.project query impl is not quite right

bug #181419