Skip to content

Conversation

@arievanwi
Copy link

Hi Peter,

As promised the changes for support of the v1.3 SCR specification. This means that the following new features should be supported now:

  • @Reference annotations for fields, both collection types and normal service references.
  • @interface as life-cycle method parameters.
  • Scope handling.

I tested the features and afterwards we checked both the original functionality and (some of the) new features in some projects we are currently busy with. This means that the code should be relatively stable, although we don't use the more advanced/exotic features.

What is changed?

  • I replaced the annotations jar with the 1.3 version release.
  • The reference handling that was already there is split to a common part (that applies both method and field references) and additional parts for the specific method/field handling.
  • You already had code in place to check the required specification version and set the xmlns accordingly. I extended this, so most parsing methods now return a version number indicating the minimum SCR specification version needed given the annotations. Depending on the overall maximum, the related xmlns is set on the XML.
  • I adapted the parsing of life-cycle methods to detect the new @interface parameters and process properties with defaults from them.
  • As a result of the previous point, slightly modified the handling of the component properties.
  • I adapted the problem handling a little to allow problems from other sources than the component source.
  • I removed the lib bundle. IMHO this one is not necessary.

Known issues

  • If the only change in the XML file applies the change of the xmlns (for example because of a signature change of a life-cycle method), the namespace is not updated because this is not detected as a change. Don't know why, maybe you know a way to force the change.
  • The field annotation parsing also looks in the class hierarchy to find protected/public fields in super classes with @Reference annotations. If such annotations contain errors/warnings, these are not shown unless both the component source and the base class source are compiled in one go (because of a clean action for example). The problem is related to the fact that DSAnnotationCompilationParticipant.processAnnotations is called twice, once for the base class and once for the main class and as such the BuildContext of the base class is not present anymore when the error occurs. As such the problem cannot be reported.

If you have any questions or need additional information, don't hesitate to contact me.

Best regards,
Arie van Wijngaarden

@pnehrer
Copy link
Member

pnehrer commented Jan 15, 2016

Thank you Arie, I'll take a look in the next few days and will come back with questions/feedback.

@pnehrer
Copy link
Member

pnehrer commented Feb 1, 2016

Arie, apologies for the delay -- I was hoping to have a more complete review by now but there are just too many things going on.

Some initial comments and observations:

  1. per spec, I believe we should only look for annotations in the implementation type and not in its super-types; even if the super-type might be annotated, we cannot take those annotations into account when generating descriptors
  2. the lib bundle should stay for the time being and here's why: In order to implement Issue Hook into PDE Classpath container #20 I had to find a way to automatically place annotations on every PDE bundle project's classpath. However, the existing mechanism extends the classpath transiently -- only in the IDE, so the user must still place the annotations jar on their project's classpath using one of the "permanent" approaches. What I found is that JDT ignores duplicate classpath entries when they map to the same filesystem location (and we need two separate classpath entries to distinguish transient from permanent contributions), so for that reason I opted for two separate sources of annotations.
  3. I'm still debating whether to just upgrade annotations to v1.3 or allow the user to select between v1.2/1.3 on workspace/per-project basis -- the user may prefer not to utilize v1.3 if they cannot deploy into SCR that supports v1.3, and having the new annotations defaulted might create confusion.

@arievanwi
Copy link
Author

Hi Peter,

Ad 1.
You're right. I overlooked it and incorrectly assumed that since in the XML definition it is allowed to specify parent class fields and methods, it would also be the case in the annotations. I will remove the recursive processing which will automatically solve the issue with the problem marking.

Ad 2, 3.
To be honest, I think the option to extend the class path should not be used. You will need to extend the permanent class path anyway, so why shouldn't you do it right in one go?

I will make the change for 1. later and commit again. That will be early next week, since I am busy with other stuff the remainder of this week.

@arievanwi
Copy link
Author

Hi Peter,

I added some commits:

  • Removed the super parsing.
  • Restored the lib project again and updated the annotations in there.

@pnehrer pnehrer self-assigned this Feb 25, 2016
@pnehrer pnehrer added this to the R1.3.0 milestone Feb 25, 2016
@pnehrer
Copy link
Member

pnehrer commented Feb 25, 2016

FYI after a long wait this project has been contributed to Eclipse PDE proper (targeted for Neon M6). The version I submitted is missing a few of the most recent commits (basically what went into v1.2.8). See https://bugs.eclipse.org/bugs/show_bug.cgi?id=376950.

Here's what I'm planning to do:

  • merge your contribution into this project (after a few revisions) and release it as v1.3.0, but only for Eclipse versions before Neon (i.e., dependencies won't be updated to support Neon)
  • back-port the outstanding commits, as well as your support for DS Annotations 1.3, to PDE proper; however, I'm not sure when annotations v1.3 will be available in Eclipse (that's TBD), so for a while older versions of Eclipse using this plugin will support more recent annotations

@pnehrer
Copy link
Member

pnehrer commented Feb 26, 2016

Arie, while reviewing and merging your changes, I realized that there will be additional code required to handle the newly added component property types. Up until now, the entire descriptor could be generated from one class (the component implementation class). However, when component property types are used, these could be in entirely separate compilation units, packages and even projects; whenever those change, the impacted implementation classes will have to be re-processed.

This is not quite trivial -- I have to think about how to approach this; e.g., maintain a mapping of cpt-to-implementation (one to many), then trigger build on those implementations; but what if they're in unrelated (non-DS) projects?.. etc. Let me know if you have any ideas.

@arievanwi
Copy link
Author

Hi Peter,
AFAIK this is not an issue: if the component property interface is adapted, the classes that use it are automatically recompiled by Eclipse and therefore the annotations are recreated.
At least that is what I observed while debugging the code, but if you can think of some specific situation where this is not the case, I would suggest to just try it.

Regards,
Arie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants