eclipse / xtext-eclipse

xtext-eclipse
Eclipse Public License 2.0
49 stars 73 forks source link

[#1710] Don't touch the project again if it was already built #1727

Closed szarnekow closed 3 years ago

szarnekow commented 3 years ago

The window of opportunity for the autobuild is the time between forgetting the last built state and the touch of the project in its own job. Check if there is a known built state before touching the project again.

Signed-off-by: Sebastian Zarnekow sebastian.zarnekow@gmail.com

szarnekow commented 3 years ago

cc @Bananeweizen Can you please give this a try?

cdietrich commented 3 years ago

mmm on the first try it does not work (for java projects) (restart before import project with one dmodel file) forgetLastBuildState happens during build running

BuilderStateDiscarder.forgetLastBuildState(Iterable<IProject>, Map<String,String>) line: 65 
BuildScheduler.scheduleBuildIfNecessary(Iterable<IProject>, IBuildFlag...) line: 68 
ProjectClasspathChangeListener.scheduleBuildIfNecessary(Set<IProject>) line: 87 
ProjectClasspathChangeListener.elementChanged(ElementChangedEvent) line: 74 
DeltaProcessor$3.run() line: 1755   
SafeRunner.run(ISafeRunnable) line: 45  
DeltaProcessor.notifyListeners(IJavaElementDelta, int, IElementChangedListener[], int[], int) line: 1743    
DeltaProcessor.firePostChangeDelta(IJavaElementDelta, IElementChangedListener[], int[], int) line: 1576 
DeltaProcessor.fire(IJavaElementDelta, int) line: 1552  
DeltaProcessor.notifyAndFire(IJavaElementDelta) line: 2273  
DeltaProcessor.resourceChanged(IResourceChangeEvent) line: 2163 
DeltaProcessingState.resourceChanged(IResourceChangeEvent) line: 501    
NotificationManager$1.run() line: 305   
SafeRunner.run(ISafeRunnable) line: 45  
NotificationManager.notify(ResourceChangeListenerList$ListenerEntry[], ResourceChangeEvent, boolean) line: 295  
NotificationManager.broadcastChanges(ElementTree, ResourceChangeEvent, boolean) line: 158   
Workspace.broadcastPostChange() line: 381   
Workspace.endOperation(ISchedulingRule, boolean) line: 1503 
RefreshJob(InternalWorkspaceJob).run(IProgressMonitor) line: 49 
Worker.run() line: 63   

@Bananeweizen @rytina do you have normal project or java projects?

=> i still have doubts how to properly test/reproduce this

szarnekow commented 3 years ago

Testing this is indeed challenging. I’ll revisit the fix.

rytina commented 3 years ago

do you have normal project or java projects?

@cdietrich @szarnekow we have normal projects. Our Xtext grammar does not use Xbase.

szarnekow commented 3 years ago

do you have normal project or java projects?

@cdietrich @szarnekow we have normal projects. Our Xtext grammar does not use Xbase.

Thank you for the quick response. The approach taken in the current state of the merge request is not sufficient yet for either type of projects. There are more moving parts involved and an unpleasant mix of different locks at play that need to be straighten. I’ll keep you posted.

cdietrich commented 3 years ago

Bug486584Test fails even if I replace the jar with a jar containing a buildertestlanguage file namespace viech { object Viech } and the test file to namespace bar { object B references viech.Viech }

szarnekow commented 3 years ago

Let's see what Jenkins thinks about this approach.

szarnekow commented 3 years ago

@Bananeweizen @rytina @cdietrich I think this is good for a review and some battle testing.

cdietrich commented 3 years ago

does not work for mydsl general projects for me

[Worker-0: Building] Building demo2
[Worker-0: Building] Built demo2 in 10 ms
[Worker-7: Building] interrupted
[Worker-7: Building] Build interrupted.
[Worker-7: Building] Built demo2 in 1045 ms
cdietrich commented 3 years ago

same on open a closed mydsl general project after restart see a interruped build only and not a follow up build

cdietrich commented 3 years ago

looks like the problem i saw happens on all opens. TODO: check if works in real project TODO: check if old behaviour can easily be restored by projects when needed (subclassing)

rytina commented 3 years ago

@szarnekow I copied the changed files over to our target to test the bug locally and I can confirm it's fixed. The builds only happens once and the recovery build is also working perfectly. Thank you! :-)

rytina commented 3 years ago

@szarnekow could you please merge your PR and provide us an update site, so that I can test your change with all other changes that will come with Xtext 2.26?