Bug 265282 Environment persistence reload issues. Previously environment was loaded from the preference store and cached forever more. StorableEnvironment now has a handle on the ISerializeInfo so it can respond to external changes outside of CDT's realm.

core/org.eclipse.cdt.core.tests/misc/org/eclipse/cdt/core/envvar/IEnvironmentVariableManagerTests.java

@@ -0,0 +1,178 @@
+/*******************************************************************************
+ * Copyright (c) 2009 Broadcom Corp. and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *    James Blackburn - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.cdt.core.envvar;
+
+import junit.framework.Test;
+import junit.framework.TestCase;
+import junit.framework.TestSuite;
+
+import org.eclipse.cdt.core.CCorePlugin;
+import org.eclipse.cdt.core.internal.errorparsers.tests.ResourceHelper;
+import org.eclipse.cdt.core.model.CoreModel;
+import org.eclipse.cdt.core.settings.model.ICConfigurationDescription;
+import org.eclipse.cdt.core.settings.model.ICProjectDescription;
+import org.eclipse.cdt.core.settings.model.util.CDataUtil;
+import org.eclipse.core.resources.IProject;
+import org.eclipse.core.resources.IResource;
+import org.eclipse.core.resources.ResourcesPlugin;
+import org.eclipse.core.runtime.jobs.IJobManager;
+import org.eclipse.core.runtime.jobs.ISchedulingRule;
+import org.eclipse.core.runtime.jobs.Job;
+
+public class IEnvironmentVariableManagerTests extends TestCase {
+
+	@Override
+	protected void setUp() throws Exception {
+		super.setUp();
+	}
+
+	@Override
+	protected void tearDown() throws Exception {
+		ResourceHelper.cleanUp();
+	}
+
+	public static Test suite() {
+        TestSuite suite = new TestSuite(IEnvironmentVariableManagerTests.class);
+        return suite;
+	}
+
+	/**
+	 * Create a project with 2 configurations. Set an environment variable on one of
+	 * the configurations. Close and reopen the project. Check persistence
+	 * @throws Exception
+	 */
+	public void testSimpleVar() throws Exception {
+		final IProject project = ResourceHelper.createCDTProjectWithConfig("envProject");
+
+		// Add another, derived configuration
+		ICProjectDescription prjDesc = CoreModel.getDefault().getProjectDescription(project);
+		ICConfigurationDescription desc = prjDesc.getActiveConfiguration();
+		final String id1 = desc.getId();  	      // Config 1's ID
+		final String id2 = CDataUtil.genId(id1);  // Config 2's ID
+		prjDesc.createConfiguration(id2, "config2", desc);
+		CoreModel.getDefault().setProjectDescription(project, prjDesc);
+
+		// Get all the configurations
+		prjDesc = CoreModel.getDefault().getProjectDescription(project);
+		ICConfigurationDescription[] descs = prjDesc.getConfigurations();
+		assertTrue(descs.length == 2);
+
+		IEnvironmentVariableManager envManager = CCorePlugin.getDefault().getBuildEnvironmentManager();
+		IContributedEnvironment contribEnv = envManager.getContributedEnvironment();
+
+		// Try setting an environment variable
+		final IEnvironmentVariable var = new EnvironmentVariable("FOO", "BAR");
+		contribEnv.addVariable(var, prjDesc.getConfigurationById(id1));
+
+		// Check that the variable exists on the config1 & not in config2:
+		IEnvironmentVariable var2 = envManager.getVariable(var.getName(), prjDesc.getConfigurationById(id1), true);
+		assertEquals(var2, var);
+		var2 = envManager.getVariable(var.getName(), prjDesc.getConfigurationById(id2), true);
+		assertNull(var2);
+
+		CoreModel.getDefault().setProjectDescription(project, prjDesc);
+
+		// Close and reopen, variables should still exist
+		project.close(null);
+		project.open(null);
+		descs = CoreModel.getDefault().getProjectDescription(project).getConfigurations();
+		var2 = envManager.getVariable(var.getName(), prjDesc.getConfigurationById(id1), true);
+		assertEquals(var2, var);
+		var2 = envManager.getVariable(var.getName(), prjDesc.getConfigurationById(id2), true);
+		assertNull(var2);
+	}
+
+
+	/**
+	 * This bug checks for an environment load race during project open / import.
+	 *
+	 * This occurs because enviornment is stored using platform Preferences (persisted in
+	 * the .settings directory) and, when background refresh is enabled this is loaded
+	 * asynchronously.
+	 *
+	 * The model shouldn't cache incorrect variables / values in the project description
+	 * @throws Exception
+	 */
+	public void testBug265282() throws Exception {
+		final IProject project = ResourceHelper.createCDTProjectWithConfig("envProject");
+
+		// Add another, derived configuration
+		ICProjectDescription prjDesc = CoreModel.getDefault().getProjectDescription(project);
+		ICConfigurationDescription desc = prjDesc.getActiveConfiguration();
+		final String id1 = desc.getId();
+
+		IEnvironmentVariableManager envManager = CCorePlugin.getDefault().getBuildEnvironmentManager();
+		IContributedEnvironment contribEnv = envManager.getContributedEnvironment();
+
+		// Try setting an environment variable
+		final IEnvironmentVariable var = new EnvironmentVariable("FOO", "BAR");
+		final IEnvironmentVariable var1 = new EnvironmentVariable("FOO1", "BAR1");
+		final IEnvironmentVariable var2 = new EnvironmentVariable("FOO2", "BAR2");		
+		contribEnv.addVariable(var, prjDesc.getConfigurationById(id1));
+
+		// Check that the variable exists on config1
+		IEnvironmentVariable readVar = envManager.getVariable(var.getName(), prjDesc.getConfigurationById(id1), true);
+		assertEquals(var, readVar);
+
+		// Save the project description
+		CoreModel.getDefault().setProjectDescription(project, prjDesc);
+
+		// Delete and reimport the project, environment should persist...
+		project.close(null);
+		project.delete(false, null);
+
+		IJobManager jm = Job.getJobManager();
+		ISchedulingRule root = ResourcesPlugin.getWorkspace().getRoot();
+
+		// Var 3 is to overwrite var2
+		final IEnvironmentVariable var3 = new EnvironmentVariable("FOO2", "BAR3");
+		try {
+			// lock the workspace preventing any asynchronous refresh job from detecting new environment
+			jm.beginRule(root, null);
+
+			project.create(null);
+			project.open(IResource.BACKGROUND_REFRESH, null);
+
+			prjDesc = CoreModel.getDefault().getProjectDescription(project);
+			readVar = envManager.getVariable(var.getName(), prjDesc.getConfigurationById(id1), true);
+			// At this point readVar will be null -- we've locked the resource tree, so async refresh can't proceed
+			assertNull(readVar);
+
+			// Remove one variable
+			envManager.getContributedEnvironment().removeVariable(var2.getName(), prjDesc.getConfigurationById(id1));
+			// repalce one with another
+			envManager.getContributedEnvironment().addVariable(var3, prjDesc.getConfigurationById(id1));
+		} finally {
+			jm.endRule(root);
+		}
+
+		// Make everything up to date
+		project.refreshLocal(IResource.DEPTH_INFINITE, null);
+
+		// Environment should now be correct
+		readVar = envManager.getVariable(var.getName(), prjDesc.getConfigurationById(id1), true);
+		assertEquals(var, readVar);
+		readVar = envManager.getVariable(var1.getName(), prjDesc.getConfigurationById(id1), true);
+		assertNull(readVar);
+		readVar = envManager.getVariable(var2.getName(), prjDesc.getConfigurationById(id1), true);
+		assertEquals(var3, readVar);
+		
+		// Get project description should only have the persisted envvar
+		prjDesc = CoreModel.getDefault().getProjectDescription(project);
+		readVar = envManager.getVariable(var.getName(), prjDesc.getConfigurationById(id1), true);
+		assertEquals(var, readVar);
+		readVar = envManager.getVariable(var1.getName(), prjDesc.getConfigurationById(id1), true);
+		assertNull(readVar);
+		readVar = envManager.getVariable(var2.getName(), prjDesc.getConfigurationById(id1), true);
+		assertNull(readVar);
+	}
+
+}

