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

Bug 335476

Summary: CDT build doesn't notice cdt.core.prefs is modified externally
Product: [Tools] CDT Reporter: James Blackburn <jamesblackburn+eclipse>
Component: cdt-buildAssignee: cdt-build-inbox <cdt-build-inbox>
Status: NEW --- QA Contact: Jonah Graham <jonah>
Severity: normal    
Priority: P3 CC: cdtdoug, yevshif
Version: 8.0   
Target Milestone: ---   
Hardware: PC   
OS: Linux-GTK   
Whiteboard:
Bug Depends on: 295436    
Bug Blocks:    

Description James Blackburn CLA 2011-01-26 12:24:13 EST 
I can reproduce this issue again...

+++ This bug was initially created as a clone of Bug #295436 +++

To reproduce:

1) Create hello world project
2) Disable build automatically
3) Set an environment variable on a configuration
4) build the project
5) Change the variable in org.eclipse.cdt.core.prefs 
6) Examine the environment in Project Properties (Note that cdt.core has noticed the environment has changed)
7) Press cancel on the dialog
8) Build project 

At step 8 the environment set during build is the previous environment. If you press build again (without changing anything) the environment becomes correct.

There are now tests in HEAD for 5&6 -- i.e. external changes are picked up by cdt.core using the IEnvManager getVariables() API. However somehow the builder is also caching the environment(?).

If you enable autobuild, or change the environment through the UI (resulting in an explicit project description change), everything remains in sync.
Comment 2 James Blackburn CLA 2011-01-27 12:45:59 EST 
There's a test committed which demonstrates the problem.  I'm not sure what the best way to fix this is...

Eclipse notices that the environment file has changed, and the configurations on the project tap directly into that (it wouldn't make sense for the read-only configurations to reflect stale data). However the MBS, which has another cache of the data under the IManagedBuildInfo tree, has the old values.

   - I could fire another loaded event when things change externally - though all this caching and object creation would make this more expensive than it already is.
   - I could make the MBS use getProjectDesc on the core rather than relying on it's cache.

To me the model seems completely broken:
 - On the MBS side all state is static'ly accessible and writable.  Makes it easy for multiple users in multiple threads to change state as they see fit with the associated data races and corruption.
 - On the cdt.core side the model transition from read-only to writable to read-only, with no regard to concurrent users.  If there are concurrent accesses, one writer wins (so the only safe thing to do is for *every* caller to do getProjDesc + setProjDesc within a resource scheduling rule held).
  - There is no effective delta.  Yes a listener can get the old and new description. However they must manually check every interesting bit of state.

*frustrated*
Comment 3 Doug Schaefer CLA 2011-01-27 12:50:08 EST 
(In reply to comment #2)
>    - I could fire another loaded event when things change externally - though
> all this caching and object creation would make this more expensive than it
> already is.
>    - I could make the MBS use getProjectDesc on the core rather than relying on
> it's cache.

Since this doesn't happen very often, option 1 may be worth the cost to keep things simple. Option 2 probably should be where we're going long term. Duplicating data is never a good thing for the exact reasons that lead to this bug.

> *frustrated*

Of that, I have no doubt :)
Comment 4 Andrew Gvozdev CLA 2011-01-27 13:40:22 EST 
(In reply to comment #2)
> To me the model seems completely broken:
> - On the MBS side all state is static'ly accessible and writable.  Makes it
> easy for multiple users in multiple threads to change state as they see fit with
> the associated data races and corruption.
> - On the cdt.core side the model transition from read-only to writable to
> read-only, with no regard to concurrent users.  If there are concurrent
> accesses, one writer wins (so the only safe thing to do is for *every* caller to
> do getProjDesc + setProjDesc within a resource scheduling rule held).
The model has many parts some of which pretty loosely connected. Even the writers concern with unrelated parts the latter one takes away what was written by the other one. Do you think it is good idea (on the long run) to reduce granularity of setProjDesc to smaller pieces? Each one could be persisted separately so you could easier figure out what changed and update only that part of the model.

> - There is no effective delta.  Yes a listener can get the old and new
> description. However they must manually check every interesting bit of state.
> 
> *frustrated*
Comment 5 James Blackburn CLA 2011-01-27 14:01:08 EST 
(In reply to comment #4)
> Each one could be persisted
> separately so you could easier figure out what changed and update only that
> part of the model.

I think so.  Even at the moment the core.model as well as CDT preferences are distinct from the project description, and I think this is good.  

The project description and config desc hierarchy, by virtue of where they've come from, is all about defining build system configuration and build system language settings.  The platform is learning about build configurations. And the current state of the project description (and it's scalability issues) mean I think we shouldn't bolt anything more into it. 

Clearly build settings are one cohesive unit, but it makes sense that unrelated features are managed separately where they're not part of the build configurations.