Community
Participate
Working Groups
Currently, the org.eclipse.pde.ui bundle contains all of PDE's launching code. This mingling with UI code prevents using the launcher in a headless environment. In particular, this blocks a decent JUnit integration with Buckminster. See this discussion in the buckminster-dev newsgroup: http://dev.eclipse.org/mhonarc/lists/buckminster-dev/msg00759.html
Bug #278844 is the equivalent enhancement request for JDT's JUnit integration. That must be fixed first before it makes sense to do anything in PDE.
Hi PDE team. I've read your plans for the 3.6 release and I'd like to raise awareness for this issue here. The fixes for Bug #278844 are basically finished and wait for approval and application to CVS. Sebastian (Zarnekow, from the Xtext team) and I are now planning to start working on this issue soon. We'd appreciate if you guys could reserve some time for reviewing our changes within the next couple of weeks. The patch will be huge because we'll have to move a number of classes. It would be best if we could do this as early in the 3.6 development cycle as possible. Any objections from your side?
(In reply to comment #2) > Any objections from your side? No, we love patches. Let us know when you're ready and we'll work with you.
Adding Guillaume to CC.
Achim, are you ready to work on this given that the JDT changes will be going in soon?
(In reply to comment #5) > Achim, are you ready to work on this given that the JDT changes will be going > in soon? > Hi Chris, I've been working together with Achim on this one and we already implemented a working solution. We still want to clean up some things but I think the first patch will be available in the next days. Regards, Sebastian
Please note that the patch can't break API, just be cautious of that. Thanks for working on this.
Here's some more info about where we are right now: We've been working collaboratively on a temporary Google code repository. If you want to take a first look, checkout instructions are here: http://code.google.com/p/pde-junit/source/checkout Things are looking really good. I was able to include the new pde.launching bundle in headless Buckminster and execute headless JUnit 4 plug-in tests. We still have some clean-up work to do, like throwing out some constants and NLS messages that we've copied over from the UI bundle. Oh, and yes, we accidentally moved a couple of API classes into other (non-UI) packages. We'll restore them soon. Other than that, the only annoying thing is that the PDESourceLookupDirector has a "soft" dependency on WorkingSetSourceContainer, which comes from the debug.ui bundle. Luckily, that's only a reference to the the container's TYPE_ID constant. I replaced that with a String literal, and things work fine now. Maybe you can come up with a better solution.
Let us know when you have something concrete to review. Thanks.
Created attachment 145815 [details] Initial Patch Sebastian and I have now finalized our work on the refactoring of the PDE launching facilities. The attached patch contains the changes to the org.eclipse.pde.ui as well as the new org.eclipse.pde.launching bundle. Here's what we did: 1) Moved the relevant classes from org.eclipse.pde.internal.ui and ...ui.launcher into org.eclipse.pde.internal.launching and ...launching.launcher, respectively. 2) Moved some classes from o.e.pde.ui.launcher to o.e.pde.launching.launcher. The UI bundle still contains the classes, but they are marked deprecated and now merely extend their o.e.pde.launching.launcher counterparts. o.e.pde.ui re-exports o.e.pde.launching. Thus, the API is fully compatible. 3) We had to make some minor changes for the separation of launching code and the UI code. This involved adding new API. Sebastian will elaborate on this, since this was his work. 4) The osgiFrameworks extension point was also moved into the o.e.pde.launching bundle. The extension point ID, however, is still o.e.pde.ui.osgiFrameworks so we have no API break here, too. 5) We split up and cleaned up declarations of constants and the NLS messages. 6) Two preference settings (PROP_AUTO_MANAGE and DEFAULT_OSGI_FRAMEOWRK) are used in the launching part. These settings are now consequently stored in the o.e.pde.launching InstanceScope. A PreferenceInitializer migrates these settings from the o.e.pde.ui scope to the new scope. Also, the PDE preference UI now stores these two values in the new scope. NOTICE: You will have to apply Patch 6 from bug #278844 before applying this patch in order to have a compilable result. This one obviously depends on the new o.e.jdt.junit.core stuff. When you review the code, please don't forget the issue with the PDESourceLookupDirector that I mentioned in comment #8. We're looking forward to your comments :-)
I will review for M2. This patch is large enough that we need to go through the IP process.
(In reply to comment #10) > 3) We had to make some minor changes for the separation of launching code and > the UI code. This involved adding new API. Sebastian will elaborate on this, > since this was his work. I've identified the various places that used UI components to interact with the user even if the other aspects of the class did not have something to do with the UI. I refactored those bits and peaces and introduced a strategy interface with a bunch of loosly related methods (handleThisErrorCondition, handleThatErrorCondition...). There are two implementations for this interface, one with UI dependencies to interact with the user, display dialogs or whatever (placed in the ui plugin), another with a reasonable default behaviour (in the rt plugin). An instance that fullfils this interface can be obtained from the rt plugin activator which is configured with a factory for this purpose. The ui plugin activator assigns another factory to be used. This solution should not break backwards compatibility as no existing public API had to be changed.
Created attachment 146314 [details] pde-launching.psf Here is a PSF file. I created org.eclipse.pde.launching bundle in the PDE incubator so we can work off of patches now. Please provide patches using this bundle now.
Now that bug 278844 has been fixed, we can start working on this. Here's a couple things that need to be addressed: 1) everything in the org.eclipse.pde.ui.launcher package needs to stay in it. I started some of this work but it's not done yet. We have to do this so we don't break API. 2) For error conditions, we should use the same pattern that Debug uses. That is, I'm not happy with the way HeadlessPDELauncherSettings works at the moment. We can slowly start working on these items.
> 1) everything in the org.eclipse.pde.ui.launcher package needs to stay in it. I > started some of this work but it's not done yet. We have to do this so we don't > break API. > The API tools did not complain about broken API with the solution from the initial patch. All classes from the org.eclipse.pde.ui.launcher package are still present, albeit deprecated, in the org.eclipse.pde.ui bundle. Why does the org.eclipse.pde.launching bundle have to expose the org.eclipse.pde.ui package? Sure, clients won't see so many deprecation warnings if we don't change the package name. But we don't break anything, and all a client needs to do is to change the import statements. > 2) For error conditions, we should use the same pattern that Debug uses. That > is, I'm not happy with the way HeadlessPDELauncherSettings works at the moment. > I'll take a look at this. > We can slowly start working on these items. Just to be sure: We're working on the vanilla org.eclipse.pde.launching project that you've checked into CVS, and the org.eclipse.pde.ui bundle modified with its share of the initial patch that I've attached, right?
(In reply to comment #15) > The API tools did not complain about broken API with the solution from the > initial patch. All classes from the org.eclipse.pde.ui.launcher package are > still present, albeit deprecated, in the org.eclipse.pde.ui bundle. Why does > the org.eclipse.pde.launching bundle have to expose the org.eclipse.pde.ui > package? Sure, clients won't see so many deprecation warnings if we don't > change the package name. But we don't break anything, and all a client needs to > do is to change the import statements. I don't like the deprecated approach, I think it's cleaner if we just move the package. No API is broken either way. > Just to be sure: We're working on the vanilla org.eclipse.pde.launching project > that you've checked into CVS, and the org.eclipse.pde.ui bundle modified with > its share of the initial patch that I've attached, right? Correct, we are working off that project and the org.eclipse.pde.ui bundle in HEAD at the moment. So whatever you post as a patch, can be cleanly applied and tested.
Great to see that this work is coming along.
Created attachment 146484 [details] Updated patch I've done this to get rid of the IPDELauncherSettings: * I removed the IPDELaunchConfigurationHelper interface. Instead of delegating to the helper to display a CoreException in AbstractPDELaunchConfiguration.launch(), I simply rethrow it. This results in a slightly different error dialog in the UI, but the net result is about the same. I'll attach screenshots in a minute. * I also removed the IPDELaunchListener interface. The LaunchListener itself is now in the org.eclipse.pde.launching bundle. It uses IStatusHandlers from the DebugPlugin to handle errors. The org.eclipse.pde.ui bundle registers handlers for these conditions. If no handlers are registered (i.e. in pure headless mode), the errors are simply logged. Is that what you had in mind? If so, I'll continue with this strategy for the other responsibilities of the IPDELauncherSettings. I'd appreciate if you committed the changes from this patch into the pde.launching incubator. This helps me isolate upcoming changes. Please note that I failed to come up with a situation that produces returnValue == 15, i.e. a failed launch because of a workspace in use. It seems that PDE catches this error _before_ the application is actually launched. I've also changed the bundle version to 3.6.0. Since the API classes have benn moved into the launching bundle, bug #265699 seems to require that we must not start with 1.0.0.
Created attachment 146485 [details] Screenshot of the old error dialog This is how the error dialog looks with PDE 3.5 if a CoreException occurs in AbstractPDELaunchConfiguration.launch().
Created attachment 146486 [details] Screenshot of new error dialog This is how the error dialog looks after the changes, i.e. by simply re-throwing the CoreException. I'd say this is even better than before, because the user can now jump to the error log directly.
(In reply to comment #18) > Created an attachment (id=146484) [details] > Updated patch Awesome, applied pde.launching segments. > Is that what you had in mind? If so, I'll continue with this strategy for the > other responsibilities of the IPDELauncherSettings. I'd appreciate if you > committed the changes from this patch into the pde.launching incubator. This > helps me isolate upcoming changes. This is exactly what I had in mind. The status handler approach in debug was made for this use case. > Please note that I failed to come up with a situation that produces returnValue > == 15, i.e. a failed launch because of a workspace in use. It seems that PDE > catches this error _before_ the application is actually launched. This shouldn't be that hard to reproduce. We catch this before the application is launched as it's quicker to catch the error that way (checking for a workspace lock using equinox APIs) > I've also changed the bundle version to 3.6.0. Since the API classes have benn > moved into the launching bundle, bug #265699 seems to require that we must not > start with 1.0.0. That's fine. Thanks for your hard work Achim!
Created attachment 146606 [details] Updated patch This updated version uses IStatusHandlers exclusively. No more IPDELauncherSettings and other helper interfaces. The only new API we now introduce is a new constant in IPDELaunchererConstants. My tests were all positive. The UI still behaves as expected. If you approve these changes, I'll move the remaining classes from o.e.pde.launching.launcher to o.e.pde.ui.launcher tomorrow.
We're looking better :) One problem I still see is this: IPDELauncherConstants.java JUnitLaunchConfigurationDelegate.java PDESourcePathProvider.java These classes should be in org.eclipse.pde.ui.launcher in the org.eclipse.pde.launching bundle in order to not break API. Introducing a new constant is fine.
I committed modifications to org.eclipse.pde.launching, please attach a new patch when ready. We are getting close to review time :)
Created attachment 146662 [details] (Hopefully) final patch I moved the remaining classes o.e.pde.ui.launcher. Both bundle versions are now 3.6.0 because or the new constant declaration. I guess we're ready for review now. :-) One final quirk maybe: the IPDELauncherConstants interface declares a number of IDs for the tabs in the UI. The o.e.pde.launching bundle is not the right place for them to be, but we can't remove them since they're API. You may, however, want to deprecate them and move them to another location.
We will do this in M3, M2 is next week and there's not enough time but the last patch is looking good. We will look at releasing this early in M3 after we get PMC approval for the new plug-in.
Created attachment 146687 [details] org.eclipse.pde.ui.patch An update patch.
Created attachment 147722 [details] org.eclipse.pde.patch The final patch.
Darin, can you give this one a review? This takes the approach of deprecating the old classes and extending the new ones.
Achim, can you also verify this latest patch meets your needs?
Created attachment 147726 [details] org.eclipse.pde.ui.patch I made the patch smaller by committing changes to org.eclipse.pde.launching already.
(In reply to comment #30) > Achim, can you also verify this latest patch meets your needs? I locally built Buckminster against the o.e.pde.launching bundle from the pde repo (not the pde-incubator) and tried some headless launching. Everything still worked perfectly. We're good to go from my side.
(In reply to comment #32) > I locally built Buckminster against the o.e.pde.launching bundle from the pde > repo (not the pde-incubator) and tried some headless launching. Everything > still worked perfectly. We're good to go from my side. Thanks Achim. I'm currently waiting for PMC approval in bug 290054 for the new plug-in. If you have any comments there, please let them be known.
There is a reference to a 1.5 method (easily fixed), in LauncherUtilsStatusHandler, by using "new Integer(...)" instead: * The method valueOf(String) in the type Integer is not applicable for the arguments (int) There are API compatbility problems reported by API tooling that are valid. Some class hierachies have been modified (reduced), such that "instanceof" checks would no longer work. For example: ui.launcher.EclipseApplicationLaunchConfiguration, no longer subclasses ui.launcher.Abstract.AbstractPDELaunchConfiguration.
(In reply to comment #34) > There is a reference to a 1.5 method (easily fixed), in > LauncherUtilsStatusHandler, by using "new Integer(...)" instead: > > * The method valueOf(String) in the type Integer is not applicable for the > arguments (int) Sure, I can fix that. > There are API compatbility problems reported by API tooling that are valid. > Some class hierachies have been modified (reduced), such that "instanceof" > checks would no longer work. For example: > > ui.launcher.EclipseApplicationLaunchConfiguration, no longer subclasses > ui.launcher.Abstract.AbstractPDELaunchConfiguration. What's the solution here? Are people really doing instanceof checks on these API classes?
(In reply to comment #35) > What's the solution here? Are people really doing instanceof checks on these > API classes? Clients should to migrate to the new classes, but to be compatible, we need leave the old *leaf* classes as is (was). I will attach a patch with a solution. This is unfortunate, but is the best I can come up with.
Created attachment 147800 [details] patch Fixes compatibility issues in the old/deprecated classes.
I'll +1 this, adding Chris for review now :-)
Created attachment 147804 [details] org.eclipse.pde.patch I just updated some javadoc in Darin's latest patch. +1 for inclusion
done. Thanks for your help Achim in getting this through. > 20090922
> > Thanks for your help Achim in getting this through. > You're very welcome. Thanks for accepting this not-so-trivial refactoring. I'd also like to thank Sebastian for helping me getting things started. We're now finally able to run JUnit tests in headless builds with Buckminster and thus ultimately with b3, too. Now I'll go and reward myself with a beer in the hotel lobby. :-)