core/org.eclipse.cdt.core.tests/suite/org/eclipse/cdt/core/suite/AutomatedIntegrationSuite.java

@@ -17,6 +17,7 @@ import junit.framework.TestSuite;
 
 import org.eclipse.cdt.core.cdescriptor.tests.CDescriptorOldTests;
 import org.eclipse.cdt.core.cdescriptor.tests.CDescriptorTests;
+import org.eclipse.cdt.core.envvar.IEnvironmentVariableManagerTests;
 import org.eclipse.cdt.core.internal.errorparsers.tests.ErrorParserTests;
 import org.eclipse.cdt.core.internal.tests.PositionTrackerTests;
 import org.eclipse.cdt.core.internal.tests.ResourceLookupTests;
@@ -58,6 +59,7 @@ public class AutomatedIntegrationSuite extends TestSuite {
 		// Add all success tests
 		suite.addTest(CDescriptorTests.suite());
 		suite.addTest(CDescriptorOldTests.suite());
+		suite.addTest(IEnvironmentVariableManagerTests.suite());
 		suite.addTest(ErrorParserTests.suite());
 		suite.addTest(ParserTestSuite.suite());
 		suite.addTest(AllCoreTests.suite());

core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/envvar/EnvironmentVariable.java

@@ -1,12 +1,13 @@
 /*******************************************************************************
- * Copyright (c) 2005, 2007 Intel Corporation and others.
+ * Copyright (c) 2005, 2009 Intel Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
  * http://www.eclipse.org/legal/epl-v10.html
  *
  * Contributors:
- * Intel Corporation - Initial API and implementation
+ *    Intel Corporation - Initial API and implementation
+ *    James Blackburn (Broadcom Corp.)
  *******************************************************************************/
 package org.eclipse.cdt.core.envvar;
 
@@ -15,7 +16,7 @@ import org.eclipse.cdt.internal.core.envvar.EnvironmentVariableManager;
 
 
 /**
- * a trivial implementation of the IBuildEnvironmentVariable
+ * A trivial implementation of {@link IEnvironmentVariable}
  * 
  * @since 3.0
  */
@@ -25,17 +26,20 @@ public class EnvironmentVariable implements IEnvironmentVariable, Cloneable {
 	protected String fDelimiter;
 	protected int fOperation;
 	
-	public EnvironmentVariable(String name, String value, int op, String delimiter){
+	public EnvironmentVariable(String name, String value, int op, String delimiter) {
 		fName = name;
 		fOperation = op;
 		fValue = value;
-		fDelimiter = delimiter;
+		if (delimiter == null)
+			fDelimiter = EnvironmentVariableManager.getDefault().getDefaultDelimiter();
+		else
+			fDelimiter = delimiter;
 	}
-	
+
-	protected EnvironmentVariable(){
+	protected EnvironmentVariable() {
-		
+		fDelimiter = EnvironmentVariableManager.getDefault().getDefaultDelimiter();
 	}
-	
+
 	public EnvironmentVariable(String name){
 		this(name,null,ENVVAR_REPLACE,null);
 	}
@@ -65,12 +69,76 @@ public class EnvironmentVariable implements IEnvironmentVariable, Cloneable {
 	}
 
 	public String getDelimiter(){
-		if (fDelimiter == null)
+		return fDelimiter;
-			return EnvironmentVariableManager.getDefault().getDefaultDelimiter();
-		else
-			return fDelimiter;
 	}
-	
+
+	@Override
+	public int hashCode() {
+		final int prime = 31;
+		int result = 1;
+		result = prime * result + ((fDelimiter == null) ? 0 : fDelimiter.hashCode());
+		result = prime * result + ((fName == null) ? 0 : fName.hashCode());
+		result = prime * result + fOperation;
+		result = prime * result + ((fValue == null) ? 0 : fValue.hashCode());
+		return result;
+	}
+
+	@Override
+	public boolean equals(Object obj) {
+		if (obj == this)
+			return true;
+		if (!(obj instanceof IEnvironmentVariable))
+			return super.equals(obj);
+		IEnvironmentVariable other = (IEnvironmentVariable)obj;
+		if (!equals(fName, other.getName()))
+			return false;
+		if (!equals(fValue, other.getValue()))
+			return false;
+		if (!equals(fDelimiter, other.getDelimiter()))
+			return false;
+		if (fOperation != other.getOperation())
+			return false;
+		return true;
+	}
+
+	// Helper method to check equality of two objects
+	private boolean equals(Object obj1, Object obj2) {
+		if (obj1 == obj2)
+			return true;
+		else if (obj1 == null)
+			return false;
+		else
+			return obj1.equals(obj2);
+	}
+
+	@Override
+	public String toString() {
+		StringBuilder sb = new StringBuilder();
+		if (fName != null)
+			sb.append(fName);
+		if (fValue != null)
+			sb.append("=").append(fValue); //$NON-NLS-1$
+		sb.append(" ").append(fDelimiter); //$NON-NLS-1$
+		switch (fOperation) {
+			case ENVVAR_REPLACE:
+				sb.append(" [REPL]"); //$NON-NLS-1$
+				break;
+			case ENVVAR_REMOVE:
+				sb.append(" [REM]"); //$NON-NLS-1$
+				break;
+			case ENVVAR_PREPEND:
+				sb.append(" [PREP]"); //$NON-NLS-1$
+				break;
+			case ENVVAR_APPEND:
+				sb.append(" [APP]"); //$NON-NLS-1$
+				break;
+			default:
+				sb.append(" [NONE]"); //$NON-NLS-1$
+				break;
+		}
+		return sb.toString();
+	}
+
 	@Override
 	public Object clone(){
 		try {

core/org.eclipse.cdt.core/src/org/eclipse/cdt/internal/core/envvar/UserDefinedEnvironmentSupplier.java

@@ -1,12 +1,13 @@
 /*******************************************************************************
- * Copyright (c) 2005, 2007 Intel Corporation and others.
+ * Copyright (c) 2005, 2009 Intel Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
  * http://www.eclipse.org/legal/epl-v10.html
  *
  * Contributors:
- * Intel Corporation - Initial API and implementation
+ *    Intel Corporation - Initial API and implementation
+ *    James Blackburn (Broadcom Corp.)
  *******************************************************************************/
 package org.eclipse.cdt.internal.core.envvar;
 
@@ -151,6 +152,7 @@ public class UserDefinedEnvironmentSupplier extends
 					settings.setEnvironment(env);
 				}
 			} catch (CoreException e) {
+				CCorePlugin.log(e);
 			}
 		}
 		else if(context instanceof IWorkspace || context == null){
@@ -161,22 +163,20 @@ public class UserDefinedEnvironmentSupplier extends
 		
 		return env;
 	}
-	
+
 	@Override
 	protected ISerializeInfo getSerializeInfo(Object context){
 		ISerializeInfo serializeInfo = null;
-		
+
 		if(context instanceof ICConfigurationDescription){
-			ICConfigurationDescription cfg = (ICConfigurationDescription)context;
+			final ICConfigurationDescription cfg = (ICConfigurationDescription)context;
-			
-			final Preferences prefs = getConfigurationNode(cfg.getProjectDescription());
 			final String name = cfg.getId();
-			if(prefs != null && name != null)
+			if(name != null)
 				serializeInfo = new ISerializeInfo(){
 				public Preferences getNode(){
-					return prefs;
+					return getConfigurationNode(cfg.getProjectDescription());
 				}
-				
+
 				public String getPrefName(){
 					return name;
 				}

core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/envvar/StorableEnvironment.java

@@ -1,16 +1,16 @@
 /*******************************************************************************
- * Copyright (c) 2005, 2008 Intel Corporation and others.
+ * Copyright (c) 2005, 2009 Intel Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
  * http://www.eclipse.org/legal/epl-v10.html
  *
  * Contributors:
- * Intel Corporation - Initial API and implementation
+ *    Intel Corporation - Initial API and implementation
+ *    James Blackburn (Broadcom Corp.)
  *******************************************************************************/
 package org.eclipse.cdt.utils.envvar;
 
-import java.util.Collection;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
@@ -20,31 +20,59 @@ import org.eclipse.cdt.core.envvar.IEnvironmentVariableManager;
 import org.eclipse.cdt.core.settings.model.ICStorageElement;
 import org.eclipse.cdt.internal.core.envvar.EnvironmentVariableManager;
 import org.eclipse.cdt.internal.core.settings.model.ExceptionFactory;
+import org.eclipse.cdt.utils.envvar.StorableEnvironmentLoader.ISerializeInfo;
 
 /**
  * This class represents the set of environment variables that could be loaded
  * and stored in XML
  * 
  * @since 3.0
- *
  */
 public class StorableEnvironment /*implements Cloneable*/{
 	public static final String ENVIRONMENT_ELEMENT_NAME = "environment"; //$NON-NLS-1$
 	private static final String ATTRIBUTE_APPEND = "append";  //$NON-NLS-1$
 	private static final String ATTRIBUTE_APPEND_CONTRIBUTED = "appendContributed";  //$NON-NLS-1$
 	private static final boolean DEFAULT_APPEND = true;
+	/** The map of in-flight environment variables */
 	private HashMap<String, IEnvironmentVariable> fVariables;
+	/** Map of 'deleted' variables (which shouldn't be updated by a backing store change) */
+	private HashMap<String, IEnvironmentVariable> fDeletedVariables;
 	private boolean fIsDirty = false;
 	private boolean fIsChanged = false;
-	private boolean fIsReadOnly;
+	private final boolean fIsReadOnly;
 	private boolean fAppend = DEFAULT_APPEND;
 	private boolean fAppendContributedEnv = DEFAULT_APPEND;
-	
+
+
+	// State to manage and handle external changes to the environment
+
+	/** A cache copy of the environment as stored in the {@link ISerializeInfo} 
+	 *  used to work-out whether the cachedStorableEnvironment map needs refreshing */
+	private String fCachedSerialEnvString;
+	/** Map of Environment as loaded from the {@link ISerializeInfo} */
+	private HashMap<String, IEnvironmentVariable> fCachedSerialEnv;
+	private ISerializeInfo fSerialEnv;
+	// State to track whether API users have changed these boolean values
+	private boolean fAppendChanged = false;
+	private boolean fAppendContributedChanged = false;
+
+
+	/** 
+	 * @return the live {@link IEnvironmentVariable} map
+	 */
 	private Map<String, IEnvironmentVariable> getMap(){
 		if(fVariables == null)
 			fVariables = new HashMap<String, IEnvironmentVariable>();
 		return fVariables;
 	}
+	/** 
+	 * @return the live removed {@link IEnvironmentVariable} map
+	 */
+	private Map<String, IEnvironmentVariable> getDeletedMap(){
+		if(fDeletedVariables == null)
+			fDeletedVariables = new HashMap<String, IEnvironmentVariable>();
+		return fDeletedVariables;
+	}
 
 	public StorableEnvironment(IEnvironmentVariable variables[], boolean isReadOnly) {
 		setVariales(variables);
@@ -61,8 +89,16 @@ public class StorableEnvironment /*implements Cloneable*/{
 			final HashMap<String, IEnvironmentVariable> clone = (HashMap<String, IEnvironmentVariable>)env.fVariables.clone();
 			fVariables = clone;
 		}
+		if(env.fDeletedVariables != null) {
+			@SuppressWarnings("unchecked")
+			final HashMap<String, IEnvironmentVariable> clone = (HashMap<String, IEnvironmentVariable>)env.fDeletedVariables.clone();
+			fDeletedVariables = clone;
+		}
+		fSerialEnv = env.fSerialEnv;
 		fAppend = env.fAppend;
+		fAppendChanged = env.fAppendChanged;
 		fAppendContributedEnv = env.fAppendContributedEnv;
+		fAppendContributedChanged = env.fAppendContributedChanged;
 		fIsReadOnly = isReadOnly;
 		fIsDirty = env.isDirty();
 	}
@@ -71,13 +107,63 @@ public class StorableEnvironment /*implements Cloneable*/{
 		load(element);
 		fIsReadOnly = isReadOnly;
 	}
-	
+
+	/**
+	 * Create a StorableEnvironment backed by this ISerializeInfo.
+	 * 
+	 * @param serializeInfo
+	 * @since 5.2
+	 */
+	StorableEnvironment(ISerializeInfo serializeInfo, boolean isReadOnly) {
+		fIsReadOnly = isReadOnly;
+		fSerialEnv = serializeInfo;
+
+		// Update the cached state
+		checkBackingSerializeInfo();			
+	}
+
+	/**
+	 * Check and update the state of the backing {@link ISerializeInfo} cache
+	 */
+	private void checkBackingSerializeInfo() {
+		String envString = StorableEnvironmentLoader.loadPreferenceNode(fSerialEnv);
+
+		// Has anything changed?
+		if (envString == null || envString.equals(fCachedSerialEnvString))
+			return;
+		fCachedSerialEnvString = envString;
+
+		ICStorageElement element = StorableEnvironmentLoader.environmentStorageFromString(fCachedSerialEnvString);
+		if (element == null)
+			return;
+
+		// Now update the cached environment
+		if (fCachedSerialEnv == null)
+			fCachedSerialEnv = new HashMap<String, IEnvironmentVariable>();
+		else
+			fCachedSerialEnv.clear();
+
+		for (ICStorageElement child : element.getChildren())
+			if (child.getName().equals(StorableEnvVar.VARIABLE_ELEMENT_NAME))
+				addVariable(fCachedSerialEnv, new StorableEnvVar(child));
+
+		// If user hasn't changed fAppend or fAppend Contributed, then update
+		if (!fAppendChanged) {
+			String append = element.getAttribute(ATTRIBUTE_APPEND);
+			fAppend = append != null ? Boolean.valueOf(append).booleanValue() : DEFAULT_APPEND;
+		}
+		if (!fAppendContributedChanged) {
+			String append = element.getAttribute(ATTRIBUTE_APPEND_CONTRIBUTED);
+			fAppendContributedEnv = append != null ? Boolean.valueOf(append).booleanValue()	: DEFAULT_APPEND;
+		}
+	}
+
 	private void load(ICStorageElement element){
 		ICStorageElement children[] = element.getChildren();
 		for (int i = 0; i < children.length; ++i) {
 			ICStorageElement node = children[i];
 			if (node.getName().equals(StorableEnvVar.VARIABLE_ELEMENT_NAME)) {
-				addVariable(new StorableEnvVar(node));
+				addVariable(getMap(), new StorableEnvVar(node));
 			}
 		}
 		
@@ -108,15 +194,20 @@ public class StorableEnvironment /*implements Cloneable*/{
 		fIsDirty = false;
 	}
 
-	private void addVariable(IEnvironmentVariable var){
+	/**
+	 * Add the environment variable to the map
+	 * @param map
+	 * @param var
+	 */
+	private void addVariable(Map<String, IEnvironmentVariable> map, IEnvironmentVariable var){
 		String name = var.getName();
 		if(name == null)
 			return;
 		IEnvironmentVariableManager provider = EnvironmentVariableManager.getDefault();
 		if(!provider.isVariableCaseSensitive())
 			name = name.toUpperCase();
-		
+
-		getMap().put(name,var);
+		map.put(name,var);
 	}
 
 	public IEnvironmentVariable createVariable(String name, String value, int op, String delimiter){
@@ -129,7 +220,10 @@ public class StorableEnvironment /*implements Cloneable*/{
 		IEnvironmentVariable var = checkVariable(name,value,op,delimiter);
 		if(var == null){
 			var = new StorableEnvVar(name, value, op, delimiter);
-			addVariable(var);
+			addVariable(getMap(), var);
+			// Variable added, ensure it's not in the removed set
+			if (fDeletedVariables != null)
+				fDeletedVariables.remove(EnvironmentVariableManager.getDefault().isVariableCaseSensitive() ? name : name.toUpperCase());
 			fIsDirty = true;
 			fIsChanged = true;
 		}
@@ -210,16 +304,34 @@ public class StorableEnvironment /*implements Cloneable*/{
 		fIsChanged = changed;
 	}
 
+	/**
+	 * @param name
+	 * @return the environment variable with the given name, or null
+	 */
 	public IEnvironmentVariable getVariable(String name){
 		if(name == null || "".equals(name = name.trim())) //$NON-NLS-1$
 			return null;
 		IEnvironmentVariableManager provider = EnvironmentVariableManager.getDefault();
 		if(!provider.isVariableCaseSensitive())
 			name = name.toUpperCase();
-		
+
-		return getMap().get(name);
+		IEnvironmentVariable var = getMap().get(name);
+		if (var != null)
+			return var;
+
+		if (fDeletedVariables != null && fDeletedVariables.containsKey(name))
+			return null;
+
+		checkBackingSerializeInfo();
+		if (fCachedSerialEnv != null)
+			return fCachedSerialEnv.get(name);
+		return null;
 	}
 	
+	/**
+	 * Set the enviornment variables in this {@link StorableEnvironment}
+	 * @param vars
+	 */
 	public void setVariales(IEnvironmentVariable vars[]){
 		if(fIsReadOnly)
 			throw ExceptionFactory.createIsReadOnlyException();
@@ -252,13 +364,23 @@ public class StorableEnvironment /*implements Cloneable*/{
 					vars[i].getOperation(),
 					vars[i].getDelimiter());
 	}
-	
+
 	public IEnvironmentVariable[] getVariables(){
-		Collection<IEnvironmentVariable> vars = getMap().values();
+		checkBackingSerializeInfo();
-		
+		// Get all the environment from the backing store first
-		return vars.toArray(new IEnvironmentVariable[vars.size()]);
+		Map<String, IEnvironmentVariable> vars = new HashMap<String, IEnvironmentVariable>();
+		if (fCachedSerialEnv != null)
+			vars.putAll(fCachedSerialEnv);
+		if (fDeletedVariables != null)
+			for (String name : fDeletedVariables.keySet())
+				vars.remove(name);
+
+		// Now overwrite with the live variables set, and return
+		vars.putAll(getMap());
+
+		return vars.values().toArray(new IEnvironmentVariable[vars.size()]);
 	}
-	
+
 	public IEnvironmentVariable deleteVariable(String name){
 		if(fIsReadOnly)
 			throw ExceptionFactory.createIsReadOnlyException();
@@ -269,6 +391,7 @@ public class StorableEnvironment /*implements Cloneable*/{
 			name = name.toUpperCase();
 
 		IEnvironmentVariable var = getMap().remove(name);
+		getDeletedMap().put(name, var);
 		if(var != null){
 			fIsDirty = true;
 			fIsChanged = true;
@@ -284,6 +407,7 @@ public class StorableEnvironment /*implements Cloneable*/{
 		if(map.size() > 0){
 			fIsDirty = true;
 			fIsChanged = true;
+			getDeletedMap().putAll(map);
 			map.clear();
 			return true;
 		}
@@ -307,6 +431,7 @@ public class StorableEnvironment /*implements Cloneable*/{
 			throw ExceptionFactory.createIsReadOnlyException();
 
 		fAppend = append;
+		fAppendChanged = true;
 		fIsDirty = true;
 	}
 
@@ -322,6 +447,7 @@ public class StorableEnvironment /*implements Cloneable*/{
 			throw ExceptionFactory.createIsReadOnlyException();
 		
 		fAppendContributedEnv = append;
+		fAppendContributedChanged = true;
 		fIsDirty = true;
 	}
 	

core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/envvar/StorableEnvironmentLoader.java

@@ -6,14 +6,14 @@
  * http://www.eclipse.org/legal/epl-v10.html
  *
  * Contributors:
- * Intel Corporation - Initial API and implementation
+ *    Intel Corporation - Initial API and implementation
+ *    James Blackburn (Broadcom Corp.)
  *******************************************************************************/
 package org.eclipse.cdt.utils.envvar;
 
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
-import java.io.InputStream;
 import java.io.UnsupportedEncodingException;
 
 import javax.xml.parsers.DocumentBuilder;
@@ -28,6 +28,7 @@ import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
 
 import org.eclipse.cdt.core.CCorePlugin;
+import org.eclipse.cdt.core.settings.model.ICStorageElement;
 import org.eclipse.cdt.internal.core.settings.model.xml.XmlStorageElement;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IStatus;
@@ -55,8 +56,14 @@ public abstract class StorableEnvironmentLoader {
 	 * @noimplement This interface is not intended to be implemented by clients.
 	 */
 	public interface ISerializeInfo{
+		/**
+		 * @return the Preferences Node into which environment should be (de) serialized
+		 */
 		Preferences getNode();
-		
+
+		/**
+		 * @return the key in the preference node to use for loading preferences 
+		 */
 		String getPrefName();
 	}
 
@@ -67,18 +74,23 @@ public abstract class StorableEnvironmentLoader {
 	 */
 	protected abstract ISerializeInfo getSerializeInfo(Object context);
 
-	/*
+	/**
-	 * loads the stored environment for the given context 
+	 * Loads the environment from the context's {@link ISerializeInfo}.
+	 * 
+	 * NB the environment in the {@link ISerializeInfo} need not be available
+	 * yet. The {@link ISerializeInfo} may be held by the {@link StorableEnvironment} 
+	 * to pick up any external changes in the environment.
+	 * 
+	 * @param context
+	 * @param readOnly
+	 * @return StorableEnvironment
 	 */
 	protected StorableEnvironment loadEnvironment(Object context, boolean readOnly){
 		ISerializeInfo serializeInfo = getSerializeInfo(context);
 		if(serializeInfo == null)
 			return null;
-		
+
-		InputStream stream = loadInputStream(serializeInfo.getNode(),serializeInfo.getPrefName());
+		return new StorableEnvironment(serializeInfo, readOnly);
-		if(stream == null)
-			return new StorableEnvironment(readOnly);
-		return loadEnvironmentFromStream(stream, readOnly);
 	}
 	
 	/*
@@ -95,37 +107,41 @@ public abstract class StorableEnvironmentLoader {
 		ByteArrayOutputStream stream = storeEnvironmentToStream(env);
 		if(stream == null)
 			return;
-		storeOutputStream(stream,serializeInfo.getNode(),serializeInfo.getPrefName(), flush);
+		storeOutputStream(stream,serializeInfo.getNode(), serializeInfo.getPrefName(), flush);
 		
 		env.setDirty(false);
 	}
 	
-	private StorableEnvironment loadEnvironmentFromStream(InputStream stream, boolean readOnly){
+	/**
+	 * @param env String representing the encoded environment
+	 * @return ICStorageElement tree from the passed in InputStream
+	 *                          or null on failure
+	 */
+	static ICStorageElement environmentStorageFromString(String env) {
+		if (env == null)
+			return null;
 		try{
 			DocumentBuilder parser = DocumentBuilderFactory.newInstance().newDocumentBuilder();
-			InputSource inputSource = new InputSource(stream);
+			InputSource inputSource = new InputSource(new ByteArrayInputStream(env.getBytes()));
 			Document document = parser.parse(inputSource);
 			Element el = document.getDocumentElement();
 			XmlStorageElement rootElement = new XmlStorageElement(el);
-			
+
 			if(!StorableEnvironment.ENVIRONMENT_ELEMENT_NAME.equals(rootElement.getName()))
 				return null;
-			
+
-			return new StorableEnvironment(rootElement, readOnly);
+			return rootElement;
 		}
 		catch(ParserConfigurationException e){
-			
+			CCorePlugin.log(e);
+		} catch(SAXException e) {
+			CCorePlugin.log(e);			
+		} catch(IOException e) {
+			CCorePlugin.log(e);
 		}
-		catch(SAXException e){
-			
-		}
-		catch(IOException e){
-			
-		}
-		
 		return null;
 	}
-	
+
 	private ByteArrayOutputStream storeEnvironmentToStream(StorableEnvironment env) throws CoreException{
 		try{
 			DocumentBuilderFactory factory= DocumentBuilderFactory.newInstance();
@@ -171,23 +187,31 @@ public abstract class StorableEnvironmentLoader {
 					e));
 		}
 	}
-	
+
-	private InputStream loadInputStream(Preferences node, String key){
+	/**
+	 * @return String value stored in the node or null if no such value exists.
+	 */
+	static String loadPreferenceNode(ISerializeInfo serializeInfo) {
+		if (serializeInfo == null)
+			return null;
+		return loadPreferenceNode(serializeInfo.getNode(), serializeInfo.getPrefName());
+	}
+
+	/**
+	 * Returns the value stored in a Preferences node
+	 * @param node Preferences node
+	 * @param key
+	 * @return String value stored in the node or null if no such value exists.
+	 */
+	static String loadPreferenceNode(Preferences node, String key){
 		if(node == null || key == null)
 			return null;
 		
-		String value = node.get(key,null);
+		String value = node.get(key, null);
 		if(value == null || value.length() == 0)
 			return null;
 		
-		byte[] bytes;
+		return value;
-		try {
-			bytes = value.getBytes("UTF-8"); //$NON-NLS-1$
-		} catch (UnsupportedEncodingException e) {
-			bytes = value.getBytes();
-		}
-		
-		return new ByteArrayInputStream(bytes);
 	}
 	
 	private void storeOutputStream(ByteArrayOutputStream stream, Preferences node, String key, boolean flush) throws CoreException{