Bug 172666 - Importing pde.ui and dependencies as binary gives compile error
Summary: Importing pde.ui and dependencies as binary gives compile error
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.3   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.3 M6   Edit
Assignee: Jerome Lanneluc CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-02-02 09:43 EST by Markus Keller CLA
Modified: 2007-03-20 02:59 EDT (History)
5 users (show)

See Also:


Attachments
One trace of a peculiar instance of the scenario (138.30 KB, text/plain)
2007-02-20 11:14 EST, Maxime Daniel CLA
no flags Details
DO NOT RELEASE - instrumentation patch (16.43 KB, patch)
2007-02-21 01:26 EST, Maxime Daniel CLA
no flags Details | Diff
Fix and regression tests (10.19 KB, patch)
2007-02-23 10:26 EST, Jerome Lanneluc CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2007-02-02 09:43:07 EST
N20070202-001

- new workspace
- import org.eclipse.pde.ui and all dependent plug-ins as binary

==> 3 compile errors:
1. The project cannot be built until its prerequisite org.apache.ant is built. Cleaning and building all projects is recommended		org.eclipse.ant.ui	

2. The project cannot be built until its prerequisite org.eclipse.ant.ui is built. Cleaning and building all projects is recommended		org.eclipse.pde.ui

3. The project cannot be built until its prerequisite org.eclipse.osgi is built. Cleaning and building all projects is recommended		
org.apache.ant

- do Project > Clean all
==> same 3 errors

- close project org.eclipse.osgi
==> 0 errors

- open project org.eclipse.osgi
==> 0 errors

The (secondary) problem that Clean does not remove the error, but Open/Close does could be related to bug 172345.
Comment 1 Jerome Lanneluc CLA 2007-02-02 09:46:16 EST
While investigating this bug, the first thing to try is to look at the build order after the import. 
Comment 2 Maxime Daniel CLA 2007-02-09 09:25:04 EST
Initial errors reproduced with I20070208-0010, except that I tend to have many more errors (50+). Will investigate.
Comment 3 Maxime Daniel CLA 2007-02-16 08:10:05 EST
My understanding is that the 'simultaneous' opening of a plugin projects series leads RequiredPluginsClasspathContainer#addPlugin to add a given dependency sometimes (if it is already open) as a project, sometimes (if it is not opened yet) as an external plugin. As a result, the subsequent call to ProjectDescription#setDynamicReferences will miss some projects, and the build order will then get miscomputed (because some dependencies are missing in the graph).
Moving to PDE for comment.
Comment 4 Wassim Melhem CLA 2007-02-17 22:41:34 EST
Maxime,

when a project is closed, it is not a plug-in, so RequiredPluginsClasspathContainer#addPlugin must return the external plugin.

when the project is open, it turns into a plug-in and we must return the plug-in project itself.

When asked to resolve the classpath container of a project, we always return a classpath that reflects the state of the workspace at that instant.  

That's the correct behaviour.  Nothing to be done on the PDE side here.
Comment 5 Maxime Daniel CLA 2007-02-18 03:10:21 EST
Wassim,
From the user perspective, the import of a series of plugins is an atomic operation, which correct output is a set of plugin projects which are open and, in the case auto-build is on, built. The fact that some plugins are in the transient 'not open yet' state is an implementation detail, and should not interfere with the desired result. The fact that the set of imported plugins results from the import recommendation (add the dependent plugins) makes us look even worse.
Who owns the import for a set of plugins?
Comment 6 Wassim Melhem CLA 2007-02-18 03:32:18 EST
The operation, owned by PDE, is currently being run as an atomic operation to avoid intermediate updates via:

IWorkspace.run(IWorskpaceRunnable op, IProgressMonitor monitor), where op is the import op.

This is fully equivalent to:
workspace.run(action, workspace.getRoot(), IWorkspace.AVOID_UPDATE, monitor)

However, the platform is still sending an intermediate notification (which can be visually verified by seeing a handful of projects created in the workspace before the operation finishes).  
It is this intermediate notification that is causing the compiler grief.

cc JohnA to comment on why AVOID_UPDATE is not avoiding *all* intermediate updates and/or how the build order can recover from it.
Comment 7 Philipe Mulet CLA 2007-02-19 08:06:06 EST
Re: comment 4 on "when a project is closed, it is not a plug-in"

Wassim : you are the one creating the projects. The fact they are closed or not at some stage doesn't depend on JDT/Core. BTW, why does it even matter in this process ? You know up front the list of projects you are going to create, so why not always ensure that if in the list then a project plug-in ref is added (can't be external); or maybe you need to do this in 2 passes.
Comment 8 Wassim Melhem CLA 2007-02-19 10:51:12 EST
I am not sure why we are talking about 'closed' vs. 'open'.  I don't think that's the issue here.

This is a more general issue than a big plug-in import batch.  For example, it would also happen if you are checking out a large number of projects from CVS (which is an operation not controlled by PDE).  So we need to find a solution for the real problem, not a workaround for operations that PDE controls like the import op (e.g. the 2 pass-idea suggested in comment 7)

When importing/checking out a large number of plug-in projects, periodic workspace notifications are sent out, so the classpath container for some projects is resolved before all the projects are imported/checked out.  In this case, PDE returns a classpath that reflects the current state of the workspace/target and may return JAR classpath entries if the corresponding plug-in project has not been imported yet.  That's fine, of course.

