Community
Participate
Working Groups
Build Identifier: I20110613-1736 I've found a couple of issues concerning the incremental build which seem tricky enough to deserve reporting (if some of this is already known, please forgive me). Both issues relate to Java type deletion, so I decided to report them together, but their fundamental nature may be different. Reproducible: Always Steps to Reproduce: 1. Import the standard 'Xtext Domain-Model Example' projects into the workspace. 2. Run the runtime workbench and import the attached 'example' project into the runtime workspace. 3. *** issue #1 Open '_Foo.java' (note the underscore) and comment out its entire contents. Wait a second for reconciling to happen (seems important). Save the changes. Note that 'Foo.dmodel' has not been rebuilt (no error marker on it). 4. *** issue #2 Revert the changes made on the previous step and make a clean build (or start with a freshly imported 'example' project). Open 'Foo.dmodel' and comment out the entire declaration of entity 'Foo'. Save the changes. Note that 'Bar.dmodel' has not been rebuilt (no error marker on it).
Created attachment 201643 [details] 'example' project
Issue #1 seems to be quite straightforward to explain: DeltaConverter converts the corresponding IJavaElementDelta into nothing, because it can't find any types in '_Foo.java' compilation unit. So nobody gets aware of the change. The corresponding JDT event looks like: org.eclipse.jdt.core.ElementChangedEvent[source=Java Model[*]: {CHILDREN} example[*]: {CHILDREN} src[*]: {CHILDREN} example[*]: {CHILDREN} [Working copy] _Foo.java[*]: {PRIMARY RESOURCE}] I shall report my findings regarding issue #2 in a while. It seems more tricky (and more interesting, I think).
Some more notes on the first issue. It seems related to bug 354605. In both cases the DeltaConverter must deal with a 'course-grained' Java delta (as opposed to F_FINE_GRAINED), which just says 'a primary resource has changed' without going into details of what concretely has changed inside the compilation unit. The challenge for DeltaProcessor is that in this case it has no direct way of getting names of 'removed' top-level types in the CU. If we ignore secondary types (which Xtext seems to do currently, if I'm not mistaken), then one possible strategy can be described as follows. If there is no types in the CU, fire a ChangedResourceDescriptionDelta with a name-based 'oldDescription' for the type corresponding to the name of the CU. If there is a public top-level type, but its name is different from the name of the CU, fire a ChangedResourceDescriptionDelta with a name-based 'oldDescription' for the type corresponding to the name of the CU _and_ a ChangedResourceDescriptionDelta with a 'newDescription' for the present top-level type. A drawback of this strategy is that it makes possible duplication of events (i.e., a delta for the same 'removed' type may be fired more than once). It doesn't seem critical for dependency management. Other clients will have to be made resilient (idempotent). In case we must confront secondary types there seems to be no other way than some kind of indexing, so that we can compare the 'new' state of the CU with its 'old' state. It may have its own drawbacks (e.g. complexity, performance), but is a cleaner solution.
Now to the second issue. I've had a fairly long debugging session to investigate the inner workings of this and here are the results. Several words about the debugging itself at first (as I think it may be useful to describe it). Firstly I made a clean build, then turned off the auto-build and set the following breakpoints (line numbers correspond to 2.0.0 release): BuildManager [line: 356] - basicBuildLoop(IBuildConfiguration[], IBuildConfiguration[], int, MultiStatus, IProgressMonitor) ClusteringBuilderState [line: 130] - doUpdate(BuildData, ResourceDescriptionsData, IProgressMonitor) JavaChangeQueueFiller [line: 37] - elementChanged(ElementChangedEvent) JavaProjectBasedBuilderParticipant [line: 122] - handleChangedContents(Delta, IBuildContext, IFileSystemAccess) XtextBuilder [line: 116] - incrementalBuild(IResourceDelta, IProgressMonitor) So the initial state of affairs was: 'Foo' exports its inferred type 'Foo' 'Bar' is linked to the 'Foo's inferred type Then I commented out the entire declaration of entity 'Foo' and invoked 'Build All' command. The sequence of events observed was as following. *** round 1 of build loop. First, the ClusteringBuilderState is invoked: 1. 'Foo.dmodel' is built (and no more exports the inferred type 'Foo') 2. 'Bar.dmodel' is built (as 'affected' by 'Foo.dmodel'). 'Bar' entity is (re-)linked to the 'real' Java type 'Foo' (i.e., 'java:/Objects/foo.Foo') - which still exists at this point. The build participants are then invoked: 3. 'Foo.java' is deleted. The corresponding resource delta is sent asynchronously (sic!) and converted into nothing (sic!) by DeltaConverter in JavaChangeQueueFiller. See the attached callstack1.txt for details. 4. 'Bar.java' is (re-)generated. Note that the corresponding resource delta is not sent at this point. *** round 2 of build loop. XtextBuilder is called for the '/example/bin/...' delta (note that there is no delta for the changes made to '/example/src-gen' in the previous round). This time, the ClusteringBuilderState is NOT called, because the 'buildData' isEmpty. *** end of build operation. The corresponding resource delta is sent for 'Bar.java' (synchronously). JavaChangeQuequeFiller converts it into the Xtext delta for 'java:/Objects/bar.Bar' and puts the converted delta into the queue. See the attached callstack2.txt for details. *** END. Note that 'Bar.dmodel' description still references (not existing) type 'Foo'.
Created attachment 201695 [details] call stack #1
Created attachment 201696 [details] call stack #2
So it seems that a couple of problems are spotted by issue #2. The obvious one is that the DeltaConverter doesn't handle REMOVED compilation units. The less obvious one is that even if the first problem was solved, there is absolutely no guarantee that the Java deltas will be queued and processed in the same build cycle that have generated Java source or deleted compilation units, since workspace POST_CHANGE notifications (and hence JDT events) may be sent anytime after the changes occurred (from the same thread that made the changes or from a different one).
Thanks for the detailed report.
Created attachment 201925 [details] Proposed patch for issue #2 Some notes on the patch. The main part of the fix is using IWorkspace#checkpoint(false) to let the JavaChangeQueueFiller know about the changes made by participants. This ensures that those changes will be processed in the same build cycle. The trick with checkpointing was told me by John Arthorne himself (see bug 341360). Note that after applying the patch, exactly converse problem appears: when we uncomment the 'Foo' entity declaration and save the change, 'Bar.dmodel' is not going to be rebuilt and still has an error marker on it. That's caused by bug 354910.
Created attachment 201981 [details] Proposed patch A better patch. Fixes both issues. Also fixes bug 354605.
Excuse me for disturbance, but is there any chance to get it fixed in 2.1? The proposed fix may be a little rough, but at least it can serve as a workaround for the meantime and could be bettered in the future. Issue #2 described above seems like quite a major issue, doesn't it?
(In reply to comment #11) > Excuse me for disturbance, but is there any chance to get it fixed in 2.1? > > The proposed fix may be a little rough, but at least it can serve as a > workaround for the meantime and could be bettered in the future. Issue #2 > described above seems like quite a major issue, doesn't it? Schedule for review. Please add a unit test to your patch that illustrates the problem. org.eclipse.xtext.common.types.ui.notification.RebuildDependentResourcesTest may serve as a starting point.
Created attachment 204445 [details] Updated patch Updated patch. Reflects the current state of master
Created attachment 204447 [details] Unit tests Patch to 'org.eclipse.xtext.common.types.tests' demonstrating the problem (both issue #1 and #2)
The patch and the tests look good. Sorry for keeping you on hold for that long. I've committed that to MASTER. I also had to change the way the MockJavaProject is being created in order to avoid side effects in tests from other plug-ins because of the newly registered builder participant.
Thank you, Jan. Please note that the patch should also fix bug 354605.
Checked it and it's fixed indeed. Thanks for the hint.
Requested via bug 522520. -M.
