Bug 413450 - A message should be generated if PlatformAdmin is not available
Summary: A message should be generated if PlatformAdmin is not available
Status: VERIFIED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.3   Edit
Hardware: PC Mac OS X
: P3 normal (vote)
Target Milestone: 4.4 M1   Edit
Assignee: Curtis Windatt CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
Depends on:
Blocks:
 
Reported: 2013-07-22 09:50 EDT by Thomas Watson CLA
Modified: 2013-08-07 11:21 EDT (History)
3 users (show)

See Also:


Attachments
Opens a message dialog on PDE UI startup (4.41 KB, patch)
2013-07-29 14:07 EDT, Curtis Windatt CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Watson CLA 2013-07-22 09:50:12 EDT
I'm opening this against PDE, but it could be decided that this message belongs elsewhere (for example, as an error/warning from org.eclipse.core.runtime).

In Luna the Equinox framework no longer publishes the PlatformAdmin service.  This causes the method org.eclipse.core.runtime.Platform.getPlatformAdmin() to return null unless the compatibility fragment org.eclipse.osgi.compatibility.state is installed.  Without this fragment PDE currently gets NPEs.  I think PDE should be updated to avoid the NPEs and post a message to the user that PDE cannot function unless the org.eclipse.osgi.compatibility.state is installed.

Another option is for the method org.eclipse.core.runtime.Platform.getPlatformAdmin() to post a message to the log when PlatformAdmin is not available.  This would not inform the user right away about the issue with PDE not being able to function properly without the compatibility fragment.  Instead the user (likely as a last resort) would look in the log file and see the error message.
Comment 1 Curtis Windatt CLA 2013-07-22 11:58:02 EDT
AFAICT PDE is only using the PlatformAdmin for the plugin registry view.  Without it the view doesn't act at all properly (even if we avoid the NPEs).  Either the registry view should display the error (and possibly log it) or the pde runtime plug-in should have a dependency on the compatibility fragment.

