Community
Participate
Working Groups
Build Identifier: N20100727-2000 Currently p2 extensibility is implemented on top of extension points (Eclipse standard extension mechanism). Even p2 is a self-contained functionality, it's almost impossible to use it outside Eclipse IDE due to the extension points usage. To keep such a powerful feature as extensibility, p2 could be implemented on top of OSGi standard extension mechanism - for example using publish/bind model of OSGi services. Such refactoring would make p2 consumable not only by Eclipse IDE, but also by any other party who is looking for a reliable provisioning. To keep the backwards compatibility, a new bundle could be introduced to "translate" custom-defined extension points (only p2 related) into OSGi services. Reproducible: Always
Created attachment 178910 [details] Change the existing components to use and consume OSGi services instead of extension points Corresponding tests adapted
Created attachment 178911 [details] The bundle registers existing extension points as OSGi services on its start
*** Bug 304854 has been marked as a duplicate of this bug. ***
The aim here is to replace Eclipse extension points in p2 with OSGi services. The change will affect only Eclipse extension points registered on behalf of p2 bundles. Backward compatibility will be ensured by a new bundle which will translate custom defined Eclipse extension points into OSGi services. Here's how this change would look like if we take for example discovering touchpoint entries in p2 engine bundle: Current state: private synchronized Map<String, TouchpointEntry> getTouchpointEntries() { if (touchpointEntries != null) return touchpointEntries; IExtensionPoint point = RegistryFactory.getRegistry().getExtensionPoint(EngineActivator.ID, PT_TOUCHPOINTS); IExtension[] extensions = point.getExtensions(); touchpointEntries = new HashMap<String, TouchpointEntry>(extensions.length); for (int i = 0; i < extensions.length; i++) { try { IConfigurationElement[] elements = extensions[i].getConfigurationElements(); for (int j = 0; j < elements.length; j++) { String elementName = elements[j].getName(); if (!ELEMENT_TOUCHPOINT.equalsIgnoreCase(elementName)) { reportError(NLS.bind(Messages.TouchpointManager_Incorrectly_Named_Extension, elements[j].getName(), ELEMENT_TOUCHPOINT)); continue; } String id = elements[j].getAttribute(ATTRIBUTE_TYPE); if (id == null) { reportError(NLS.bind(Messages.TouchpointManager_Attribute_Not_Specified, ATTRIBUTE_TYPE)); continue; } if (touchpointEntries.get(id) == null) { touchpointEntries.put(id, new TouchpointEntry(elements[j])); } else { reportError(NLS.bind(Messages.TouchpointManager_Conflicting_Touchpoint_Types, id)); } } } catch (InvalidRegistryObjectException e) { //skip this extension } } return touchpointEntries; } Rough proposal: public Touchpoint getTouchpoint(String typeId, String versionRange) { if (typeId == null || typeId.length() == 0) throw new IllegalArgumentException(Messages.TouchpointManager_Null_Touchpoint_Type_Argument); ServiceReference<Touchpoint> touchPointRef; try { touchPointRef = bundleContext.getServiceReference(Touchpoint.class, "(" + ATTRIBUTE_TYPE + "=" + typeId+ ")"); if(touchPointRef == null) { return null; } return bundleContext.getService(touchPointRef); } catch (InvalidSyntaxException e) { e.printStackTrace(); return null; } } The proposed solution has some advantages: - it would be easier for end user to extend p2 with custom logic since the p2 extensions would be now defined in unified way - interface name and ID key - it would be possible to look for p2 extension on demand without performance impact; no need to gather and keep all registered extensions before they are actually needed - simplicity; p2 will use only one discovery mechanism Any objections to that approach?
As discussed during today's call, let's go ahead with the proposed approach and see what happens :) In order to keep things flowing, let's first do the complete work for one extension point, review, comment, agree on the approach and then see for the others.
Move all 3.8 bugs to Juno.
How could an application or product customize or remove functionality that's being surfaced this way? ex, with extensions, and product can take advantage of adapter hooks or http://wiki.eclipse.org/Product_Customization to prevent or enable certain contributions. The enabling part makes sense to me, it just has to register a service. Can the product prevent certain services from starting? PW
(In reply to comment #7) One difference between services and extension points is that extension points become available after the provider is resolved while in order to have services available you need to start the provider. In that sense if "unwanted" providers are not declared to be started in the product, corresponding services won't be available at runtime. A limitation of that approach is that you cannot filter which services to become available and which not without an additional effort. I checked the link you provided - it would be interesting to discuss which of the problems addressed with adaptor hooks and product customizations are relevant in p2 context and how they could be covered by the proposed approach. Your concerns are more about preventing some extensions of one provider from being registered or about preventing a provider to register any extensions?
Created attachment 198026 [details] Touchpoint manager modifed to discover touchpoints as OSGi services
Created attachment 198027 [details] Touchpoint extensions traslator Translates toucpoints declared as extensions to OSGi services
Let's start with touchpoint extension point. Since it's very important to keep things backward compatible I decided to keep productive extensions (eclipse and native touchpoints) as they are. It helps validating that the new discovery mechanism works properly with extensions and to test the translation logic (extensions to services) as a beginning. I modified correpsponding tests to register test extensions in the new way so new touchpoints registered as services will be discovered as well. The next step will be to register eclipse and native touchpoints as services and create a dedicated test to guarantees compatibility. I didn't managed to edit All p2 Tests.launch run configuration in a way to start the new compatibility bundle "org.eclipse.equinox.p2.extension.point.compatibility" so in order to run the tests with the new stuff you have to make sure the new bundle is started somehow in the very beginning. I'll be glad if you can give me any hints how this inclusion is supposed to be done. There are lots of details to discuss - we can start with the question: Should we allow touchpoints with the same id and different/same versions? I let myself implement "prefer higher version" approach but it's just meant to be a starting point for discussions. Looking forward to your comments.
I went through this and here are some questions / concerns / feedback: - When is the new bundle supposed to be activated? Should every bundle that use to use extensions (e.g. engine) cause the bundle to be activated in its own activator to ensure that extensions are properly registered before anything interesting happen? - The service lookup code does not seem to handle the case where there are multiple services registering the same touchpoint. The code that has been moved into the new bundle only covers the case of things contributed by extensions. The duplication detection logic should reside in the service lookup. - Why does p2.core has a dependency on the new bundle? - In which feature should this new bundle live? - Could you show a couple of examples on the markup that replaces the registration of extensions? - Not sure I like the constants on ITouchpointType. Do we expect ppl to be able to use those? What do you think? Overall I think the direction is good.
I just realized that there is something quite profound being changed. Whereas the extension based code creates new instances of the touchpoint each time, the service approach reuses the same instance which is not desirable. A factory needs to be introduced to preserve this behaviour. I'm also curious to see how that changes the work required by extenders.
(In reply to comment #12) > - When is the new bundle supposed to be activated? Should every bundle that use to use extensions (e.g. engine) cause the bundle to be activated in its own > activator to ensure that extensions are properly registered before anything > interesting happen? The new bundle is supposed to be activated with other p2 bundles. In order to prevent bundle start ordering I would suggest to use dependency injection instead of direct lookup in the activator. If the new bundle registers a marker service (e.g IExtensionPointTranslator), we could add an optional reference to it in engine component (OSGI-INF/engine.xml). Thus p2 will work with and without the new bundle. To answer the second question - each extension point should be aware of the existense of the new bundle - the benefit is clear separation of concerns. > - The service lookup code does not seem to handle the case where there are > multiple services registering the same touchpoint. The code that has been > moved into the new bundle only covers the case of things contributed by > extensions. The duplication detection logic should reside in the service > lookup. Some of the code is moved to the new bundle. But it's still the touchpoint manager which is responsible to decide what to do with multiple touchpoints of the same type. In the attached patch the highest version is returned if touchpoints have versions but you're right that registration of two equal extention points is not handled. I modified the code to log an error as it was before. I'm thinking about use cases broken by having two same touchpoints registered. Are you aware of such? > - Why does p2.core has a dependency on the new bundle? Because I wanted to use LogHelper from p2.core in the new bundle but the corresponding package is exported only to friends. > - In which feature should this new bundle live? To be honest I don't know. On one hand people should be able to get p2 bundles without the new bundle, on the other p2 feature which is used by Eclipse should have the new bundle. What are the options? > - Could you show a couple of examples on the markup that replaces the > registration of extensions? I modified Eclipse touchpoint to register itself via ds. Also touchpoint tests are modified to register test tochpoints programatically. Is this sufficient example? > - Not sure I like the constants on ITouchpointType. Do we expect ppl to be > able to use those? What do you think? I think people should use the new constants - in order to define new extensions people should know how they are supposed to be identified. Using publicly available constants seems to me clearer and more deterministic compared to string copy. Also unifying the way extensions are identified would make p2 lookup code cleaner. Why would you like to hide that information? Or your concern is more about changing the interface? (In reply to comment #12) > I just realized that there is something quite profound being changed. Whereas > the extension based code creates new instances of the touchpoint each time, > the service approach reuses the same instance which is not desirable. A > factory needs to be introduced to preserve this behaviour. The new patch introduces factories instead of direct instantiation and a simple test case to validate that different touchpoint instances are returned. Do you think it would be useful to have more complex test about that? > I'm also curious to see how that changes the work required by extenders. To me conceptually the change is in making things explicit. Eclipse touchpoint and tests are examples showing the technical aspect.
Created attachment 198332 [details] Touchpoint factories replace direct instantiation
Created attachment 198333 [details] Compatibility bundle registers factories instead of a single touchpoint instance for existing extension points
(In reply to comment #8) > > I checked the link you provided - it would be interesting to discuss which of > the problems addressed with adaptor hooks and product customizations are > relevant in p2 context and how they could be covered by the proposed approach. > > Your concerns are more about preventing some extensions of one provider from > being registered or about preventing a provider to register any extensions? My main question is about the ability of a product to pick and choose what gets consumed. We often don't have a choice but to start the bundle, as they contain useful classes and implementations that we need to use. The adapter hook allows the engine to not see some or all extension points coming from a specific bundle. Can you make some of the services go away? It would be a change if you *had* to take those service contributions because the bundle was started (in our case, all it takes is one class access). PW
I've reviewed the new code and I think it is good. I've raised below a few minor concerns that will have to be addressed but that's it. I'll let a chance to others CC'ed to review before I commit a corrected version of the patch early next week. - I'm not a fan of the name org.eclipse.equinox.p2.extension.point.compatibility and would rather use something like org.eclipse.equnox.p2.compatibility.extensionregistry or compatibility.registry, ... other suggestions welcome :) - Make sure the copyright headers are all clean - Add javadoc to the factory class > The new bundle is supposed to be activated with other p2 bundles. In order to > prevent bundle start ordering I would suggest to use dependency injection > instead of direct lookup in the activator. If the new bundle registers a marker > service (e.g IExtensionPointTranslator), we could add an optional reference to > it in engine component (OSGI-INF/engine.xml). Thus p2 will work with and > without the new bundle. This is a nice idea. Let's add this. [about duplication of providers] > I'm thinking about use cases broken by having two same touchpoints registered. > Are you aware of such? I have never run into this situation before. > > - Why does p2.core has a dependency on the new bundle? > Because I wanted to use LogHelper from p2.core in the new bundle but the > corresponding package is exported only to friends. I had missed the friendly aspect of the relationship. > > - In which feature should this new bundle live? > To be honest I don't know. On one hand people should be able to get p2 bundles > without the new bundle, on the other p2 feature which is used by Eclipse should > have the new bundle. What are the options? At this point I would go with p2.core.feature since people consuming it will expect their actions provided with extensions to just work. > > - Could you show a couple of examples on the markup that replaces the > > registration of extensions? > I modified Eclipse touchpoint to register itself via ds. Also touchpoint tests > are modified to register test tochpoints programatically. Is this sufficient > example? Yes thanks. It really represents more files for ppl to mess around with, but I guess they'll get over it and they can always resort to use extension if they want. Btw, the patch is missing the change to the build.properties for the ds file to be shipped as part of the binary. > > - Not sure I like the constants on ITouchpointType. Do we expect ppl to be > > able to use those? What do you think? > I think people should use the new constants - in order to define new extensions > people should know how they are supposed to be identified. Using publicly > available constants seems to me clearer and more deterministic compared to > string copy. Also unifying the way extensions are identified would make p2 > lookup code cleaner. > Why would you like to hide that information? Or your concern is more about > changing the interface? I wanted to hide because I had not really seen the use for it. It seems that it is only of use when you register a service through code, which currently is only done in the test. Given the purpose of these constants (really tailored to the extenders), they would be better located on Touchpoint rather than ITouchpointType. > The new patch introduces factories instead of direct instantiation and a simple > test case to validate that different touchpoint instances are returned. Do you > think it would be useful to have more complex test about that? Thanks for this change. The test you added probably suffice.
(In reply to comment #17) > (In reply to comment #8) > My main question is about the ability of a product to pick and choose what gets > consumed. > We often don't have a choice but to start the bundle, as they contain useful > classes and implementations that we need to use. There is no need to start a bundle in order to consume classes - as soon as the bundle is resolved it has a classloader and its exported resources are availlable. Do I miss something? Even if you need to start a bundle (let's take touchpoints for example) could you please explain how undesired touchpoint would harm your environment? If there is no touchpoint data to be processed by it there's no difference from p2 pov whether a corresponding service would be registered or not. If there is, maybe the consumer expects to have it processed. What are the benefits of forbidding touchpoint registration in p2?
(In reply to comment #18) I updated the patch with your comments, thank you for the guidence. There is just one exception: It seems that dalaying p2.engine start after the start of the new bundle via service(engine component depends on IExtensionPointTranslator) with cardinality 0..1 cannot be done. When p2.engine starts before the other bundle the behaviour will be the same as if there's no reference between the ds components. 1..1 would do but we don't want to have that dependency explicit since p2.engine could live without it. So we should make sure somehow that compatibility bundle is started before any provisioning operation take place.
Created attachment 199074 [details] touchpoints as services - comments addresses
Created attachment 199075 [details] tests
Created attachment 199076 [details] compatibility bundle
Could someone of the committers take a look at the latest patch? It's not that different from the previous one but I'm afraid that the official code line is moving forward and since the patch is relatively big it would get more and more difficult to be merged back. Thanks in advance.
I'm unlikely going to replace the extension registry with OSGi services. Pascal , Katya, (or others), if you want to push forward with this, feel free to add it back to Kepler.
Sorry, but there is just no resource to step forward and make fundamentally complex changes such as this, and then support that long term. :-(
