Community
Participate
Working Groups
When creating a plugin that contributes to the application model via model fragment or model processor, you need to have a plugin.xml in place to register that. As we have already a lot of replacements for extension points, like the whole application model and even imperative expressions, it would be consistent to even get rid of the org.eclipse.e4.ui.workbench extension point. At least on the contribution side it would avoid the need for a plugin.xml file. For this I would like to introduce two new component interfaces that actually represent the possible configurations of the available extensions: - IModelFragmentContribution - IModelProcessorContribution Instead of the plugin.xml, a fragment could then be simply registered like this: @Component public class MyTestFragment implements IModelFragmentContribution { } a model processor contribution could look like this: @Component public class ExampleProcessorContribution implements IModelProcessorContribution { @Override public Class<?> getProcessorClass() { return ExampleProcessor.class; } } That of course means to have proper default methods in place to make the component implementation as lean as possible. Inside ModelAssembler the components can get injected using the @Service annotation. Of course the implementation should support extension points and the services approach for backwards compatibility.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/177574
Created attachment 286015 [details] Example project With the attached example project the implementation can be tested. It is a plain e4 application. The bundle org.fipro.eclipse.tutorial.inverter contains the contributions via fragment.xml and a model processor. It is not activated lazily, so on startup the part is missing. Activating the bundle via console the part will show up, stopping the bundle again the part will be dropped again. It also shows existing issues, e.g. the menu contribution is not added dynamically. If you activate the bundle lazily you see that the menu contribution is there. So you can see that the existing behavior on initial building the application model still works, but for the dynamics there are still some issues. Also the remove on bundle stop is not complete. It currently only removes ui elements, but commands and handlers are not removed. Maybe it would be worth to implement a remove() in ModelFragment similar to the merge() that removes all elements from the application model that were contributed by the fragment. But surely there are several use cases to consider, e.g. if the fragment overrides an existing element, how to bring back the previous element after the one from the fragment is removed? I need support on the application model / EMF topics for the dynamic removal and also for some of the adding use cases. IMHO dynamically adding a menu on top level was always an issue. I am not sure if this patch has to fix all the dynamic issues directly, or if we could start with this and extend the dynamic application model processing step by step.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/177574 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=898e59bd172fe2709193c82f9898bf68f63ce21b
New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/179290
I've looked at the N&N. I'm a bit unsure about whether adding `Model-Fragment:` header in MANFIEST.MF is a good idea. This header will be unknown to PDE, undocumented, and really opens the door to MANIFEST.MF becoming yet-another-entry-point for extensibility, and that conflicts with my "less is more" feeling about making a Platform scale. Anyway, it's there now, and I'm not going to argue against it further, I'll just keep this thought deep inside me. However, it's very important to have it documented as a new form of extensibility in the documentation, just like we added a page about OSGi services recently. All other documentation entries that reference model fragments and the extension point will also need to now reference the documentation of this new MANIFEST.MF header and the documentation of the OSGi service.
I haven't followed the latest changes but can someone summerize why the MANIFEST-Header path has been chose and DS is not enough? I have to agree with Mikael that we then now have 3 ways to provide extensibility: * plugin.xml * DS * this header thing here where I would have hoped we DS would be the only one in the future. BTW: Why is the method named ModelAssembler#set/unsetModelProcessorContribution/ the default naming scheme would be register/unregister for 1..n cardinality.
Well Christoph argued that it would be "more OSGi way" to have the MANIFEST header for registering instead of having to implement a service for providing a reference to a file. So maybe he can explain why he asked to change it. From a plain OSGi perspective it is correct. And BTW a fragment can not be registered via DS, only a processor can. Regarding the method names, my fault, I will change it. I can also revert the MANIFEST header thing with the DS approach. IMHO it will make the implementation also easier as there is no need for a BundleTracker then. But as Christoph had strong feelings about this, IMHO he should answer your questions on that.
Please don't forget to check SDK test results after merging of changes. We have 17 new test fails with Java 16, I assume that this bug is the root cause. See bug 572839 for details.
(In reply to Mickael Istria from comment #5) > I'm a bit unsure about whether adding `Model-Fragment:` header in > MANFIEST.MF is a good idea. This header will be unknown to PDE, > undocumented, and really opens the door to MANIFEST.MF becoming > yet-another-entry-point for extensibility, and that conflicts with my "less > is more" feeling about making a Platform scale. > Anyway, it's there now, and I'm not going to argue against it further, I'll > just keep this thought deep inside me. You were added as a reviewer and could have come into the discussion earlier. If you look at the comments of the contribution you will notice that we had quite a long discussion about that topic. As nobody stepped in to support my idea of using the DS approach for fragments, I was kind of "forced" to change it. No blaming, just a notice that you could have given your thoughts earlier. ;-) I don't want to blame Christoph here. From a plain OSGi point of view he is right that the manifest header is the cleaner solution. And it also avoids the creation of a service class for registering a file. But from the Platform point of view and the "less-is-more" feeling, the DS approach would fit better. Anyhow, we are not in a freeze period now, and the solution for the DS approach for fragments is in the history. So I could add that again by keeping the dynamic support the same way it works now. I don't think "it's there now, and I'm not going to argue against it further" does apply now. We are still able to change. I don't want to introduce something that the majority don't like. @Mickael/@Tom/@Christoph Please discuss and let me know which direction to go. IMHO also the DS approach would work. From a plain OSGi perspective probably not perfect, but from a Platform perspective and also the perspective of adopters maybe the better solution.
(In reply to Dirk Fauth from comment #9) > No blaming, just a notice that you could have given your thoughts earlier. > ;-) I'm dealing with dozens of reviews, and not only trivial ones, daily. To this, add my own tasks, in term software development and organizational stuff. As a consequence, I want to clarify that I cannot review in details everything, and that I simply ignore many cases where I'm added as a reviewer on a bug change without a clear and concise explanation of what specifically I need to consider. This is one instance of those. It's indeed my fault, and I can't blame people for assuming I'm doing complete reviews just as they add me as a reviewer on Gerrit. It's how things are and I don't really see them changing in a short-term. On the other hand, I'd like to also remind that there is a mailing-list that should be used to suggest important design changes (eg new form of extensibility) which need wider consideration and that are identified up front as critical or tricky. This could have been a good candidate. However, none of this is a big deal here: it's OK that you & Christoph merged it from a process and techical POV (no regression, good code...), you did things right. And it's OK for anyone to provide feedback after the merge. It's not best, but it's all acceptable and usually works fine; and I'm confident this is yet another case where this is working fine. > I don't think "it's > there now, and I'm not going to argue against it further" does apply now. We > are still able to change. I don't want to introduce something that the > majority don't like. OK. So the question is "Is the addition of the MANIFEST.MF header bringing value *now* that's hard to get otherwise, and that is worth opening a new area of extensibility? Does it solve some current or very near future problems?". If there is enough agreement on "yes", we keep it and cascade the decision to maintenance, documentation, to PDE and so on (eg "pay the cost"); if there is more substantial weight on "no", then we remove it. From my (limited) POV, I see no need for a MANIFEST header, but I'm not building any other RCP app, so I may be missing an important and pragmatic reason that makes extensibility via MANIFEST.MF worth it.
@Mickael As I said, no blaming. I am sure you have quite a lot of stuff to work on. And yes, writing on the mailing list would have been better. But my initial goal was to give an option to get rid of extension points, which we do in several places already. It has grown in the patch itself without further notice unfortunately. For anyone interested in the topic: The goal is to get rid of the need for an extension point when contributing to an e4 application. As additional benefit it then supports dynamics (although not yet completely working, especially the addition and removal of for example menu contributions). But the example already shows that a part contributed by a fragment gets added/removed when the bundle is started/stopped. The model fragment registration consists of two parameters, an uri pointing to the fragment file and an apply property. This information can also be provided in a service via DS. Services can come and go and we can handle the dynamics in the reference injection. As such a service would only provide static information and no further functionality, from a plain OSGi perspective it would not be worth to be a service. Instead such static information can be provided via Manifest header. Actually I think this is what Christoph had in mind when he asked me to do it like this. Switching back from the Manifest header to the service approach that only provides static information should be easy to do.
(In reply to Mickael Istria from comment #5) > I've looked at the N&N. > > I'm a bit unsure about whether adding `Model-Fragment:` header in > MANFIEST.MF is a good idea. This header will be unknown to PDE, There is so much OSGi things "unknown" to PDE ;-) anyways, there is no reason to enhance PDE (or the application model tooling) to understand it. > undocumented, and really opens the door to MANIFEST.MF becoming > yet-another-entry-point for extensibility, and that conflicts with my "less > is more" feeling about making a Platform scale. Better explicit than implicit (like "search for a file named plugin.xml" what adds some overhead if not used). (In reply to Mickael Istria from comment #10) > So the question is "Is the addition of the MANIFEST.MF header bringing value > *now* that's hard to get otherwise, and that is worth opening a new area of > extensibility? Does it solve some current or very near future problems?". If > there is enough agreement on "yes", we keep it and cascade the decision to > maintenance, documentation, to PDE and so on (eg "pay the cost"); if there > is more substantial weight on "no", then we remove it. > > From my (limited) POV, I see no need for a MANIFEST header, but I'm not > building any other RCP app, so I may be missing an important and pragmatic > reason that makes extensibility via MANIFEST.MF worth it. MANIFEST.MF is the way to go if we'd like to move ahead in my opinion. Its just the way it works in OSGi and it was crafted that way to scale and be performant even on low resource systems. Everything that could be deduced from the manifest comes with no cost as the manifest is the first and only entity a framework parses and that is stable across the whole life-time of a bundle. That's also the reason why DS declares its XMLs in the manifest even if the classes are annotated, as otherwise classpathscanning would be necessary for all classes in the framework to discover components.
(In reply to Christoph Laeubrich from comment #12) > Better explicit than implicit (like "search for a file named plugin.xml" > what adds some overhead if not used). To be clear and ensure you don't build too much strategy on top of irrealistic expectations: the plugin.xml will *always* remain part of the Platform. There is 20 years of legacy extensions relying on that file, millions of lines of plugin.xml is thousands of distinct projects, the related code is bullet proof, it has tons of documentation, good tools and this file is known of every Eclipse plugin developer as soon as they read their first tutorial or start their first Eclipse project template. We cannot and mustn't get rid of the "overhead" the comes with it, this is not something you should expect will change one day. Similarly, specific extension points cannot be removed just like that: they are to be treated as API and removal have to go through agreement of the PMC and so on. > MANIFEST.MF is the way to go if we'd like to move ahead in my opinion. Its > just the way it works in OSGi and it was crafted that way to scale and be > performant even on low resource systems. Everything that could be deduced > from the manifest comes with no cost as the manifest is the first and only > entity a framework parses and that is stable across the whole life-time of a > bundle. That triggers an echo of my comment on the mailing-list: Platform has its way of doing things (plugin.xml) which is there to stay. It's not a bad thing to challenge that, but from Platform POV, good extensibility via plugin.xml is and remains more important than doing pure OSGi. Good OSGi support (eg extensions in MANIFEST.MF) is nice-to-have, but only worth it if it really enables added-value over existing extensibility strategy like plugin.xml.
(In reply to Mickael Istria from comment #13) > (In reply to Christoph Laeubrich from comment #12) > > Better explicit than implicit (like "search for a file named plugin.xml" > > what adds some overhead if not used). > > To be clear and ensure you don't build too much strategy on top of > irrealistic expectations: the plugin.xml will *always* remain part of the > Platform. I just take this as an example how design choices can impact performance on the long run. So if creating new ways its always good to think if e.g. reading of additional resources is absolutely necessary (e.g. plugin.xml must be queried as far as I understand regardless of presences, while for example DS requires to mention them in a header so there is no need for resource scanning if the bundle actually do not offer DS stuff. But there are APIs in OSGi that also just define specific locations so there are pro/cons for both). > That triggers an echo of my comment on the mailing-list: Platform has its > way of doing things (plugin.xml) which is there to stay. It's not a bad > thing to challenge that, but from Platform POV, good extensibility via > plugin.xml is and remains more important than doing pure OSGi. Actually I never proposed moving away from plugin.xml that was brought by Dirk in his initial request (any maybe by other demands), I just can give advice/recommendations for "if such a feature has to be implemented then ...) for whatever the reason was for it. I also never really understand (and have tried to explain why) the hope that simply removing activators (even though they are sometimes unnecessary and 'old school') will speed up the start-up dramatically but also there I can only give advice how an alternative might be better shaped and can't judge if that should be the way to go.
(In reply to Christoph Laeubrich from comment #14) > I also never really understand (and have tried to explain why) the hope that > simply removing activators (even though they are sometimes unnecessary and > 'old school') will speed up the start-up dramatically but also there I can > only give advice how an alternative might be better shaped and can't judge > if that should be the way to go. Yet the results are visible and startup is getting faster and faster. But it's not purely because of removal of activators, it's more that many Activator are/were doing a lot of work and classloading very/too early while this could have been delayed with lazier loading. Trying to get rid of activator is just one approach to "more lazily and delayed laoding".
(In reply to Mickael Istria from comment #13) > To be clear and ensure you don't build too much strategy on top of > irrealistic expectations: the plugin.xml will *always* remain part of the > Platform. There is 20 years of legacy extensions relying on that file, > millions of lines of plugin.xml is thousands of distinct projects, the > related code is bullet proof, it has tons of documentation, good tools and > this file is known of every Eclipse plugin developer as soon as they read > their first tutorial or start their first Eclipse project template. > We cannot and mustn't get rid of the "overhead" the comes with it, this is > not something you should expect will change one day. > > Similarly, specific extension points cannot be removed just like that: they > are to be treated as API and removal have to go through agreement of the PMC > and so on. To be more precise about this ticket. It is clear that the extension point mechanism and the extension registry will stay. That is sure. But we can reduce the number of extension points that are needed when implementing a plain e4 application or contributions. And the patch adds a way that developers can choose. Either they use the existing extension point for registering a fragment/processor, or they use the Manifest header / DS. With the later we also provide support for dynamics, which is not supported by extension points. For me the story is cleaner if I don't have to specify a plugin.xml with a single extension point in my bundle for the model contributions. In a lot of places we did this in E4 already, so IMHO we should also support this in other places in that area. Other people might not even care about that topic, but for me it feels inconsistent to get rid of extension points to contribute parts, menus, commands etc, but then have to specify an extension point to register the fragment. This is the goal of this ticket. Nothing more, nothing less. And the dynamic support is really a nice addition that several people requested in the past years.
Short comments on dynamics. First, there are still many bugs related to directly manipulating the E4 model. For a overview of 'just' merging fragments see Bug 562497. Removing fragments will trigger another range of bugs, if it is possible at all. Dynamics in the E4 model are still tricky, especially when the compatibility layer of E3 is involved, for instance removing of menus is not supported, Bug 374568 and many more. Furthermore, Eclipse 3 used to support dynamic activation/deactivation of plugins including the extension points. However, this behavior is still not implemented on Eclipse 4, see Bug 405296 and Bug 366635.
(In reply to Rolf Theunissen from comment #17) > Furthermore, Eclipse 3 used to support dynamic activation/deactivation of > plugins including the extension points. However, this behavior is still not > implemented on Eclipse 4, see Bug 405296 and Bug 366635. That's not exactly true. Most consumers of extension points in Platform are placing listeners on the registry to properly react to plugin activation/deactivation/addition/removal, but some others don't (yet?). So while the API allows for dynamism with extension, it's not something safe to rely on because it's not always available.
(In reply to Mickael Istria from comment #18) > (In reply to Rolf Theunissen from comment #17) > > Furthermore, Eclipse 3 used to support dynamic activation/deactivation of > > plugins including the extension points. However, this behavior is still not > > implemented on Eclipse 4, see Bug 405296 and Bug 366635. > > That's not exactly true. Most consumers of extension points in Platform are > placing listeners on the registry to properly react to plugin > activation/deactivation/addition/removal, but some others don't (yet?). So > while the API allows for dynamism with extension, it's not something safe to > rely on because it's not always available. At least this behavior is broken w.r.t. to Platform UI, i.e. org.eclipse.ui.tests.dynamicplugins.DynamicPluginsTestSuite is disabled due to missing implementations in E4.
Please take a look at bug 572970, it looks like a regression from this one. With bug 572839 it is a second regression, and looking on the discussion here and on bug 572839 I think we probably want to revert original patch until we have a solid, acceptable & tested solution. We can't simply continue to merge changes and disregard side effects. NB: this bug was not assigned to anyone - so I've assigned it to Dirk, as the merged code contributor. Dirk, please drive resolution of regressions or revert the original change.
(In reply to Andrey Loskutov from comment #20) > Please take a look at bug 572970, it looks like a regression from this one. Actually this is an annoying log MESSAGE (its not a warning nor error nor anything harmful) where Felix simply tells that a dynamic reference was delayed due a cyclic dependency chain (what is perfectly valid). I don't know who is to blame here that this message actually appears in the log without enabling any tracing/debug facilities.
(In reply to Christoph Laeubrich from comment #21) > (In reply to Andrey Loskutov from comment #20) > > Please take a look at bug 572970, it looks like a regression from this one. > > Actually this is an annoying log MESSAGE (its not a warning nor error nor > anything harmful) where Felix simply tells that a dynamic reference was > delayed due a cyclic dependency chain (what is perfectly valid). > > I don't know who is to blame here that this message actually appears in the > log without enabling any tracing/debug facilities. It's not a message, it's an error, it has severity IStatus.ERROR == 4. Even if it was, and something was just delayed, it causes severe side effects, which can be seen by all the following error log entries.
(In reply to Andrey Loskutov from comment #20) > With bug 572839 If I understand correctly from Alexander's investigation this is actually because of outdated dependencies, something I would merely blame to this change. I can understand that this causes some frustrations but honestly delaying this for some "big bang" would simply make this never happens. If I remember right Alexander was also the one mention it in a recent mailing-list comment: The world outside eclipse is evolving faster and faster and if (for whatever reason) we can't keep up with this because of fear-for-change on everything eclipse will more an more loose interest. As Rolf pointed out the "dynamic" story is nothing new, sadly until now it was delayed over and over again I just see that at least this change has given it some new spin :-)
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/179511
(In reply to Sebastian Ratz from comment #22) > It's not a message, it's an error, it has severity IStatus.ERROR == 4. The please point out the functional limitation/broken feature here that would help in investigate this. e.g. "if I open eclipse and do this or that then..." the first error I can see in the log for example is Unresolved requirement: Require-Capability: eclipse.platform; filter:="(osgi.os=win32)"
(In reply to Eclipse Genie from comment #24) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/179511 It seems Dirk is already working on it, thanks. Any functional description would help anyways to verify this I think. Its always a bit hard to guess from logfiles.
It is only a wild guess what the issue could be. The only relation between the IContentTypeManager and the ModelAssembler now is that both reference the IExtensionRegistry. So my assumption is that because the ModelAssembler is an immediate component, it triggers the creation of the IExtensionRegistry too early. So I assume that we have some strange timing issues. At least in the platform code I do not find a place where the ModelAssembler is referenced despite the ResourceHandler that is used in E4Application. I really wonder where a circular reference is coming from, as th IExtensionRegistry should not have a reference to the others.
(In reply to Christoph Laeubrich from comment #23) > (In reply to Andrey Loskutov from comment #20) > > With bug 572839 > > If I understand correctly from Alexander's investigation this is actually > because of outdated dependencies, something I would merely blame to this > change. We build & ship SDK every 3 months. If every change would add just a single test failure, no one will be able to trace the SDK state pretty soon. However, we expect that the SDK has some maintainable state that is at least *not worse* compared to the previous release. If you look at current SDK tests, you will see that this is not the case. The recurring pattern I see is that contributors are happy to solve their own small issues in some specific code part but do not take any action or see no interest to check/solve any test regressions introduced by this change. If you think that there is someone "special" to keep the tests green after your changes - it is just plain wrong. Everyone touching code in SDK is responsible that his change does not cause any side effects / no test failures. If the change shows test or functional or any other kind of regression, I expect that contributor responsible to the change either fixes that regression or asks for help or, finally, if no help will be given, reverts the change. We can't simply continue with growing test fails numbers, at some point in time the whole thing explodes. > I can understand that this causes some frustrations but honestly delaying > this for some "big bang" would simply make this never happens. It is not "some frustration", it is about to manage clean SDK build & test state, which is not not clean anymore. "Big bang" is happened already - we have 17 failing tests. > If I remember right Alexander was also the one mention it in a recent > mailing-list comment: > The world outside eclipse is evolving faster and faster and if (for whatever > reason) we can't keep up with this because of fear-for-change on everything > eclipse will more an more loose interest. You shouldn't mix "fear-for-change" with "breaking the build". Creating more test failures is stealing time from other contributors trying to keep the SDK relevant.
(In reply to Andrey Loskutov from comment #28) > The recurring pattern I see is that contributors are happy to solve their > own small issues in some specific code part but do not take any action or > see no interest to check/solve any test regressions introduced by this > change. If you think that there is someone "special" to keep the tests green > after your changes - it is just plain wrong. I don't think someone is fixing test but what I can tell from a developer/contributors perspective is that currently there is no easy way to verify this for me in advance: 1) I'm happy if I at least can start up an SDK local. Every few day "something" changes that keeps me with a broken workspace and I have to update my SDK, restart Setup actions, pull from 15+repositories, clean and rebuild and so on to get at least a state where I can start coding and testing simple cases. 2) same for tests, some fail on my machine while they suceed on the gerrit, some are even not executable for me 3) if I finally got to a working gerrit build that even is no guarantee that nothing was "broken", as some things (javadoc, relation to other repos) are not checked by the gerrit verification 4) if something is broken, I even don't get a mail but has to know where to check, then there are not the build alone there are some external checker tools and so on. All this makes the overall contribution process fragile and is not to blame on the individual contribution. If I can't trust the verification build how should I know if my change would break something? Why do I get 50+ gerrit mails a day but none that the overall build was broken or a test has failed?
(In reply to Dirk Fauth from comment #27) > It is only a wild guess what the issue could be. The only relation between > the IContentTypeManager and the ModelAssembler now is that both reference > the IExtensionRegistry. So my assumption is that because the ModelAssembler > is an immediate component, it triggers the creation of the > IExtensionRegistry too early. > > So I assume that we have some strange timing issues. At least in the > platform code I do not find a place where the ModelAssembler is referenced > despite the ResourceHandler that is used in E4Application. I really wonder > where a circular reference is coming from, as th IExtensionRegistry should > not have a reference to the others. It is not really a "circular reference", that name is extremely misleading, IMHO. It is actually more like a "reentrant". I attached a remote debugger and tracked it down to the following: - Bundle org.eclipse.core.contenttype starts - loadComponents -> org.eclipse.core.runtime.IExtensionRegistry - Bundle org.eclipse.core.runtime.IExtensionRegistrystarts starts - ComponentManager -> org.eclipse.e4.ui.internal.workbench.ModelAssembler Now, org.eclipse.e4.ui.internal.workbench.ModelAssembler is loaded and setExtensionRegistry() is about to be called: <snip> at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:171) <- "org.eclipse.emf.ecore.EStructuralFeature" !? at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.getDeclaredMethods0(Native Method) at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3166) at java.base/java.lang.Class.getDeclaredMethod(Class.java:2473) at org.apache.felix.scr.impl.inject.methods.BaseMethod.getMethod(BaseMethod.java:345) <- "org.eclipse.e4.ui.internal.workbench.ModelAssembler" / "setExtensionRegistry" </snip> - emf bundles are loaded - emf wants org.eclipse.core.runtime.IExtensionRegistry, too BUT, org.eclipse.core.runtime.IExtensionRegistry is currently being loaded further down in the stack -> "cicular" reference error. Now, the main question is: Why does Class<org.eclipse.e4.ui.internal.workbench.ModelAssembler>.getDeclaredMethod("setExtensionRegistry") suddenly need org.eclipse.emf.ecore.EStructuralFeature?
(In reply to Christoph Laeubrich from comment #29) > (In reply to Andrey Loskutov from comment #28) > > The recurring pattern I see is that contributors are happy to solve their > > own small issues in some specific code part but do not take any action or > > see no interest to check/solve any test regressions introduced by this > > change. If you think that there is someone "special" to keep the tests green > > after your changes - it is just plain wrong. > > I don't think someone is fixing test but what I can tell from a > developer/contributors perspective is that currently there is no easy way to > verify this for me in advance: I've not asked to do this in advance. The tests are failing *since a week* now. > 1) I'm happy if I at least can start up an SDK local. Every few day > "something" changes that keeps me with a broken workspace and I have to > update my SDK, restart Setup actions, pull from 15+repositories, clean and > rebuild and so on to get at least a state where I can start coding and > testing simple cases. Yep, the life is hard. Feel free to improve the process. Note: I use same workspace since few years and update my SDK daily - with the only effective work load of ~5 minutes (plus 30 minutes to download new build, but that is in the background). I have most SDK projects inside my workspace. > 2) same for tests, some fail on my machine while they suceed on the gerrit, > some are even not executable for me Yep, life is hard. Feel free to improve the process. > 3) if I finally got to a working gerrit build that even is no guarantee that > nothing was "broken", as some things (javadoc, relation to other repos) are > not checked by the gerrit verification Yep, life is hard. Feel free to improve the process. > 4) if something is broken, I even don't get a mail but has to know where to > check, then there are not the build alone there are some external checker > tools and so on. Yep, life is hard. Feel free to improve the process. > All this makes the overall contribution process fragile and is not to blame > on the individual contribution. So "feel free to break anything, since the process is so bad, unreliable and broken"? Should be this an excuse to *not* fix a *known* regression? I don't get that point. > If I can't trust the verification build how > should I know if my change would break something? Why do I get 50+ gerrit > mails a day but none that the overall build was broken or a test has failed? Not sure what is this about. Bug 572839 describes clearly the problem, it is reproducible and has nothing to do with gerrit notifications. You can't expect we run *every* SDK test on a UI module change? That would make gerrit busy for days... In general: one can complain about the process or one can try to improve the process, so if you have concrete issues with the process, please create bugs for them / try to improve that. Independently on the process discussion, I still expect that every *known* regression coming from a new change must addressed by the author / committer merged that change, latest after they got a note about this regression. It is not a "free beer" here, no one except contributors itself can/will fix those regressions.
(In reply to Andrey Loskutov from comment #31) > Not sure what is this about. Bug 572839 describes clearly the problem, it is > reproducible and has nothing to do with gerrit notifications. And I'm not sure whats the complaint about, the bug is been working on, the problem has been identified and a solution has been proposed, so everything seems going like you describe (beside that life is still hard... except for you). > You can't expect we run *every* SDK test on a UI module change? No but why got Dirk not become aware of it (and instead you need to trigger him), why not a warning to platform-dev when the "global" build fails so everyone gets alerted and could check? > In general: one can complain about the process or one can try to improve the > process, so if you have concrete issues with the process, please create bugs > for them / try to improve that. I already done this e.g. Bug 570430 > Independently on the process discussion, I still expect that every *known* > regression coming from a new change must addressed by the author / committer > merged that change, latest after they got a note about this regression. And Independently from this particular case I just pointed out that it currently is extremely hard to get aware of those for people not having the luxury to can work every day fulltime on eclipse-platform development :-) For me everything is working as you expect, Dirk has started working on issues as soon as he got aware of them (unless you like to state that Dirk intentionally has ignored this even though he was aware of it).
(In reply to Sebastian Ratz from comment #30) > It is not really a "circular reference", that name is extremely misleading, > IMHO. > It is actually more like a "reentrant". > > I attached a remote debugger and tracked it down to the following: Many thanks Sebastian for debugging this, is there any chance you can fetch Dirks gerrit here and verify if that relaxes the situation?
(In reply to Christoph Laeubrich from comment #32) > (In reply to Andrey Loskutov from comment #31) > > Not sure what is this about. Bug 572839 describes clearly the problem, it is > > reproducible and has nothing to do with gerrit notifications. > > And I'm not sure whats the complaint about, the bug is been working on, the > problem has been identified and a solution has been proposed, so everything > seems going like you describe (beside that life is still hard... except for > you). Please read carefully comment 20. It is not a complaint, it is just reminder, a gentle call for action. Dani would just revert the patch without long discussions. > > You can't expect we run *every* SDK test on a UI module change? > > No but why got Dirk not become aware of it (and instead you need to trigger > him), why not a warning to platform-dev when the "global" build fails so > everyone gets alerted and could check? Please subscribe to https://www.eclipse.org/mailman/listinfo/platform-releng-dev . You will get build mails for every build with the link to the test results (something like https://download.eclipse.org/eclipse/downloads/drops4/I20210418-1800/testResults.php). We had also test results *in* the mail some time ago, but that get lost after transition from one to another build system. It was lost also because we had no one who could spent time to fix it, because of other duties. > > Independently on the process discussion, I still expect that every *known* > > regression coming from a new change must addressed by the author / committer > > merged that change, latest after they got a note about this regression. > > And Independently from this particular case I just pointed out that it > currently is extremely hard to get aware of those for people not having the > luxury to can work every day fulltime on eclipse-platform development :-) I'm not working full time on the platform, only part time. And is it *really* "extremely hard" next day after submitting the patch to look at the page https://download.eclipse.org/eclipse/downloads/ and check the most recent build state?
I am actually responding on error reports. But yes I am not able to check every build issue as I currently don't have time to check everything all the time. Therefore many thanks to the people that are reporting the issues to make people aware of them. For the cycle issue I assume this is because the IExtensionRegistry is now referenced earlier in the process. My patch should delay this. If not maybe we can use the lookup strategy for accessing the IExtensionRegistry once needed. But some verification would be great so I know what to do next. For the issues with Java 16 I have no idea. It seems to be an issue with Java 16 and the Mockito / ByteBuddy library versions we use. I am really sorry that I can't help here, but I simply don't know what to do. The tests fail because the mocking is not working with Java 16. Any ideas?
(In reply to Andrey Loskutov from comment #34) > Please subscribe to > https://www.eclipse.org/mailman/listinfo/platform-releng-dev Done, thanks for the pointer! > I'm not working full time on the platform, only part time. and some do only on their free time... > And is it *really* "extremely hard" next day after submitting the patch > to look at the page https://download.eclipse.org/eclipse/downloads/ and > check the most recent build state? I don't say it is hard, I just say it *might* help to make the developer more aware of it...
(In reply to Dirk Fauth from comment #35) > For the issues with Java 16 I have no idea. It seems to be an issue with > Java 16 and the Mockito / ByteBuddy library versions we use. I am really > sorry that I can't help here, but I simply don't know what to do. The tests > fail because the mocking is not working with Java 16. Any ideas? See bug 572839 comment 11. If I read it right, we need new byte-buddy, objenesis and mockito-core bundles in Orbit, so that platform tests could consume them. If that wouldn't be possible for M3 time frame, one possibility would be to skip the tests on Java 16 or try to workaround that failure in some way (no idea how).
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/179511 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e39a1c096178d94c522244eaf4f47a17ee1df0c8
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/179582
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/179582 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e012a2a9e87c1a09be18ec7fb69fda109004f817
Gerrit change https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/179290 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=cd60c7293c5e6d721c0007e4b8ac4289139e8906
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/180832
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.ui/+/180832 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=61d7571217e47f583bc5b3e1aeaa775322cbd7e6
Can this bug be resolved now? If not, what is missing?
(In reply to Andrey Loskutov from comment #44) > Can this bug be resolved now? If not, what is missing? Documentation in Eclipse Help.
(In reply to Mickael Istria - away from work until June from comment #45) > (In reply to Andrey Loskutov from comment #44) > > Can this bug be resolved now? If not, what is missing? > > Documentation in Eclipse Help. @Dirk: could you manage that before RC2 this Wendnesday? If not, please close this bug and create new one to update the help, targeting 4.21.
(In reply to Andrey Loskutov from comment #46) > (In reply to Mickael Istria - away from work until June from comment #45) > > (In reply to Andrey Loskutov from comment #44) > > > Can this bug be resolved now? If not, what is missing? > > > > Documentation in Eclipse Help. > > @Dirk: could you manage that before RC2 this Wendnesday? If not, please > close this bug and create new one to update the help, targeting 4.21. Sorry, but I never contributed to the help system, and to be honest I rarely use it because I never find the information I am looking for. Same here, I do not find any information about the application model in the Eclipse help. So I don't know where I should add the information about the manifest header for the model fragment or the service for the model processor contribution. There are too many obstacles for me to contribute that information to the Eclipse help. So no, I am not able to contribute until Wednesday. And I doubt that I will ever be in a position to contribute that, as I do not even find information about the application model in the Eclipse help.
(In reply to Dirk Fauth from comment #47) > Sorry, but I never contributed to the help system There is a 1st time to everything. > Same here, I > do not find any information about the application model in the Eclipse help. It's not much, but there is https://help.eclipse.org/2021-03/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Freference%2Fextension-points%2Forg_eclipse_e4_workbench_model.html . > There are too many obstacles for me to contribute that information to the > Eclipse help. So no, I am not able to contribute until Wednesday. And I > doubt that I will ever be in a position to contribute that, as I do not even > find information about the application model in the Eclipse help. I first mentioned request for documentation more than 6 weeks ago, there was time to ask questions. Although there are some glitches (eg missing doc about application model), Platform is not a sandbox, there are some rules that needs to be followed to guarantee some sustainability and writing documentation together with major design decisions is one of them. To be clear, if I had known in advance that the request to write documentation would be withdrawn by lack of interest, I'd have interpreted that as a lack of seriousness and would have reverted the patch much earlier. Adding new extensibility approaches without committing to full integration into Eclipse Platform (eg documentation) is hurtful for the project on the long run. a Platform is more that just a collection of undocumented and incomplete mechanisms. If you're not willing to contribute documentation in the future, please refrain from implementing anything that affects the architectural design of the Platform.
(In reply to Mickael Istria - away from work until June from comment #48) > (In reply to Dirk Fauth from comment #47) > > Sorry, but I never contributed to the help system > > There is a 1st time to everything. > > > Same here, I > > do not find any information about the application model in the Eclipse help. > > It's not much, but there is > https://help.eclipse.org/2021-03/index.jsp?topic=%2Forg.eclipse.platform.doc. > isv%2Freference%2Fextension-points%2Forg_eclipse_e4_workbench_model.html . > > > There are too many obstacles for me to contribute that information to the > > Eclipse help. So no, I am not able to contribute until Wednesday. And I > > doubt that I will ever be in a position to contribute that, as I do not even > > find information about the application model in the Eclipse help. > > I first mentioned request for documentation more than 6 weeks ago, there was > time to ask questions. Although there are some glitches (eg missing doc > about application model), Platform is not a sandbox, there are some rules > that needs to be followed to guarantee some sustainability and writing > documentation together with major design decisions is one of them. > To be clear, if I had known in advance that the request to write > documentation would be withdrawn by lack of interest, I'd have interpreted > that as a lack of seriousness and would have reverted the patch much earlier. > Adding new extensibility approaches without committing to full integration > into Eclipse Platform (eg documentation) is hurtful for the project on the > long run. a Platform is more that just a collection of undocumented and > incomplete mechanisms. If you're not willing to contribute documentation in > the future, please refrain from implementing anything that affects the > architectural design of the Platform. The page you linked is about the extension points. Nobody would ever find the information there. And despite of that, there is not a single place in the whole Eclipse help system about the application model. At least I haven't found it. Your request for adding the documentation on that specific feature is again a request for much more. And I don't have the time to write a good documentation about the application model and its extensiability. Actually there is only some small wiki page and some books that cover that information. So what should I do now? Document the whole stuff from scratch for that small new feature? I feel very offended by your statement that I am not willing to write documentation! I have written the N&N to document what I have done and how it can be used. Copying that information to any other place is easy, finding the place to add it additionally is not that easy. Tell me where I should add the information for the documentation and I will add it to satisfy you. IMO there is no place in the help system where it would fit. I implemented the new feature, I wrote the new and noteworthy, I fixed all the bugs and Christoph added the tooling support. And now you are telling me I should stop contributing to the Platform because I don't want to start a whole new documentation in the help system about the application model from scratch that actually does not exist for years? If this is your way of managing the Platform project, don't expect much from my side in the future!
For the manifest header we could add the information here: https://help.eclipse.org/2021-03/topic/org.eclipse.platform.doc.isv/reference/misc/bundle_manifest.html Not sure about the OSGi service for the processor. Where is that page you mentioned about OSGi services? And is there any documentation available that explains where to find the sources of the platform help? I haven't found it anywhere.
I'm sorry I was offensive. My main goal was to emphasize that documentation is a requirement, not to address this as a personal criticism. The code of the help content is https://git.eclipse.org/c/platform/eclipse.platform.common.git/tree/bundles/org.eclipse.platform.doc.isv (The MANIFEST.MF of the org.eclipse.platform.doc.isv bundle, like all other platforms bundle, has an "Eclipse-SourceReferences" header with reference to the Git repository and commit where source could be found, and the plug-ins view has an "Import from Repository" contextual action to allow importing a bundle from the Git repo). Adding this header to the list of MANIFEST header would be good. OSGi services placeholders are listed in reference/services. No need to add more things about the application model as part of that change. Although it would be important to add some doc about it at some point, it's not the topic of that change. That change should simply focus on the addition of the MANIFEST header "extension point" and use the same grain as what's in the org.eclipse.e4.workbench.model without trying to fill the current holes in the documentation. Filling the holes can be a topic of another issue; but isn't a requirement here.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.common/+/181211
@Mickael Probably it was too late for that discussion yesterday and we had some misunderstandings. I apologize for my harsh reaction. I have added a rudimentary documentation in the MANIFEST headers and the services. Hope that is fine for the moment so the new additions are documented.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.common/+/181211 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.common.git/commit/?id=ea85aee3ff5ae9401a3ba43bcf74469a2e20c706
Doc got merged, thanks. I think we're done here; let's mark as resolved.
@Dirk I have used this feature in one of my applications right now and it worked like a charm! This makes it finally possible to interface OSGi with E4 to get the Workbenchcontext in a declarative way.
