eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
861 stars 784 forks source link

Inconsistent import of `org.osgi.service.component` package #5954

Open svilenvul opened 6 years ago

svilenvul commented 6 years ago

While trying to build the Eclipse SmartHome in an IDE that is not provided by ESH, I got some compilation errors in several bundles.

It turned out that some bundles are using org.osgi.service.component.annotations.Component annotation, but do not import the corresponding package in their manifest file. So I had to add the imports.

Initial list (probably not full) of files where the imports are missing is:

    modified:   bundles/core/org.eclipse.smarthome.core.voice/META-INF/MANIFEST.MF
    modified:   bundles/io/org.eclipse.smarthome.io.console.eclipse/META-INF/MANIFEST.MF
    modified:   bundles/io/org.eclipse.smarthome.io.console/META-INF/MANIFEST.MF
    modified:   bundles/io/org.eclipse.smarthome.io.rest.sitemap/META-INF/MANIFEST.MF
    modified:   bundles/io/org.eclipse.smarthome.io.transport.upnp/META-INF/MANIFEST.MF
    modified:   bundles/model/org.eclipse.smarthome.model.item/META-INF/MANIFEST.MF
    modified:   extensions/binding/org.eclipse.smarthome.binding.ntp/META-INF/MANIFEST.MF
    modified:   extensions/binding/org.eclipse.smarthome.binding.yahooweather/META-INF/MANIFEST.MF

I see that the majority of the bundles import the package (org.osgi.service.component).

If I don't miss something, this looks like a perfect candidate for the static analysis tool.

maggu2810 commented 6 years ago

You don't use that annotations by intention IIRC because we inject older annotations...

maggu2810 commented 6 years ago

reference: #4624

maggu2810 commented 6 years ago

@kaikreuzer I realized that the Eclipse IDE contains a setting about the DS version if annotations are used. It is set to 1.3. I assume this is not correct if we want to ensure compatibility to OSGi 4.2

kaikreuzer commented 6 years ago

@maggu2810 I wasn't yet aware of this setting, but I agree, it should be 1.2. But this is only relevant for the generation of the XMLs in the IDE, which we anyhow do not commit to git, right?

kaikreuzer commented 6 years ago

Wrt the issue: The import is not required at runtime, thus it does not have to be in the manifest. The annotations are available on the classpath for the generators, so they do not require it in the manifest.

maggu2810 commented 6 years ago

That is correct, but I am pretty sure (at least I have read comments) that people also use the export function of the Eclipse IDE to get bundles of their current code base.

svilenvul commented 6 years ago

OK, I understand that they are not required at runtime, but I wonder how to convince Eclipse IDE that the bundles should compile without these imports.

kaikreuzer commented 6 years ago

people also use the export function of the Eclipse IDE to get bundles of their current code base.

Good point, so for this as well as for IDE development purposes, this change definitely makes sense.

I wonder how to convince Eclipse IDE that the bundles should compile without these imports.

Simple: In Preferences->Plug-in Development->DS Annotations, the "Generate descriptors" needs to be activated. This automatically makes the annotations available on the classpath.

svilenvul commented 6 years ago

@kaikreuzer, thanks!

kaikreuzer commented 6 years ago

Which effectively means for your issue: We might want a warning in SAT if people HAVE such an import.