search

eclipse-equinox / equinox.bundles

Eclipse Public License 2.0
8 stars 16 forks source link

Replace embedded OSGi-prefs sources by bundles from Maven-Central #39

Closed HannesWell closed 2 years ago

HannesWell commented 2 years ago

Similar to the other changes explicitly discussed in https://github.com/eclipse-equinox/equinox/issues/18, this replaces the embedded OSGi-sources of org.eclipse.equinox.preferences by the corresponding osgi-bundle from Maven-Central.

As a side effect this updates the versions of

Since many other plug-ins are likely requiring o.e.equinox.preferences and are using the classes in org.osgi.service.prefs that bundle is required and re-exported.

HannesWell commented 2 years ago

This requires https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/pull/247 to build successfully. But since we have not yet talked about this particular Plug-in I would like the clarify first if this change should be done or if there are reasons that speak against it.

Additionally some more features have to be updated to keep the sources of org.osgi.service.prefs available.

bjhargrave commented 2 years ago

This requires eclipse-equinox/equinox.framework#39 to build successfully.

This is PR 39. Perhaps you mean another PR?

HannesWell commented 2 years ago

This requires eclipse-equinox/equinox.framework#39 to build successfully.

This is PR 39. Perhaps you mean another PR?

Ups, indeed. I meant: https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/pull/247

Since you require the osgi prefs API bundle at 1.1.0, the import package version should match. It is currently 1.1.1

Thanks for the hint. Shouldn't I remove the Import-Package entirely to not have bot Import-Package and Require-bundle of org.osgi.service.prefs?

bjhargrave commented 2 years ago

Shouldn't I remove the Import-Package entirely to not have bot Import-Package and Require-bundle of org.osgi.service.prefs?

I am not sure if Require-Bundle with reexport allows for substitution? @tjwatson? If it does, then having the Import-Package is good since it provides more room for the resolver.

tjwatson commented 2 years ago

I am not sure if Require-Bundle with reexport allows for substitution? @tjwatson? If it does, then having the Import-Package is good since it provides more room for the resolver.

I think this is a grey area. I know the resolver will make sure the class space is consistent for the importing bundle. But I don't recall what would happen for the bundles requiring the re-exporter (org.ecipse.equinox.prefs in this case). I don't recall if they get wired to the target of the import package wire or directly to the reexported bundle.

HannesWell commented 2 years ago

Given that the build succeeds now (the dependencies were added to the Eclipse-SDK-target), do you have any objections against this change? If not I would like to submit it.

I will created corresponding PRs to include the org.osgi.service.prefs into those features that contain org.eclipse.equinox.preferences already to make the sources available.

HannesWell commented 2 years ago

Thanks everyone!

HannesWell commented 2 years ago

For completeness, I created/updated the following PRs to include org.osgi.service.prefs into those features that already include o.e.equinox.preferences:

tjwatson commented 2 years ago

We should move the application admin tests out of org.eclipse.osgi.tests and into some dedicated org.eclipse.equinox.app.tests project. Of coarse the test failures in the org.eclipse.osgi.tests would have been caught during the this PR build if we had combined framework and bundles repos also.

HannesWell commented 2 years ago

We should move the application admin tests out of org.eclipse.osgi.tests and into some dedicated org.eclipse.equinox.app.tests project. Of coarse the test failures in the org.eclipse.osgi.tests would have been caught during the this PR build if we had combined framework and bundles repos also.

Sounds good. Having finer grained test-plugins is probably at all not a bad idea. Yes combined repos would have helped too, but the merge is not too far away.