+1 on META-INF/layer.xml, convention over configuration.

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?

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.

Created attachment 72335 [details]
Example showing how easy it is to define, process, and use an annotation

Created attachment 72337 [details]
API & infrastructure

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.

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

Created attachment 72432 [details]
Revised example accommodating Y04, and also showing two processors at work

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.

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.

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

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.

Created attachment 72497 [details]
Current state of LayerGeneratingProcessor & LayerBuilder

Created attachment 72498 [details]
New example demonstrating more advanced features of LayerBuilder: instanceAttribute and "smart" bundlevalue

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?

Created attachment 72515 [details]
FYI: Successes and failures of LayerGeneratingProcessor clueless user

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

Created attachment 72631 [details]
Basic processor example, revised to new SPI

Created attachment 72632 [details]
More complex processor example, revised

Created attachment 72633 [details]
your example, revised to new SPI; fixing annotation definition to use nesting properly; now validates shortcut syntax; test still broken

Created attachment 72635 [details]
Example of non-object-oriented annotation defined on packages (HelpSet registration)

*** Issue 151365 has been marked as a duplicate of this issue. ***

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.

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.

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.

Merged in core-main #78103b644dfc.

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.

Created attachment 73235 [details]
Possible patch to add extra convenience constructors to LayerGenerationException

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 on attachment 73235 [details]
Possible patch to add extra convenience constructors to LayerGenerationException

See bug #194545 for last patch.