Bug 149136 - Support annotations for creation of layer entries
Support annotations for creation of layer entries
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Filesystems
6.x
All All
: P1 (vote)
: 6.x
Assigned To: Jesse Glick
issues@platform
:
: 151365 (view as bug list)
Depends on: 32576 147393 194545
Blocks: 150194 150804
  Show dependency treegraph
 
Reported: 2008-10-03 22:22 UTC by Jesse Glick
Modified: 2011-07-18 22:24 UTC (History)
4 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Example showing how easy it is to define, process, and use an annotation (10.21 KB, patch)
2008-10-21 05:19 UTC, Jesse Glick
Details | Diff
API & infrastructure (54.32 KB, patch)
2008-10-21 05:30 UTC, Jesse Glick
Details | Diff
Revised example accommodating Y04, and also showing two processors at work (19.67 KB, patch)
2008-10-22 00:47 UTC, Jesse Glick
Details | Diff
Current state of LayerGeneratingProcessor & LayerBuilder (41.19 KB, patch)
2008-10-22 23:10 UTC, Jesse Glick
Details | Diff
New example demonstrating more advanced features of LayerBuilder: instanceAttribute and "smart" bundlevalue (13.78 KB, patch)
2008-10-22 23:13 UTC, Jesse Glick
Details | Diff
FYI: Successes and failures of LayerGeneratingProcessor clueless user (20.12 KB, patch)
2008-10-23 09:12 UTC, Jaroslav Tulach
Details | Diff
Basic processor example, revised to new SPI (19.25 KB, patch)
2008-10-24 21:38 UTC, Jesse Glick
Details | Diff
More complex processor example, revised (13.19 KB, patch)
2008-10-24 21:39 UTC, Jesse Glick
Details | Diff
your example, revised to new SPI; fixing annotation definition to use nesting properly; now validates shortcut syntax; test still broken (18.18 KB, patch)
2008-10-24 21:41 UTC, Jesse Glick
Details | Diff
Example of non-object-oriented annotation defined on packages (HelpSet registration) (13.74 KB, patch)
2008-10-24 22:28 UTC, Jesse Glick
Details | Diff
Possible patch to add extra convenience constructors to LayerGenerationException (5.55 KB, patch)
2008-11-04 19:28 UTC, Jesse Glick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2008-10-03 22:22:19 UTC
I am playing with using annotation processors to generate XML layer entries at build time.

There are two major issues:

1. Where should the processors live?

1a. Issue #147393 (building on the earlier patch #6e9ef144f2cb) assumes that the processor is right inside the module
JAR file.

1b. An alternative would be to keep processors in separate JARs.

1a is simpler to use, since you do not need any special javac options (you only need to ensure you are running a
269-compliant compiler), so it should work best with CoS and Maven builds. 1b minimizes module JAR size and dependencies.

For now I am going with 1a. It means that we will need to have libs.javacapi in the platform so long as support JDK 5.
In the longer term, when we can assume JDK 6, the special compiler trick can be removed and the runtime dependencies
will be minimal (a module supplying an annotation processor will just need to depend on org.openide.modules or whatever
contains the base class for layer-generating processors).

2. How should the generated layer be placed? It needs to be created somewhere in build/classes/, but then what?

2a. Hack <jarwithmoduleattributes> to define OpenIDE-Module-Layer if not already present, and if present, to merge the
generated layer into the source layer when packing the JAR.

2b. Modify the module system (NbInstaller, ExternalUtil, ModuleLayeredFileSystem, apisupport's LayerUtils) to look for
and load META-INF/generated-layer.xml if present, in addition to any regular declared layer if present.

For now I am going with 2b. 2a would be a bit easier to implement, but adds complexity to the build system and will
likely cause problems for CoS and Maven. In the long run I would also like to discourage OpenIDE-Module-Layer and
instead recommend source layers be conventionally stored as META-INF/layer.xml - again easing CoS and potentially
simplifying other things by removing an unnecessary variable (the resource path of the layer).
Comment 1 Milos Kleint 2008-10-04 10:20:34 UTC
+1 on META-INF/layer.xml, convention over configuration.
Comment 2 rmichalsky 2008-10-20 13:39:45 UTC
Have you already thought about the syntax for this or more precisely mapping of annotations to XML elements?

And, just as in issue #150447, I suppose wizards should create annotations instead of XML layer entries once this is ready?
Comment 3 Jesse Glick 2008-10-21 00:04:23 UTC
The syntax for particular annotations will need to be decided on a case-by-case basis in individual API reviews, but for
example we might have something like

package org.netbeans.modules.apisupport.project.hyperlink;
import ...;
@HyperlinkProvider(mimeType="text/x-java")
public class ApisupportHyperlinkProvider implements HyperlinkProviderExt {...}

And yes, wizards should create annotations rather than XML layer entries whenever possible, i.e. for SPIs which have
associated convenience annotations when the user is using a sufficiently new platform.
Comment 4 Jesse Glick 2008-10-21 05:19:20 UTC
Created attachment 72335 [details]
Example showing how easy it is to define, process, and use an annotation
Comment 5 Jesse Glick 2008-10-21 05:30:30 UTC
Created attachment 72337 [details]
API & infrastructure
Comment 6 Jaroslav Tulach 2008-10-21 09:56:18 UTC
Y01 Make sure that subsequent starts do not open JAR files (e.g. that the org.netbeans.Archive cache is handling the 
new location of generated layer files correctly)

Y02 No new reflection in modules, please. I am refering to: ClassLoader.class.getDeclaredMethod("findResources", 
String.class)

Y03 Is there any value in LayerGeneratingProcessor extending AbstractProcessor? It makes the process method final, 
anyway. I'd rather see it to an independent interface which one AbstractProcessor provided by openide.filesystems 
implementation delegates to. Its doProcess method shall probably take Document or LayerContext or LayerBuilder itself 
(!?) as argument. Probably for deeper discussion than which can fit into this issue.

Y04 I'd suggest to move the instanceFile method to LayerBuilder. There is not much special on it, as Lookups.forPath 
with openide.filesystems on, does recognize the syntax.

Y05 I like the "self referent" use of @Service(Processor.class), yet I'd like to know why it is not better to have 
@LayerProcessor(Class<? extends LayerGeneratingProcessor> processor) annotation attached to the sample @interface 
RegisteredNodeFactory and co.
Comment 7 Jesse Glick 2008-10-21 23:51:10 UTC
Y01 - I don't know this code very well. If it can correctly cache content in META-INF/** then it should handle
generated-layer.xml like anything else. Do you have any particular tip on how to verify this?


Y02 - I thought about this for a while and I believe the current code is best. Here's the background:

ClassLoader.getResources cannot be used here because it would delegate to parent loaders which might have their own
generated layers, resulting in duplicates. Similarly, getResource would be incapable of distinguishing between a module
with a generated-layer.xml and a module with no such entry but a parent with one. (And getResource would fail to load >1
generated-layer.xml for a module defined from multiple JARs.) So findResource{,s} is really needed. (Class.findClass
need not be public because you can call loadClass and check whether the defining loader, i.e. result.getClassLoader(),
is in fact the initiating loader. By contrast, there is no way to distinguish between initiating and defining loader
when getting resources. This is really a deficiency in the ClassLoader API which I intend to file as such; you should be
able to get an Iterable<Pair<URL,ClassLoader>> when asking a loader for resources.)

While findResource{,s} is protected in ClassLoader, it is public in URLClassLoader, and I just made it public in NB's
equivalent ProxyClassLoader for consistency. (Formerly findResources was incorrectly being implemented the way
getResources should be, which was independently causing problems.) The use of reflection simply allows NbInstaller to
call findResources, whose contract is defined independently of NB; this call does not presume that the ClassLoader
target overrides the method to be public. While NbI could directly call PCL.fR, this would not really avoid the need for
reflection, since fixed modules have externally defined loaders and we do not know how these are implemented (though it
is a URLCL in the case of the app CP in a Sun JRE):

Enumeration<URL> e;
if (cl instanceof URLClassLoader) {
  e = ((URLClassLoader) cl).findResources(rsrc);
} else if (cl instanceof ProxyClassLoader) {
  e = ((ProxyClassLoader) cl).findResources(rsrc);
} else {
  // now what? still need to use reflection anyway
}


Y03 - yes, AbstractProcessor provides a number of useful implementations of Processor methods which are orthogonal to
layer generation.

I did initially consider a custom SPI, with implementations delegated to by a singleton layer-based processor. It would
need to load impls from some ClassLoader - probably ThisClass.class.getClassLoader(), since
JavaFileManager.getClassLoader(StandardLocation.ANNOTATION_PROCESSOR_PATH) would work only if you had a JavaFileManager,
but this is accessible only to callers of JavaCompiler, not to annotation processors. In fact such an implementation
would be a bit simpler because I would not need the WeakHashMap<ProcessingEnvironment,Document> trick.

The reason I rejected this possibility was that it would require making reflective calls (even indirectly through
Lookups.metaInfServices) which could be rejected by some security managers. Annotation processors cannot assume they
will run with full permissions, because they can be invoked embedded, e.g. in an IDE. By contrast, the current impl
should work unmodified even when run in a sandbox.

Another reason to prefer real Processor instances to be registered is that they can directly control
getSupportedAnnotationTypes, getCompletions, etc. (A singleton processor could implement getSupportedAnnotationTypes as
the union of a similar method called on its delegates, but this is getting uglier.)


Y04 - OK, I will try to do this. I was hoping to isolate the 269 dependencies in LayerGeneratingProcessor, but I guess
it does not really matter.


Y05 - I'm not sure I follow what you are asking for and why.

Also note that it is not safe for any processor other than ServiceProcessor to generate M-I/s/* files, as 269 forbids a
single resource to be written to more than once in a given round. (This is BTW the reason why LayerGeneratingProcessor
needs to use the WeakHashMap trick, rather than each processor of this type being completely independent - they would
all need to write to the same generated-layer.xml.)
Comment 8 Jesse Glick 2008-10-22 00:47:51 UTC
Created attachment 72432 [details]
Revised example accommodating Y04, and also showing two processors at work
Comment 9 Jaroslav Tulach 2008-10-22 16:08:12 UTC
Y06 I need support for generating .shadow files

Re. Y01 run trunk build with -J-Dorg.netbeans.JarClassLoader.lever=401 to see which JARs get open.

Re. Y03 and "This is BTW the reason why LayerGeneratingProcessor needs to use the WeakHashMap trick" - you would need 
no WHM trick if you had one Processor delegating to multiple LayerGenerators (not extending Processor). But I agree 
that looking up all of these with ServiceLoader is not really nice.

Re. Y05 - This could solve the ServiceLoader problem mentioned above. This would be the API:
@Target(ANNOTATION_TYPE)
@interface LayerContributor {
  Class<? extends LayerGenerator> generator();
}
and this would be the usage:
@LayerContributor(generator=RegisteredNodeFactoryProcessor.class)
public @interface RegisteredNodeFactory {...}
I am not sure if this is implementable efficiently. But if so, I see this style nicer than subclassing 
AbstractProcessor.
Comment 10 Jesse Glick 2008-10-22 18:44:01 UTC
Y05 boils down to registering a service of some kind, since it has to work even when the processor is present in binary
form in the classpath.

Either way, Y05 has the same problem as Y03: it would require the (actual) processor to perform reflective class
loading, possibly failing in a sandbox. (While you can get an annotation proxy in 269, I believe it will _always_ throw
MirroredTypeException upon accessing a Class-valued attribute, even if the caller could somehow "see" the class.) I am
not positive what actions would or would not be permitted in general but this seems risky to me. (Class.forName(String)
does no security checks. Class.newInstance() does, though the default in SM is to permit it for a public constructor.)

And again this would prevent processors from implementing e.g. getCompletions for themselves.
Comment 11 Jesse Glick 2008-10-22 21:06:37 UTC
Y06 - will do. Also adding a File.instanceAttribute(String attr, Element, Class, ProcessingEnvironment) to complement
instanceFile for the case that the POJO should be associated with a particular FO attribute rather than just being an
InstanceCookie (e.g. instantiatingIterator).
Comment 12 Jesse Glick 2008-10-22 21:33:44 UTC
Y01 - well, when using this in the trunk (i.e. without my changes) I see a number of modules getting opened even on a
second start. Issue #149261 seems to imply that this is not uncommon. With similar logging in the branch I also see a
bunch of opened JARs. As far as I can see, on a second start no JAR is being opened due to generated-layer.xml.
Comment 13 Jesse Glick 2008-10-22 23:10:46 UTC
Created attachment 72497 [details]
Current state of LayerGeneratingProcessor & LayerBuilder
Comment 14 Jesse Glick 2008-10-22 23:13:00 UTC
Created attachment 72498 [details]
New example demonstrating more advanced features of LayerBuilder: instanceAttribute and "smart" bundlevalue
Comment 15 Jaroslav Tulach 2008-10-23 09:07:16 UTC
I am basically OK with the previous answers to my Y0x questions. However here is new round:

Y11 Rename doProcess to something more specific. processLayer? generateLayer?
Y12 Change the API so people do not forget to call write() at the end of file creation. Maybe: add cancel() and when 
one returns from the "doProcess" method, report all unfinished creations.
Y13 new LayerBuilder(layer(e)).file("...") is too verbose. Can't I have file(e, "...")?
Y14 often I need fully qualified (dashed) name of an element. Some methods accept null, but a helper method could be 
useful too.
Y15 We need guidelines for processor writers: how to localization (smart bundle value is acceptable), still people 
need to edit two files, the Java and the Bundle one. Can't we put the default (English) text to Java file and generate 
the Bundle?
Y16 We need guidelines for processor writers: the registrations shall be lazy, can this be put in some howto?
Y17 We need guidelines for processor writers: how to write tests. Imho each processor shall be accompanied with a 
test. Can we have apisupport wizard to generate the necessary skeleton?
Comment 16 Jaroslav Tulach 2008-10-23 09:12:04 UTC
Created attachment 72515 [details]
FYI: Successes and failures of LayerGeneratingProcessor clueless user
Comment 17 Jesse Glick 2008-10-24 21:37:24 UTC
I refactored the SPI so that layer(...) directly returns a LayerBuilder and that various method parameters in LB/LB.F
convenience methods can be dropped. This simplifies processor code, reduces the chance that people will be confused
about what to use for a parameter, and permits LB code to take advantage of facilities like the Messager available to
the processor.

I have also introduced a LayerGenerationException, capturing information necessary for 269 error reporting, which is
thrown from various LB/LB.F methods and caught by the base process(...) method on your behalf. Again this simplifies
typical processor code and improves the usefulness of reported errors by ensuring they are hyperlinked to sources
whenever possible.

I am also updating the various example patches that include processor implementations to demonstrate how they will look
using the new API.


Y11 - I have renamed it to handleProcess, in line with e.g. Servlet SPI method naming. "processLayer" etc. would imply
that the processor can _only_ add layer entries during this method, which is not true; it is permitted to generate other
resources as well. Also I wanted to emphasize that the method parameters and return value are essentially identical to
those of the regular process(...) method, and that it is called directly from process(...) with the same lifecycle.


Y12 - done. No cancel method, but unwritten files generate a warning when a job completes without any reported errors.
(This also required the refactoring.)


Y13 - now layer(e).file("..."), layer(e).instanceFile("Menu/File", null, T.class), etc.


Y14 - I will hold off on this until I see a concrete use case not covered by instanceFile or instanceAttribute. There
are various ways you could stringify a type or method or field and it is not clear what some helper method should
provide in the absence of an external specification such as the syntax of InstanceDataObject. ('.' or '-' as separator,
or '.' in the class name but something else to append a method name? '$' or '.' for nested classes?)


Y15 - we could indeed generate e.g. whatever/package/SomeClass.properties when whatever/package/SomeClass.java is
processed, to include just strings specified as literals in annotations in SomeClass.java. This would need to be done
with some care so as to permit multiple annotations (possibly handled by different processors) to be cooperatively
written to the same bundle file. I think there would be no effect on an L10N team, assuming they localize binaries
rather than sources.

However I would rather leave a trick like this to another effort dedicated to permitting localizable strings to be used
in source files more generally: the proportion of such strings which happen to be in places where a layer-generating
annotation can be used is probably rather small. Most places would be regular programmatic calls to NbBundle. There have
been some ideas thrown around nbdev for extracting resource bundles mechanically from sources, possibly using 269.
Upgrading LayerBuilder.File.bundlevalue(attr,label) to also use such a system would be easy enough.


Y16 - this is good advice but not specific to SPIs using annotations as syntactic sugar; any SPI which defines layer
registrations needs to be done with this in mind.


Y17 - writing tests is somewhat subtle. As you found in your example patch, you can simply use the annotation in the
unit test and verify that the generated layer fragment is correct, and/or that the SPI which looks in the SFS loads the
final service as intended. (I needed to make some changes to issue #147393 to enable test code of a module to be
processed by an AP defined in the same module.)

However, such tests can only verify that the result of the processor is correct when it terminates without error. If you
want to fully test the processor - check that it signals an error when given incorrect sources, and that messages (fatal
or not) are informative - then you need to programmatically invoke it using JSR 199. I have test code like this in
SezPoz which can be used as an example, but the infrastructure is complex. hickory.dev.java.net provides infrastructure
for writing such tests which we might want to use (this would require that it be packaged as a module in the harness, I
guess).
Comment 18 Jesse Glick 2008-10-24 21:38:33 UTC
Created attachment 72631 [details]
Basic processor example, revised to new SPI
Comment 19 Jesse Glick 2008-10-24 21:39:19 UTC
Created attachment 72632 [details]
More complex processor example, revised
Comment 20 Jesse Glick 2008-10-24 21:41:55 UTC
Created attachment 72633 [details]
your example, revised to new SPI; fixing annotation definition to use nesting properly; now validates shortcut syntax; test still broken
Comment 21 Jesse Glick 2008-10-24 22:28:30 UTC
Created attachment 72635 [details]
Example of non-object-oriented annotation defined on packages (HelpSet registration)
Comment 22 Jesse Glick 2008-10-25 19:43:54 UTC
*** Issue 151365 has been marked as a duplicate of this issue. ***
Comment 23 Jaroslav Tulach 2008-10-29 18:09:25 UTC
Thanks for the replies. I'll try to denote Y16 and Y17 as TCRs tommorow:

Ad. Y16: In your http://www.netbeans.org/nonav/issues/showattachment.cgi/72432/149136-example.diff, the definition of 
of @RegisteredNodeFactory is exactly against the rules that I'd like to impose on new annotation processor based API. 
The annotation shall have additional attributes like name(), displayName(), icon(), etc. Those attributes shall be 
used to create a default node, displayed in the project structure. Only when the user manipulates it (expands or shows 
popup menu, etc.) the NodeFactory is called to provide the right node. Such registration style shall be used in other 
places (registration of nodes to Services tab, etc.)

Ad. Y17: I do not care that we cannot test broken code that much. However we need to have an example showing how that 
can be done before we integrate the whole infrastructure.

Comment 24 Milos Kleint 2008-10-29 18:37:25 UTC
re jtulach: ad Y16: your NodeFactory examples misinterpret the existing APIs. One factory instance can create zero to N
nodes for the given project and in no way can be replaced with a dummy node replacement. At least in the current form of
the API. In that way jglick's example seems to be correct.
Comment 25 Jesse Glick 2008-10-31 20:56:53 UTC
Y17 - I have created org.openide.util.test.AnnotationProcessorTestUtils, inspired by some infrastructure I had created
for SezPoz. Not as sophisticated as hickory.dev.java.net, but sufficient to test errors from ServiceProviderProcessor.
Comment 26 Jesse Glick 2008-11-01 16:51:15 UTC
Merged in core-main #78103b644dfc.
Comment 27 Quality Engineering 2008-11-04 16:23:54 UTC
Integrated into 'main-golden', will be available in build *200811041401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/d322e574dffb
User: Jesse Glick <jglick@netbeans.org>
Log: #32576: fixing {get,find}Resource{,s} delegation semantics to match e.g. URLClassLoader.
Needed for #149136 so that NbInstaller can find META-INF/generated-layer.xml without delegation to parents.
Comment 28 Jesse Glick 2008-11-04 19:28:42 UTC
Created attachment 73235 [details]
Possible patch to add extra convenience constructors to LayerGenerationException
Comment 29 Jesse Glick 2008-11-04 19:31:17 UTC
The above patch makes it easier for processors to report errors which point to the actual annotation (rather than the
annotated element), and even to a value within that annotation (though javac seems to ignore this). Otherwise it is
awkward to do this using 269.

TBD whether we will need it. LayerBuilder cannot really use it when constructing LGEs without other API changes, since
it does not know which annotation it is being associated with.
Comment 30 Jesse Glick 2011-07-18 22:24:33 UTC
Comment on attachment 73235 [details]
Possible patch to add extra convenience constructors to LayerGenerationException

See bug #194545 for last patch.


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