bndtools / bnd

Bnd/Bndtools. Tooling to build OSGi bundles including Eclipse, Maven, and Gradle plugins.
https://bndtools.org
Other
531 stars 304 forks source link

Support OSGi DS 1.4 annotation changes #2179

Closed bjhargrave closed 6 years ago

bjhargrave commented 6 years ago

Including constructor injection and activation fields.

bjhargrave commented 6 years ago

Also need to support marker annotations as component property types.

rakeshk15 commented 6 years ago

Hi @bjhargrave,

When will the 4.0/4.1 milestone be released? Basically I was trying OSGi R7 features and found that bnd-maven-plugin 3.5.0 doesn't recognise them and while searching issues I ended up here :)

Thanks, Rakesh

bjhargrave commented 6 years ago

Some of this support will be in 4.0 which will be released in the next week or two. But it may be incomplete and not fully tested. 4.1 will be the release which will include the complete and tested support.

rkwebitup commented 6 years ago

Thanks for the update. So by when 4.1 will be ready if you could provide a timeframe. Actually, I updated my open source OSGi based project to R7 and now tooling is the bottleneck. I think I need to ignore the R7 features/annotations for the time being. What do you suggest, pls revert.

bjhargrave commented 6 years ago

I don't have a specific timeframe for 4.1 yet. But it will be a couple of months after 4.0 I suspect.

timothyjward commented 6 years ago

I think I need to ignore the R7 features/annotations for the time being.

What R7 features do you want to use? Component Property annotations are broadly working in the 4.0.0 stream, as are the bundle annotations.

rakeshk15 commented 6 years ago

thanks @bjhargrave

@timothyjward I am looking forward to use all the latest component/bundle related annotations e.g the usage of ComponentPropertyType on various places (activation fields/objects as per the spec).I was not sure whether they will work with 4.0.0 so thought to ask for clarity. I'll try 4.0.0 as per your comment.

timothyjward commented 6 years ago

e.g the usage of ComponentPropertyType on various places (activation fields)

This will probably not work as I don't think bnd 4.0.0 will support activation fields. It will work if you inject the Component Property Type into an activate method.

e.g the usage of ComponentPropertyType on various places (objects as per the spec)

This should work fine, there are tests

I am looking forward to use all the latest component/bundle related annotations

This should also work fine, although there may be issues with processing @Attribute and @Directive on meta-annotated requirements and capabilities. It probably isn't too late to put together a couple of test cases and a fix for this, as it should be pretty small...

rakeshk15 commented 6 years ago

thanks for the update @timothyjward. This is helpful, much appreciated.

timothyjward commented 6 years ago

See also PR #2434 - this may or may not make 4.0.0, but it should complete testing/support for the bundle annotations.

cziegeler commented 6 years ago

Just to confirm what Tim said above, activation fields definitely do not work with the 4.0.0 release (I just gave it a try)

rakeshk15 commented 6 years ago

here is the error message -

