Community
Participate
Working Groups
In order to be consistent with the static configuration of subsystems through plugin.xml, ISubSystemConfiguration.getSystemTypes() MUST return the same as SubSystemConfigurationProxy.getSystemTypes(). If this were not the case, RSE would need to instantiate subsystem configurations early in order to check the return value of this method and know whether a subsystem icon may be shown below a system type or not. But we'd like to get rid of such early plugin activation through bug #181939. Therefore, this method should be made final (disabling overriding), and deprecated, mentioning that the SubSystemConfigurationProxy.getSystemTypes() should be used instead.
Uwe give your comment on this because of your comment in SystemHostCombo line 220: // Do not call ISubSystemConfigurationProxy.getSystemTypes() directly. If // some one has overriden ISubSystemConfiguration.getSystemTypes(), the // proxy cannot return the correct list anymore. This is especially important // if the systemType <--> subsystemConfiguration association is dynamic! I think that this comment is not valid, because it requires instantiating the ISubSystemConfiguration early and unnecessarily, and we want to avoid this.
Martin, the comment is valid. The association of system types with subsystems is extremely static via the current subsystem plugin.xml marked. The markup is not capable of matching a unknown system type. The problem is how to let tell the subsystem that is available for all system types which are instances of IWRSystemType? Matching the system type id's like com.windriver.* is not sufficient as we do not have unique system type ids and there exist known 3rdParty extensions where it is not practicable to apply a unique id naming scheme to all of them. Nonetheless all that extensions must still work without requiring changes. That requirement is the main reason for having introduced the dynamic system types to RSE. The only final solution to that problem is to extract the system type vs. subsystem configuration into a separate bindings extension point which should use org.eclipse.core.expressions framework to bring the full power of enabledWhen, visibleWhen and similar markups to the extension point. Making ISubSystemConfiguration.getSystemTypes() final or even remove it without providing a highly configurable alternative will break our (Wind River) integration and some functionality of RSE with respect of dynamic system types. In case of the SystemHostCombo, using ISubSystemConfigurationProxy.getSystemType() would make RSE behaving inconsistently for dynamic system types vs. static system types.
(In reply to comment #2) > not capable of matching a unknown system type. The problem is how to let tell > the subsystem that is available for all system types which are instances of > IWRSystemType? SubSystemConfiguration.getSystemTypes() { return proxy.getSystemTypes(); } and SubSystemConfigurationProxy.getSystemTypes() considers all the system types, including dynamically contributed ones, and checks whether they pull in the configuration. So, honestly, I do believe that your application should still work, because you declare dynamic system types, which pull in the subsystem configurations you need. The advantage of this is, that we only need to instantiate the dynamic system type provider, and do not have to instantiate all the SubsystemConfiguration instances. Given that, could you review again?
You are going to rip out the possibility of having a contributed 3rdParty subsystem compatible with all dynamic system types of a vendor and the vendor 3rdParty extension. In other words, if the subsystem is not contributed from the dynamic system type provider, the subsystem provider does have a impossible task to get the list of all possible system type id from the vendor(s). Your call. Wind River is most likely the only dynamic system type provider yet and there are no known 3rdParty subsystem contribution for Wind River system types yet. Just be aware of that case and judge in open source if you want break the support of that case. I checked our integration code and can now confirm that we do not (or not anymore) override ISubSystemConfiguration.getSystemTypes().
Are you talking about this scenario: Vendor X contributes a dynamic system type provider which generates system types X1, X2, X3 and links them against subsystem configuration xsub. Vendor Y would like to write a new subsystem configuration ysub that should also work with the dynamic system types X1, X2, X3 and probably others being contributed by X. Currently, Y can only statically declare the link by adding X1, X2, X3 into his subSystemConfiguration. So if the dynamic provider ever changes and contributes new system types that Y does not know in advance, he'll miss out on those. Hm... I'd still like to see this situation adressed without the need for instantiating ALL the subsystem configurations, including those that were never meant to be contributed dynamically. For me it looks like we'll need an extension to the plugin.xml markups: (a) a new systemTypeBinding extension point that links systemType and/or systemTypeProvider against subsystemConfiguration; or, (b) an extension to the subsystemConfigurations extension point, where (b1) users can give a "systemTypeProviders" slot, or (b2) users specify "dynamic=true" If we do (b2) then the link between systemType and subsystemConfiguration could be made statically by default; and only if the markup says dynamic==true, it could instantiate the SubSystemConfiguration and check its getSystemTypes() method in order to see what is really desired. The corresponding code to check for "dynamic=true", instantiate and check, could all be iside the current implementation of SubSystemConfigurationProxy.getSystemTypes() I personally like (b2) best, what do you think about these alternatives? I'd like to file an enhancement request for the alternative we like best.
>Are you talking about this scenario: > >Vendor X contributes a dynamic system type provider which generates system >types X1, X2, X3 and links them against subsystem configuration xsub. > >Vendor Y would like to write a new subsystem configuration ysub that should >also work with the dynamic system types X1, X2, X3 and probably others being >contributed by X. > >Currently, Y can only statically declare the link by adding X1, X2, X3 into his >subSystemConfiguration. So if the dynamic provider ever changes and contributes >new system types that Y does not know in advance, he'll miss out on those. I'm talking exactly about that kind of scenario. >(a) a new systemTypeBinding extension point that links systemType and/or > systemTypeProvider against subsystemConfiguration; or, >(b) an extension to the subsystemConfigurations extension point, where > (b1) users can give a "systemTypeProviders" slot, or > (b2) users specify "dynamic=true" If you handle (b2) as proposed below, it has the potential to solve the activation problem with keeping the current way of doing for dynamic system types achieved by a quite limited change. So go for it even if my personal preference is with the more generic (a), but it's way more work we don't have the resources available for doing it :).
Ok... regarding (a), I had some other ideas about "deriving" system types, such that one could contribute a new system type X1 derived from X where X1 would "inherits" all the subsystem configurations registered against X. In other words, X1 would be marked "compatible" with X -- it's also described in bug #170918. Whether we can do that by extending the currently existing extension points, or by providing a new systemTypeBindings extension point, I'm not sure. My feeling is that in order for subsystem S to properly work with systemType T, either T must know S or S must know T. I'm not sure if it can work when S has no idea about T and vice versa but some 3rd party linked them up via a binding. Do you have a concrete use case or application where a separate binding would be needed? Anyways, deferring the change to Future for now since it involves an API change.
For the same reasons: * ISubSystemConfiguration must always be consistent with SubSystemConfigurationProxy, * Provide lazy loading of ISubSystemConfiguration, use the Proxy only wherever possible some other attribute getters on ISubSystemConfiguration should also be removed, or made final in the abstract base class SubSystemConfiguration; when making them final, Class Javadoc of ISubSystemConfiguration should say "not intended to be implemented directly": getVendor() getName() getDescription() getId() getCategory() Note that for all of these, the Javadoc on SubSystemConfiguration already says: "This comes from the xml "id" attribute of the extension point." and Javadoc on ISubSystemConfiguration already says: "Matches value in name attribute in extension point xml" Since API Javadoc already says that the result must match the value in plugin.xml, making the corresponding methods final seems to not actually be an API change (API was always documented like that). Exceptions to this are public String getCategory(); public IRSESystemType[] getSystemTypes(); which do not have this Javadoc markup in ISubSystemConfiguration as per 2.0.
Created attachment 74358 [details] Patch adding Javadoc to forbid overriding offending methods Attached patch adds Javadoc to ISubSystemConfiguration and SubSystemConfiguration, warning extenders that the methods in question should not be overridden.
Patch committed.
So, it looks like the remaining issue where the full flexibility of ISubSystemConfiguration.getSystemTypes() is needed (beyond what one can express in plugin.xml), is custom systemType <-> subSystem associations. We must at any rate avoid a scenario where lazy loaders looking at the Proxy only yield different results (i.e. different associations) than when the config is loaded. Perhaps the best solution for now is this: * IF the SubSystemConfigurationProxy has a static list of systemTypes (expressed as a list), THEN ISubSystemConfiguration.getSystemTypes() must not be overridden, to ensure the same result. * OTHERWISE, if the SubSystemConfigurationProxy uses a "*" to indicate the config can be registered against all system types, THEN RSE must load the ISubSystemConfiguration eagerly in order to be able and make the association by code. In other words, the "*" association is redefined from "all system types" to "dynamically selected system types, fallback to all if no dynamic implementation is provided". For the other methods in question as per comment #8, it looks like we should make them final / deprecated indeed, in order to avoid the scenario where different results are found before and after loading the config. One exception might be the description, which could be made holding some dynamic status info about the subsystem once it is loaded (but a dummy placeholder while not loaded). Assigning M6 since this seems an important API change towards lazy plugin loading as per bug 181939.
I decided against marking these methods final, because it is likely that in a future release we might want dynamically contributed subsystems that do not directly relate to a SubSystemConfigurations extension point but are created by a SubSystemConfigurationProvider. Changed Summary, previous value was: [api] SubSystemConfiguration.getSystemTypes(), getName(), getId() etc. should be made final or removed What I did instead was use the new API Tooling @noextend along with Javadoc to discourage overriding these methods in a "soft" manner: getName() getId() getVendor() getDescription() getCategory() getSystemTypes() Consequently, this is not an API change any more. Keeping open for updating Javadoc once again according to comment #11.
Not for M7
Comment on attachment 74358 [details] Patch adding Javadoc to forbid overriding offending methods Patch has been obsoleted by new API Tooling comments.
The issue has been fixed in M7 already.