When the import/check-out operation completes, all the classpaths get set/readjusted by PDE and every project's classpath reflects the new workspace state as it should.

So it would be good if JohnA or someone from JDT would explain to me:
if all the classpaths for all projects are all correct at the end of the check-out/import, why is the overall build order still messed up?
Comment 9 Philipe Mulet CLA 2007-02-19 11:27:16 EST
Re: closed/open. I only mentionned this because you had argued based on it.

I agree that a general purpose solution should be looked for. But workarounds for standard situations wouldn't hurt either (think being paranoid). When the workspace gets into this state, users are really confused, and it is not an easy operation to make it consistent again.
Comment 10 Philipe Mulet CLA 2007-02-19 11:52:08 EST
Actually, we need to clarify whether the buildpath (expanded containers) is actually right or wrong at the end of the import/check-out operation.
If the problem only resides in dynamic project refs, then this would be a bug in JDT/Core. 

Maxime - pls double check this, and adopt the problem if we are guilty.
Comment 11 John Arthorne CLA 2007-02-19 11:59:19 EST
I don't know what's going on here. It sounds similar to bug 154880.  I think it's important not to focus on the PDE import case here. I've gotten into such a broken state recently while checking out projects from a proprietary source control system (where only closing/opening projects gets me back into a good state). I.e., assume some third party could be creating/opening projects and that resource change events are always possible at any time.
Comment 12 Maxime Daniel CLA 2007-02-20 01:48:50 EST
I would take from John's comment that we neither have, nor want, any kind of multi-project transaction model here? The consequence would be that more rebuilds would have to occur, but that would still be a good choice anyway if it drove some complexity out of the picture.
I will investigate further the scenario we have and report my findings here.
Comment 13 John Arthorne CLA 2007-02-20 09:45:10 EST
I'm not sure what you mean by a multi-project transactional model, but I should point out that IWorkspaceRunnable does guarantee that autobuild won't happen in the middle of the runnable.  There may be intermediate POST_CHANGE resource change events, but no builds.
Comment 14 Philipe Mulet CLA 2007-02-20 10:45:10 EST
To clarify, even if no intermediate (auto)build occurs, a change event notification is enough to cause classpath computation in intermediate state. We need to assess whether the intermediate state is fixed up later on at the completion of the import/checkout. 
One idea we discussed here was that if the PDE container is set in intermediate state, and not fixed up later on (or incorrectly handled), then we will end up in this state.

One additional point is that it is pretty hard to find out if the buildpath is right in the end, since expanding the container in the package explorer at the end is simply displaying the current contents of a given container (it doesn't guarantee that JDT/Core noticed the change. Basically, there is a protocol to use when updating a container so as to indicate a change. Wild guess: if updating using #setClasspathContainer(...) but reusing the same container instances after update could be ruining us - need to double check if this is the case or not).
Comment 15 Maxime Daniel CLA 2007-02-20 11:14:55 EST
Created attachment 59383 [details]
One trace of a peculiar instance of the scenario

Seems we have a concurrency problem deep into the call to SetContainerOperation.
Comment 16 Maxime Daniel CLA 2007-02-21 01:26:36 EST
Created attachment 59449 [details]
DO NOT RELEASE - instrumentation patch

This is a Q&D tracing patch (that produces traces like the one attached above). Meant to help decypher the concurrency/timing issues at work here.
Comment 17 Maxime Daniel CLA 2007-02-21 06:49:52 EST
Jerome, following our conversation.
Comment 18 Jerome Lanneluc CLA 2007-02-23 10:23:25 EST
Scenarion is actually quite complex:
1. Import is run inside an IWorkspaceRunnable
2. After an arbitrary amount of time, the Platform triggers an IResourceChangeEvent saying that the first imported projects are added
3. One of the project (org.eclipse.ant.core) is added, but doesn't have the Java nature yet: the DeltaProcessor ignores it
4. At the end of the import, pre-resourceChangeListeners are run. 
5. One of them is PDE's and update the classpath containers.
6. Since we are in an IWorkspaceRunnable, the SetClasspathOperation doesn't update the project references yet
7. The Platform triggers the remaining ResourceChangeEvent
8. One of the change describes the addition of the Java nature to the project org.eclipse.ant.core.
9. Unfortunately, the DeltaProcessor doesn't update the project references in this case
Comment 19 Jerome Lanneluc CLA 2007-02-23 10:26:34 EST
Created attachment 59660 [details]
Fix and regression tests

This patch fixes the DeltaProcessor to update the project references when the Java nature is added to a project (and also when the project is open).
It contains 2 regression tests: JavaProjectTests#testProjectImport2() and testProjectImport3()
Comment 20 Jerome Lanneluc CLA 2007-02-23 10:40:05 EST
Fix and regression tests released for 3.3 M6 in HEAD.
Comment 21 Jerome Lanneluc CLA 2007-03-14 09:20:46 EDT
Note that this bug didn't exist in 3.2.x.
Backported regression tests in R3_2_maintenance.
Comment 22 Maxime Daniel CLA 2007-03-20 02:59:13 EDT
Verified for 3.3 M6 using build I20070319-1335.