[INFO] --- bnd-maven-plugin:4.0.0:bnd-process (default) @ adeptj-modules-commons-datasource --- [ERROR] /Users/rakesh.kumar/code/adeptj/adeptj-modules/commons/datasource/bnd.bnd [0:0]: Activate annotation on a field com.adeptj.modules.commons.ds.internal.DataSourceFactory.Lorg/osgi/framework/BundleContext; [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ [INFO] Total time: 1.887 s [INFO] Finished at: 2018-05-23T15:06:16+05:30 [INFO] ------------------------------------------------------------------------ [ERROR] Failed to execute goal biz.aQute.bnd:bnd-maven-plugin:4.0.0:bnd-process (default) on project adeptj-modules-commons-datasource: Activate annotation on a field com.adeptj.modules.commons.ds.internal.DataSourceFactory.Lorg/osgi/framework/BundleContext;

rakeshk15 commented 6 years ago

@cziegeler as per discussion above this was not supposed to be in v4.0.0. v4.1 is the target milestone to include it I believe.

Comment from @timothyjward

This will probably not work as I don't think bnd 4.0.0 will support activation fields. It will work if you inject the Component Property Type into an activate method.

cvgaviao commented 6 years ago

@bjhargrave, was bndlib 4.1.0.DEV published at any maven repository ?

bjhargrave commented 6 years ago

@bjhargrave, was bndlib 4.1.0.DEV published at any maven repository ?

See the https://github.com/bndtools/bnd/blob/master/README.md#release for the URL of a snapshot repo that holds the latest builds.

cvgaviao commented 6 years ago

@bjhargrave, thanks !

glimmerveen commented 6 years ago

Will using a ComponentPropertyType as second argument to an @Reference annotated method (to represent the properties of the referenced service) be part of this OSGi DS 1.4 development effort? I tried this using version 4.0.0 and I see it uses the ComponentPropertyType as referenced service type, rather then the type of the first parameter of the method.

timothyjward commented 6 years ago

Will using a ComponentPropertyType as second argument to an @Reference annotated method (to represent the properties of the referenced service) be part of this OSGi DS 1.4 development effort?

No, it is not part of the DS 1.4 specification and so it will not be supported. Our effort is to enhance bnd to support the existing specification, not to change the specification (it is too late to do this anyway as DS 1.4 was released a few months ago).

I tried this using version 4.0.0 and I see it uses the ComponentPropertyType as referenced service type, rather then the type of the first parameter of the method.

This is a bug in bnd, it should simply tell you that the method isn't a valid bind method.

bjhargrave commented 6 years ago

Will using a ComponentPropertyType as second argument to an @Reference annotated method (to represent the properties of the referenced service) be part of this OSGi DS 1.4 development effort?

No. The DS spec does not include passing activation objects (like component property types) to bind method. Activation objects are passed to life cycle methods (e.g. activate) and, new to DS 1.4, set in activation fields or passed as constructor parameters. (Support for these 2 things are part of this issue.)

I tried this using version 4.0.0 and I see it uses the ComponentPropertyType as referenced service type, rather then the type of the first parameter of the method.

A recent fix to Bnd (under the work for this issue) means that Bnd will only use the first argument to the bind method to infer the service type. But if you use a component property type as a bind method parameter, it will never be considered to be an activation object. It will be considered as a referenced service.

glimmerveen commented 6 years ago

Hi @timothyjward and @bjhargrave, I misinterpreted the text in the spec and assumed that the usage of these types for the life cycle methods also applied for the @Reference annotated methods. Perhaps something to consider for a future iteration of the spec, but clearly not part of this development effort.

bjhargrave commented 6 years ago

All changes to support the new DS 1.4 features are now in Bnd for the 4.1 release.

New type inferencing code was written using new Java generic signature parsing code. So this is a major change to the DS annotations support in Bnd.

bjhargrave commented 6 years ago

That activate method is a method. It is not a constructor.

On Mon, Sep 17, 2018 at 18:29 ncouse notifications@github.com wrote:

Using the latest snapshot build 20180917.142949 of bnd-maven-plugin, I can see that activation fields do work, but constructor injection does not seem to work.

@Activate private BundleContext ctx;

@Activate public void activate(@Reference SimpleService simpleService) { }

The error is @Reference cannot be used for method parameters

This is based on @bjhargrave https://github.com/bjhargrave slides ( https://www.slideshare.net/bjhargrave/constructor-injection-and-other-new-features-for-declarative-services-14

  • Slide 9)

Constructor injection was mentioned in first comment as part of this work. Was it included?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bndtools/bnd/issues/2179#issuecomment-422082316, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQ8sr73VQnidxBDEwzarGMnBkUkKMBnks5ub83UgaJpZM4QBICw .

-- BJ

ncouse commented 6 years ago

That activate method is a method. It is not a constructor.

Thanks, I realised my mistake just after posting, and had deleted the original post (but not before you noticed :) )

I have been waiting for this feature, as it makes unit testing of component classes much easier. For cleaner code, developers prefer field injection to method injection. However the former makes unit testing more difficult. Constructor injection is a nice compromise.

The only disadvantage I see if that with constructor injection, you can't use field injection, and some developers may expect it to work, but it makes sense that it cannot work.

bjhargrave commented 6 years ago

The only disadvantage I see if that with constructor injection, you can't use field injection, and some developers may expect it to work, but it makes sense that it cannot work.

You can use both if you wish.

ncouse commented 6 years ago

The only disadvantage I see if that with constructor injection, you can't use field injection, and some developers may expect it to work, but it makes sense that it cannot work.

You can use both if you wish.

Well not in the activator/constructor though (as expected, since the object is not constructed yet). I have verified that the field injection has happened after the object was constructed, and was available in any business methods.

I presume this binding happens before Declarative Services finishes activation, so that we are guaranteed that they are available once a business method is called.

bjhargrave commented 6 years ago

Obviously, the object must first be constructed. See https://osgi.org/specification/osgi.cmpn/7.0.0/service.component.html#service.component-activation for the order of things during activation.