I'm leaning towards just putting an error message in the registry view.  Adding the dependency only removes access to the registry view and the plug-in spy, but it still completely hides that functionality from the user.
Comment 2 Curtis Windatt CLA 2013-07-22 12:23:47 EDT
I implemented a basic error message and pushed it to gerrit:
https://git.eclipse.org/r/14751
Comment 3 Thomas Watson CLA 2013-07-22 15:40:36 EDT
Curtis, how does PDE create the State objects that are used to resolve bundles in the target and workspace?  I thought PDE was using PlatformAdmin.getFactory() to get a factory for creating State objects.
Comment 4 Curtis Windatt CLA 2013-07-22 16:16:09 EDT
(In reply to comment #3)
> Curtis, how does PDE create the State objects that are used to resolve
> bundles in the target and workspace?  I thought PDE was using
> PlatformAdmin.getFactory() to get a factory for creating State objects.

Looks like we do use PlatformAdmin for that using the Platform plug-in's method.  This just became a lot harder as I was ignoring the 17 or so places where PDE uses PlatformAdmin in some form through Platform.

cc'ing Mike as Ant uses PlatformAdmin to get the class loader bundle (AntCorePreferences).
Comment 5 Curtis Windatt CLA 2013-07-22 16:16:29 EDT
Mike, please see the previous comment.
Comment 6 Thomas Watson CLA 2013-07-22 17:01:52 EDT
(In reply to comment #4)
> cc'ing Mike as Ant uses PlatformAdmin to get the class loader bundle
> (AntCorePreferences).

Also see the ant bug I opened: bug 411773.
Comment 7 Curtis Windatt CLA 2013-07-23 16:09:19 EDT
Tom, is the compatibility fragment going to be part of the Platform feature?
Comment 8 Thomas Watson CLA 2013-07-23 16:21:49 EDT
org.eclipse.osgi.compatibility.state is part of the e4.rcp feature.

http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/features/org.eclipse.e4.rcp/feature.xml?id=e50e409cac876b6ba9e3158649b52cdd2e6880dc
Comment 9 Curtis Windatt CLA 2013-07-23 16:59:19 EDT
(In reply to comment #8)
> org.eclipse.osgi.compatibility.state is part of the e4.rcp feature.
> 
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/features/
> org.eclipse.e4.rcp/feature.xml?id=e50e409cac876b6ba9e3158649b52cdd2e6880dc

How would a user get into a situation where they have PDE installed, but not the fragment?  PDE Feature requires Platform which includes RCP which includes e4 RCP.
Comment 10 Thomas Watson CLA 2013-07-23 17:22:09 EDT
(In reply to comment #9)
> How would a user get into a situation where they have PDE installed, but not
> the fragment?  PDE Feature requires Platform which includes RCP which
> includes e4 RCP.

I've had a couple users that ran PDE on the new framework but did not include the compatibility.state fragment.  I think the only way this can happen is when launching an eclipse instance from eclipse (self-hosting) and changing the target such that the compatiblity.state fragment is not present.  One of these uses was Dani who always sets up a self-contained workspace where he imports all required plugins into the workspace to self-host the Eclipse SDK and gets nothing from a target.  In this setup he did not import the new compatibility.state.

It does seem like an edge case.  CC'ing Dani to see if he thinks this is important to do in Luna.
Comment 11 Dani Megert CLA 2013-07-24 03:44:20 EDT
(In reply to comment #1)
> AFAICT PDE is only using the PlatformAdmin for the plugin registry view. 

Not true. As soon as one creates a PDE project or converts a Java project to PDE, one runs into this.


(In reply to comment #9)
> How would a user get into a situation where they have PDE installed, but not
> the fragment?  PDE Feature requires Platform which includes RCP which
> includes e4 RCP.

Very easily. I have a workspace where I have all required bundles imported. With a new I-build I (re-)import all existing bundles again from the new build. After the last I-build I could no longer create PDE projects in my target due to the NPE. And I had no clue in which not yet imported bundle/fragment I could find the PlatformAdmin service.
Comment 12 Curtis Windatt CLA 2013-07-24 12:03:40 EDT
Will the Platform.getPlatformAdmin() javadoc be updated to state that it may return null?  Looking at PDE, I think that the second option of having Platform.getPlatformAdmin() log an error on each call is appropriate.  PDE will try to warn the user in the UI, but there are 15 references to that method in PDE UI/Core/Build, some of which may be accessed without obvious UI interaction.
Comment 13 Curtis Windatt CLA 2013-07-29 14:07:02 EDT
Created attachment 233902 [details]
Opens a message dialog on PDE UI startup

This patch pops up a message dialog during the startup method of PDE UI if PlatformAdmin is unavailable (from a UIJob).  We could also throw an exception, but this leads to multiple error dialogs being opened up at startup.

It would still be helpful if Platform can log a message if the service is requested but unavailable.
Comment 14 Dani Megert CLA 2013-07-30 08:01:01 EDT
(In reply to comment #13)
> Created attachment 233902 [details] [diff]
> Opens a message dialog on PDE UI startup
> 
> This patch pops up a message dialog during the startup method of PDE UI if
> PlatformAdmin is unavailable (from a UIJob).  We could also throw an
> exception, but this leads to multiple error dialogs being opened up at
> startup.
> 
> It would still be helpful if Platform can log a message if the service is
> requested but unavailable.

This is too intrusive in my opinion. With that patch I'm faced with a dialog already when starting a new workspace where I maybe never ever plan to do any PDE related thing.

I think a simple log entry is all we need here.

Trivial issue: the name of the fragment should be in single quotes.
Comment 15 Curtis Windatt CLA 2013-07-30 09:25:05 EDT
(In reply to comment #14)
> This is too intrusive in my opinion. With that patch I'm faced with a dialog
> already when starting a new workspace where I maybe never ever plan to do
> any PDE related thing.
> 
> I think a simple log entry is all we need here.

I would be fine with that, but there is the possibility of that log entry getting buried.  If the user starts activating PDE editors additional errors will be logged that won't be easily understood by the user.
Comment 16 Dani Megert CLA 2013-07-30 09:30:31 EDT
(In reply to comment #15)
> (In reply to comment #14)
> > This is too intrusive in my opinion. With that patch I'm faced with a dialog
> > already when starting a new workspace where I maybe never ever plan to do
> > any PDE related thing.
> > 
> > I think a simple log entry is all we need here.
> 
> I would be fine with that, but there is the possibility of that log entry
> getting buried.  If the user starts activating PDE editors additional errors
> will be logged that won't be easily understood by the user.

First, I think a "normal" user won't run into this. Second, if something is not working, then he will read the log. If not, too bad (for him).
Comment 17 Curtis Windatt CLA 2013-07-31 18:07:21 EDT
Fixed in master:
http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=0c954a833ad91a188c6f1899aaf5fe838d5ef7bb

The most common entry point for PDE to need PlatformAdmin is initializing our models using an OSGi state.  By putting the error logging code there we can avoid dumping a message to the log if the user isn't using PDE at all.  There will still be additional errors logged, depending on what the user is trying to do.

The Plug-in Registry view was the only place where I could encounter errors for a missing PlatformAdmin without initializing the PDE models and having the error message written.
Comment 18 Dani Megert CLA 2013-08-07 07:59:25 EDT
Verified in I20130806-2000.