Bug 317402

Comment 1 John Dallaway 2010-06-21 04:06:02 EDT

Context, copied from bug 317162 :

> > More significantly, with CDT 6, when I build a project, _all_ the builders
> > associated with any referenced projects are invoked (not just the "CDT
> > Builder"). With CDT 7, it appears that only the CDT Builder is invoked for
> > the referenced projects. Some of my projects will therefore no longer build
> > correctly when built indirectly (as a referenced project) because the CDT
> > Builder requires a source file which is generated by a preceding builder in
> > the ordered list of builders for the project.
> 
> This does sound like a regression.  The issue is that previously:
>   - UI code called build on all referenced projects, before calling build on
> the top-level project
>   - CDT Builder actually runs make directly in the referenced projects,
> without going through core.resources IProject.build. 
> 
> Together this results in projects being built more than once, and the CDT
> builder behaviour causes problems: with build delta, pre-build/post-build
> event flow, resource locking, etc.
> 
> The root of the problem is the way the cdt builder implicitly builds
> references (because the platform doesn't give any context on what's being
> built...). We should fix this for 7.0.1.  It would be quicker if you could
> contribute a patch?

Comment 2 John Dallaway 2010-06-21 04:32:22 EDT

We need to agree what the correct/desirable behaviour should be.

As far as I know:

a) CDT 6 behaviour is to build the currently active project configuration for referenced projects. This is not useful if the currently active configuration of the referenced and referring projects do not match. The referenced project is built in exactly the same way as if it had been built directly via IProject.build which is robust.

b) With CDT 7, a more sophisticated project referencing system and UI allows specific configurations of referenced projects to be built. This is useful, but the referenced project is now built in the context of the referring project and only the CDT Builder is used. This makes certain assumptions about the nature of the referenced project, is less robust and can lead to build failures.

Note that it should be possible for a C/C++ project to reference another project of _any_ nature.

Without having looked at the code, my high-level proposal would be to revert to using IProject.build for robustness, but temporarily switch the active build configuration of the referenced project if necessary to ensure that the appropriate C/C++ configuration is built. The UI would require no modification.

Does anyone see any issue with changing the active configuration of a referenced C/C++ project temporarily?

Would a patch along these lines be acceptable?

Comment 3 James Blackburn 2010-06-21 05:35:17 EDT

(In reply to comment #2)
> We need to agree what the correct/desirable behaviour should be.
> 
> As far as I know:
> ...

I don't think this is quite right. The CDT common build builder does what it did before.  
The only change from CDT 6 is the addition org.eclipse.cdt.ui.BuildGroup.java:CDTBuildAction. This overrides the UI BuildAction. (bug 291751 has the patch that was committed.)

In CDT 6:
 1) Platform project references are all IProject.build(...). Building the
 2) CDT referenced configurations are then built.
The result is that the CDT CommonBuilder is run more than once, and projects are built multiple times.

In CDT7:
 CDTBuildAction doesn't perform 1 above, and CommonBuilder performs 2 as before.

> Note that it should be possible for a C/C++ project to reference another
> project of _any_ nature.

Agree.

> Without having looked at the code, my high-level proposal would be to revert to
> using IProject.build for robustness, but temporarily switch the active build
> configuration of the referenced project if necessary to ensure that the
> appropriate C/C++ configuration is built. The UI would require no modification.

I don't think we should do this.  There are two problems with this proposal I can see:
 1) It's heavyweight. Changing active build config will trigger a full project re-index if user has "Indexer follows build configuration"
 2) It still won't work right.  In general the CDT project reference graph can not be contained in a project reference graph.  Consider the simple example of a single project with two configurations: someLibraryLib and someLibraryTest.  someLibraryTest is a test binary which links against someLibraryLib.  To build the test you need to build the project's two configurations, in the correct order.

The quick fix (to restore the previous behaviour) is to remove shouldPerformResourcePruning() from CDTBuildAction.

I agree that what we do in CommonBuilder is bad, and goes against how platform build works. This code is historical and has behaved that way since CDT 4 (bug 161603). 

One way I think of fixing this (requiring Platform improvements) would be to register a CDT builder per CDT configuration. Our build action would enable / disable individual builders, and we could enhance the platform to express dependency between individual builders (or project as currently occurs).

Comment 4 James Blackburn 2010-06-21 05:52:47 EDT

Created attachment 172313 [details]
revert 7.0.x to 6.0 behaviour

John can you confirm whether this fixes the issue for you?  This reverts the 6 behaviour (reopens bug 291751).

We need to fix this differently / properly on HEAD.

Comment 5 John Dallaway 2010-06-21 09:05:53 EDT

(In reply to comment #4)
> Created an attachment (id=172313) [details]
> revert 7.0.x to 6.0 behaviour
> 
> John can you confirm whether this fixes the issue for you?  This reverts the 6
> behaviour (reopens bug 291751).

James, I can confirm that the patch restores previous (CDT 6) behaviour. We agree that this is not ideal, but it does allow C/C++ projects to reference and build projects of other natures and of multiple natures. From my perspective, this is a critical feature and I think the patch should be applied for CDT 7.0.1. Thanks.

Comment 6 James Blackburn 2010-06-21 09:24:22 EDT

Ok, thanks for testing. 
Patch applied to 70x and HEAD. I've re-opened bug 291751 to reconsider the fix for 8.0.

Comment 7 CDT Genie 2010-06-21 12:23:27 EDT

*** cdt cvs genie on behalf of jblackburn ***
Bug 317402 Not all builders invoked for referenced projects. Partial revert of patch on bug 291751

[*] BuildGroup.java 1.16.2.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/cview/BuildGroup.java?root=Tools_Project&r1=1.16&r2=1.16.2.1

[*] BuildGroup.java 1.17 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/cview/BuildGroup.java?root=Tools_Project&r1=1.16&r2=1.17

Comment 8 James Blackburn 2011-05-09 16:15:41 EDT

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