Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 325814

Summary:

[builder] cancelation and the XtextBuilder

Product:

[Modeling] TMF

Reporter:

Knut Wannheden <knut.wannheden>

Component:

Xtext

Assignee:

Project Inbox <tmf.xtext-inbox>

Status:

CLOSED FIXED

QA Contact:

Severity:

major

Priority:

CC:

mirko, sebastian.zarnekow, sven.efftinge

Version:

1.0.1

Flags:

sebastian.zarnekow: helios+
sebastian.zarnekow: indigo+

Target Milestone:

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
proposed patch	none
patch updated with test	none
updated patch now including test case	none

Description Knut Wannheden

2010-09-21 03:21:03 EDT

As documented in the IncrementalProjectBuilder#build() method and in http://http://www.eclipse.org/articles/Article-Builders/builders.html (see section "Handling cancelation") a builder is expected to call forgetLastBuiltState() and throw an OperationCanceledException if the progress monitor requests the operation to be canceled.

Currently the XtextBuilder will simply return when the user attempts to cancel the build.

I think the ClusteringBuilderState should also be allowed to throw an OperationCanceledException exception which the Xtext Builder would catch and re-throw:

    try {
        ...
    }  catch (OperationCanceledException e) {
        forgetLastBuiltState();
        throw e;
    }

Comment 1 Knut Wannheden

2010-09-21 07:41:01 EDT

Created attachment 179301 [details]
proposed patch

Attached a proposed patch. Please review.

Comment 2 Sebastian Zarnekow

2010-09-21 07:47:59 EDT

Knut, thanks for the patch.

May builder participants throw an OperationCanceledException as well or should the calling component deal with this state?

Could you please provide a testcase to illustrate that the next triggered build is a full build?

Marked as Indigo+

Comment 3 Knut Wannheden

2010-09-21 11:36:07 EDT

Sebastian,

I will document that the builder participants are also expected to throw OperationCanceledExceptions and update RegistryBuilderParticipant accordingly.

Isn't the test case you propose more of a test for the Eclipse build manager instead of for the XtextBuilder? Also I don't quite see how you would go about implementing that test. Would the test override the XtextBuilder binding?

Comment 4 Sebastian Zarnekow

2010-09-21 11:40:02 EDT

Knut,

I'ld register a builder participant in the test that throws an operation canceld on the first run and logs the build kind on the second invocation. Creating and modifying a resource with the existing builder test infrastructure should be straight forward, shouldn't it?

Comment 5 Knut Wannheden

2010-09-21 11:43:27 EDT

OK, thanks. I'm sure you're right :-) I'll take a look at BuilderParticipantTest.

Comment 6 Knut Wannheden

2010-09-22 03:00:08 EDT

Created attachment 179355 [details]
patch updated with test

I've attached an updated patch containing a test case as requested.

Comment 7 Knut Wannheden

2010-09-22 03:08:01 EDT

Created attachment 179356 [details]
updated patch now including test case

Not quite up to speed with git diff etc... This patch now includes the test case!

Comment 8 Sebastian Zarnekow

2010-09-22 03:19:57 EDT

Thanks for the updated patch. Besides this assertion: assertTrue(0 < participant.getInvocationCount());
the test looks good to me. You could get rid of it, if you add an waitForAutobuild in line 55 and reset the builder participant afterwards.

Furtermore there seem to be some minor inconsistencies with the formatting (at least the diff in the web interface suggests this - see AbstractBuilderState#145/146). A reformat should help.

Please feel free to apply the fix.

Comment 9 Knut Wannheden

2010-09-22 03:36:05 EDT

Thanks for the review.

Do we still have an Eclipse formatter config somewhere?

Comment 10 Sven Efftinge

2010-09-22 03:37:25 EDT

the formatting settings are applied (and checked-in) on the project level.

Comment 11 Knut Wannheden

2010-09-22 03:48:53 EDT

I explicitly didn't reformat the entire sources before because it changes things around quite heavily. Do you typically run the formatter on selected regions only?

Comment 12 Sebastian Zarnekow

2010-09-22 04:18:55 EDT

I usually try to keep the existing formatting as well. However in this case it seems to be inconsistent and that's worth to be fixed. I'd simply reformat the file and double check the result.

Comment 13 Knut Wannheden

2010-09-22 04:45:44 EDT

Fix pushed to master. Sorry about all the whitespace diffs...

Comment 14 Knut Wannheden

2010-09-24 04:46:12 EDT

The BuildScheduler calls the incremental project builder and has thus to cope with any OperationCanceledExceptions by returning Status.CANCEL_STATUS. Currently it logs an error and returns an ERROR status.

Comment 15 Knut Wannheden

2010-09-24 04:47:48 EDT

fix pushed to master

Comment 16 Sebastian Zarnekow

2010-11-08 05:57:05 EST

(In reply to comment #15)
> fix pushed to master

Knut, would you please be so kind and backport this one to the Helios_Maintenance branch for the SR2?

Comment 17 Knut Wannheden

2010-11-08 08:13:33 EST

(In reply to comment #16)
> Knut, would you please be so kind and backport this one to the
> Helios_Maintenance branch for the SR2?

Certainly. I will do that this week.

Comment 18 Knut Wannheden

2010-11-09 09:23:30 EST

Fix backported to Helios_Maintenance.

Comment 19 Sebastian Zarnekow

2011-04-26 17:33:56 EDT

*** Bug 343771 has been marked as a duplicate of this bug. ***

Comment 20 Karsten Thoms

2017-09-19 17:09:30 EDT

Closing all bugs that were set to RESOLVED before Neon.0

Comment 21 Karsten Thoms

2017-09-19 17:21:23 EDT

Closing all bugs that were set to RESOLVED before Neon.0