Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [cdt-dev] Variable name of USER:CONFIG environment goes to uppercase

Jonah

I'm back to trying to commit again.

The change for the basic functionality was easy and very local to StorableCdtVariables (I attached the fixed version)

I signed the contributor agreement (Still not happy with it but what can I do)

When following these steps https://wiki.eclipse.org/CDT/contributing

I jumped to the gerrit steps at https://wiki.eclipse.org/CDT/git#Using_Gerrit_for_CDT

I think I succeeded in adding gerrit

When I try to push to gerrit I get "no authorisation"; which seems fine to me

When I try to commit I get

Not sure what to do now.

Best regards

Jantje



Op 28/05/2020 om 15:10 schreef Jan Baeyens:

Jonah

Though it is not my preferred solution, I'll try to get it to work like windows does with the capitalisation in place.

Jantje

Op 27/05/2020 om 16:07 schreef Jonah Graham:
Hi Jantje,

Your idea sounds on the correct track - but you can't remove, only change Windows specific code. In particular the thing most likely to break if you start changing case sensitivity of environment variables is Path. So the more complete/correct solution is to implement the same case insensitivity in CDT that Windows does in practice, but as Windows does, preserve the incoming case. The preserving the case is the new part to this equation. 

The final thing to keep in mind is that other tools on Windows follow the same conventions that CDT does, in particular msys also uppercases all environment variables, so you may find that you end up with other places that environment variables have case changed, even if you carefully preserve them in CDT. Some tools (like mingw make I believe) uppercase the PATH environment variable, but preserve case for other variables.

HTH,
Jonah


~~~
Jonah Graham
Kichwa Coders
www.kichwacoders.com


On Sun, 24 May 2020 at 19:00, Jan Baeyens <jan@xxxxxxxxxx> wrote:

Jonah

I need to decide on working around this issue or fixing it in CDT lets say in a couple of weeks. As I know it takes time to take difficult decisions I would like to point this out now so you can think about this before the decision needs to be taken.


For me working around this issue in Sloeber is going to be painful and so is fixing this in CDT. The fix in CDT will benefit more people so has my preference.

For the second I'll need your approval for the solution. I see 3 ways to fix this in CDT

1) remove the windows specific code (which is kind of a revert back to 2010 according to this issue https://bugs.eclipse.org/bugs/show_bug.cgi?id=319676)

2) Let the user set a switch

3) 1+alert the user when a new environment variable is created that is only case different from a existing one.

My preference goes to 3 (though I might not be able to do this due to lack of swt skills) because the current implementation is ..... crap.

FI in sloeber I use

${JANTJE.MAKE_LOCATION}make

in project properties->C/C++build build command

This expands to

C:\\sloeber\\arduinoPlugin\\tools\\make\\make

If I change this to (using c instead of C)

${JANTJE.MAKE_LOcATION}make

This expands to

make

In other words the current solution is still case sensitive.

The only thing that is "fixed" right now is that when you have 2 environment variables in CDT that are only different at the level of casing one of these variables will not be silently overwritten by the other at the OS level but silently overwritten at the CDT level. (!!!read this again!!!)

This at the cost of enforcing all variable name usages in CDT to be uppercase.
For teams that share their settings: when the project is shared between multiple OS(of which one is windows) these rules will apply to all OS.

And this is where option 3 comes in. If a user creates a environment variable that is only case different from a existing variable we could warn them that windows os doesn't differentiate between them and therefore using this variable name may not be a good idea.

As I really would like this seen fixed in CDT. I hope you have some time to think this over.

Best regards

Jantje

PS Reality has proven me wrong so many times before; so I'm really reluctant to say this.
IMHO there will not be regression issues because
1)in the current implementation everything in CDT is case sensitive and everything remains case sensitive.
2)in the current implementation everything in windows is case insensitive and everything remains case insensitive.
3)Currently CDT users on windows will only have all uppercase environment variables.
4) When they reference these in CDT these references must match case and thus be upper case and that will remain so.
5) and when they use them in windows the case is and will be unimportant. 
So nothing changes to the existing cases. We only extent the number of possible cases which should not lead to regression.

Op 22/05/2020 om 19:58 schreef Jan Baeyens:

Jonah

Thanks for the feedback. I fear this won't be a quick fix :-(

I'm not sure the internal uppercasing (based on the OS) is a good idea. Actually I think it is a bad idea.

For instance if you use the "expand environment variables in makefile" the environment variables will never hit the OS. In this use case there is no need to take OS specific rules into account.

What makes this implementation worse is that CDT behaves differently between OS'es without any reason. Which makes support a nightmare.

What I think would be a better approach is:
Somewhere deep down in CDT/Eclipse there is code to "Load environment variables in a console" (what CDT needs to do before starting the makefile when the option "expand environment variables in makefile"  is disabled)
I think it would be better to have this code warn/error about "variants of variable names that will not be taken into account by the OS" (given 15 knots his email I would say warn and allow to ignore)

Best regards

Jantje

Op 22/05/2020 om 17:49 schreef Jonah Graham:
There are a number of cases that it is supposed to workaround, main one that comes to mind is that Path vs PATH on Windows refers to the same environment variable - so it seems that the assumption is on windows the case of the environment variable was not supposed to matter and so normalizing them to all uppercase made sure that Maps in java worked as expected.

There are a few places in the code that do the uppercasing:
org.eclipse.cdt.internal.core.envvar.EnvVarCollector.add(IEnvironmentVariable[], IEnvironmentContextInfo, int, ICoreEnvironmentVariableSupplier)
org.eclipse.cdt.core.CommandLauncher.parseEnvironment(String[])
org.eclipse.cdt.utils.envvar.EnvVarOperationProcessor.normalizeName(String)
org.eclipse.cdt.utils.envvar.StorableEnvironment.getNameForMap(String)
org.eclipse.cdt.utils.spawner.EnvironmentReader.init()

So either we need to do the uppercasing in more places - or fix all the places so that uses the uppercase workaround and probably change it to a case insensitive keyed map, such as new TreeMap<>(String.CASE_INSENSITIVE_ORDER);

HTH,
Jonah


~~~
Jonah Graham
Kichwa Coders
www.kichwacoders.com


On Fri, 22 May 2020 at 10:30, Jan Baeyens <jan@xxxxxxxxxx> wrote:

Thanks for the info

I tried this on Linux and indeed no conversion to uppercase on linux. So this is a windows thing.

This means that the conversion to uppercase must be inside an os check.

I'm really curious what this is supposed to workaround.

Best regards

Jantje

Op 22/05/2020 om 2:06 schreef Jonah Graham:
I can look into it further (when at my desk) but I believe the problem is a workaround for environment variables being not case sensitive on windows. What you mention rings a bell, so I'll try to find the bit of code in CDT that is doing it.

Jonah

On Thu., May 21, 2020, 19:29 Jan Baeyens, <jan@xxxxxxxxxx> wrote:
Hi

When I add an environment variable to a managed build CDT project in
project properties->C/C++ build->environment the variable name is
converted to uppercase.

If for instance you add the variable named "test" with the value
"testvalue" it will look as if "test" is not converting to upper case
but when you close the project properties and reopen the variable name
is converted to "TEST".

This is not done for environment variables of origin USER:PREFS

Because CDT is case sensitive when resolving variables this is a bit of
a problem for me and as you can see in this video "strange things happen"

https://youtu.be/Cqs7V46ffvM

So I'm wondering:

1) Is this a known feature?

2) can I turn this uppercasing off?

3) anyone can provide me with some background info?


Background info of what I'm working on right now.

ESP32 is working on a new release and they want to use nested variables. FI:

build.mcu=esp32s2
compiler.cpp.flags.esp32s2=-mlongcalls -ffunction-section
compiler.cpp.flags={compiler.cpp.flags.{build.mcu}}

resulting in

compiler.cpp.flags=-mlongcalls -ffunction-section

Though the comments in CDT code state this should not work it works
fine. But the (timing of the) uppercasing combined with the case
sensitivity causes a problem.

BUILD.MCU=esp32s2
COMPILER.CPP.FLAGS.ESP32S2=-mlongcalls -ffunction-section
COMPILER.CPP.FLAGS={COMPILER.CPP.FLAGS.{BUILD.MCU}}

gives

COMPILER.CPP.FLAGS={COMPILER.CPP.FLAGS.esp32s2}

Which is not resolved.

There is no obvious way to know that the value of BUILD.MCU should be
uppercased. Moreover BUILD.MCU is used in cases where the value can not
be uppercased.



_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/cdt-dev

_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/cdt-dev
_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/cdt-dev

_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/cdt-dev


_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/cdt-dev
_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/cdt-dev

_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/cdt-dev


_______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/cdt-dev

/*******************************************************************************
 * Copyright (c) 2005, 2012 Intel Corporation and others.
 *
 * This program and the accompanying materials
 * are made available under the terms of the Eclipse Public License 2.0
 * which accompanies this distribution, and is available at
 * https://www.eclipse.org/legal/epl-2.0/
 *
 * SPDX-License-Identifier: EPL-2.0
 *
 * Contributors:
 * Intel Corporation - Initial API and implementation
 *******************************************************************************/
package org.eclipse.cdt.internal.core.cdtvariables;

import java.util.Collection;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;

import org.eclipse.cdt.core.cdtvariables.CdtVariableException;
import org.eclipse.cdt.core.cdtvariables.ICdtVariable;
import org.eclipse.cdt.core.cdtvariables.IStorableCdtVariables;
import org.eclipse.cdt.core.settings.model.ICStorageElement;
import org.eclipse.cdt.internal.core.cdtvariables.UserDefinedVariableSupplier.VarKey;
import org.eclipse.cdt.internal.core.settings.model.ExceptionFactory;
import org.eclipse.cdt.utils.cdtvariables.CdtVariableResolver;
import org.eclipse.core.runtime.Platform;

/**
 * This class represents the set of Build Macros that could be loaded
 * and stored in XML
 *
 * On all OS (except windows) variables are treated case sensitive.
 *
 * @since 3.0
 *
 */
public class StorableCdtVariables implements IStorableCdtVariables {
	public static final String MACROS_ELEMENT_NAME = "macros"; //$NON-NLS-1$
	private TreeMap<String, ICdtVariable> fMacros;
	private boolean fIsDirty = false;
	private boolean fIsChanged = false;
	private boolean fIsReadOnly;
	public static boolean isCaseSensitive = !Platform.getOS().equals(Platform.OS_WIN32);

	private TreeMap<String, ICdtVariable> getMap() {
		if (fMacros == null)
			if (isCaseSensitive) {
				fMacros = new TreeMap<>();
			} else {
				fMacros = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
			}

		return fMacros;
	}

	public StorableCdtVariables(boolean readOnly) {
		fIsReadOnly = readOnly;
	}

	@SuppressWarnings("unchecked")
	public StorableCdtVariables(StorableCdtVariables base, boolean readOnly) {
		fMacros = (TreeMap<String, ICdtVariable>) base.getMap().clone();
		fIsReadOnly = readOnly;
	}

	public StorableCdtVariables(ICdtVariable vars[], boolean readOnly) {
		if (isCaseSensitive) {
			fMacros = new TreeMap<>();
		} else {
			fMacros = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
		}

		for (ICdtVariable var : vars) {
			addMacro(var);
		}
		fIsReadOnly = readOnly;
	}

	public StorableCdtVariables(ICStorageElement element, boolean readOnly) {
		load(element);
		fIsReadOnly = readOnly;
	}

	private void load(ICStorageElement element) {
		//		fExpandInMakefile = TRUE.equals(element.getAttribute(EXPAND_ENVIRONMENT_MACROS));

		ICStorageElement nodeList[] = element.getChildren();
		for (int i = 0; i < nodeList.length; ++i) {
			ICStorageElement node = nodeList[i];
			String name = node.getName();
			if (StorableCdtVariable.STRING_MACRO_ELEMENT_NAME.equals(name)) {
				addMacro(new StorableCdtVariable(node));
			} else if (StorableCdtVariable.STRINGLIST_MACRO_ELEMENT_NAME.equals(name)) {
				addMacro(new StorableCdtVariable(node));
			}
		}
		fIsDirty = false;
		fIsChanged = false;
	}

	public void serialize(ICStorageElement element) {
		if (fMacros != null) {
			for (ICdtVariable v : fMacros.values()) {
				StorableCdtVariable macro = (StorableCdtVariable) v;
				ICStorageElement macroEl;
				if (CdtVariableResolver.isStringListVariable(macro.getValueType()))
					macroEl = element.createChild(StorableCdtVariable.STRINGLIST_MACRO_ELEMENT_NAME);
				else
					macroEl = element.createChild(StorableCdtVariable.STRING_MACRO_ELEMENT_NAME);
				macro.serialize(macroEl);
			}
		}
		fIsDirty = false;
	}

	private void addMacro(ICdtVariable macro) {
		String name = macro.getName();
		if (name == null)
			return;

		getMap().put(name, macro);
	}

	@Override
	public ICdtVariable createMacro(String name, int type, String value) {
		if (name == null || "".equals(name = name.trim()) || CdtVariableResolver.isStringListVariable(type)) //$NON-NLS-1$
			return null;

		ICdtVariable macro = checkMacro(name, type, value);
		if (macro == null) {
			macro = new StorableCdtVariable(name, type, value);
			addMacro(macro);
			fIsDirty = true;
			fIsChanged = true;
		}
		return macro;
	}

	public ICdtVariable checkMacro(String name, int type, String value) {
		if (fIsReadOnly)
			throw ExceptionFactory.createIsReadOnlyException();
		ICdtVariable macro = getMacro(name);
		if (macro != null) {
			if (macro.getName().equals(name) && macro.getValueType() == type) {
				try {
					String val = macro.getStringValue();
					if ((val != null && val.equals(value)) || val == value) {
						return macro;
					}
				} catch (CdtVariableException e) {
				}
			}
		}
		return null;
	}

	public ICdtVariable checkMacro(String name, int type, String value[]) {
		if (fIsReadOnly)
			throw ExceptionFactory.createIsReadOnlyException();
		ICdtVariable macro = getMacro(name);
		if (macro != null) {
			if (macro.getName().equals(name) && macro.getValueType() == type) {
				try {
					String val[] = macro.getStringListValue();
					if (val != null) {
						if (value != null && value.length == val.length) {
							int i;
							for (i = 0; i < val.length; i++) {
								if (!value[i].equals(val[i]))
									break;
							}
							if (i == value.length)
								return macro;
						}
					} else if (value == val) {
						return macro;
					}
				} catch (CdtVariableException e) {
				}
			}
		}
		return null;
	}

	/**
	 * sets the storable macros to hold the geven number of macros
	 * all macros that are present in the store but not included in the given array
	 * will be removed
	 */
	public void setMacros(ICdtVariable macros[]) {
		if (fIsReadOnly)
			throw ExceptionFactory.createIsReadOnlyException();
		if (macros == null || macros.length == 0)
			deleteAll();
		else {
			if (getMap().size() != 0) {
				Set<String> existing = new HashSet<>();
				Set<String> macroNames = new HashSet<>();

				for (ICdtVariable m : getMap().values()) {
					existing.add(m.getName());
				}

				for (ICdtVariable m : macros) {
					macroNames.add(m.getName());
				}

				for (String name : existing) {
					if (!macroNames.contains(name)) {
						deleteMacro(name);
					}
				}
			}
			createMacros(macros);
		}
	}

	@Override
	public void createMacros(ICdtVariable macros[]) {
		if (fIsReadOnly)
			throw ExceptionFactory.createIsReadOnlyException();
		for (ICdtVariable macro : macros) {
			createMacro(macro);
		}
	}

	@Override
	public boolean isEmpty() {
		return fMacros == null || fMacros.isEmpty();
	}

	@Override
	public ICdtVariable createMacro(ICdtVariable copy) {
		if (fIsReadOnly)
			throw ExceptionFactory.createIsReadOnlyException();
		String name = copy.getName();
		if (name == null || "".equals(name = name.trim())) //$NON-NLS-1$
			return null;

		int type = copy.getValueType();

		ICdtVariable macro = null;
		try {
			if (CdtVariableResolver.isStringListVariable(type)) {
				String value[] = copy.getStringListValue();
				macro = checkMacro(name, type, value);
				if (macro == null) {
					macro = new StorableCdtVariable(name, type, value);
					addMacro(macro);
					fIsDirty = true;
					fIsChanged = true;
				}
			} else {
				String value = copy.getStringValue();
				macro = checkMacro(name, type, value);
				if (macro == null) {
					macro = new StorableCdtVariable(name, type, value);
					addMacro(macro);
					fIsDirty = true;
					fIsChanged = true;
				}
			}

		} catch (CdtVariableException e) {
		}
		return macro;
	}

	@Override
	public ICdtVariable createMacro(String name, int type, String value[]) {
		if (name == null || "".equals(name = name.trim()) || !CdtVariableResolver.isStringListVariable(type)) //$NON-NLS-1$
			return null;

		ICdtVariable macro = checkMacro(name, type, value);
		if (macro == null) {
			macro = new StorableCdtVariable(name, type, value);
			addMacro(macro);
			fIsDirty = true;
			fIsChanged = true;
		}
		return macro;
	}

	/**
	 * Returns the "dirty" state for this set of macros.
	 * If the dirty state is <code>true</code>, that means that the macros
	 * is out of synch with the repository and the macros need to be serialized.
	 * <br><br>
	 * The dirty state is automatically set to <code>false</code> when the macros are serialized
	 * by calling the serialize() method
	 * @return boolean
	 */
	public boolean isDirty() {
		return fIsDirty;
	}

	/**
	 * sets the "dirty" state for this set of macros.
	 * @see org.eclipse.cdt.internal.core.cdtvariables.StorableCdtVariables#isDirty()
	 * @param dirty represents the new state
	 */
	public void setDirty(boolean dirty) {
		fIsDirty = dirty;
	}

	/**
	 * Returns the "change" state for this set of macros.
	 * The "change" state represents whether the macros were changed or not.
	 * This state is not reset when the serialize() method is called
	 * Users can use this state to monitor whether the macros were changed or not.
	 * The "change" state can be reset only by calling the setChanged(false) method
	 * @return boolean
	 */
	@Override
	public boolean isChanged() {
		return fIsChanged;
	}

	/*	public boolean isExpanded(){
			return fExpandInMakefile;
		}
	*/
	/*	public void setExpanded(boolean expand){
			if(fExpandInMakefile != expand){
				fExpandInMakefile = expand;
				fIsDirty = true;
				//should we set the change state here?
				fIsChanged = true;
			}
		}
	*/
	/**
	 * sets the "change" state for this set of macros.
	 * @see org.eclipse.cdt.internal.core.cdtvariables.StorableCdtVariables#isChanged()
	 * @param changed represents the new "change" state
	 */
	public void setChanged(boolean changed) {
		fIsChanged = changed;
	}

	@Override
	public ICdtVariable getMacro(String name) {
		if (name == null || "".equals(name = name.trim())) //$NON-NLS-1$
			return null;

		ICdtVariable var = getMap().get(name);
		if (var == null) {
			int indx = name.indexOf(':');
			if (indx != -1) {
				String baseName = name.substring(0, indx);
				ICdtVariable tmp = getMap().get(baseName);
				if (tmp != null && CdtVariableManager.getDefault().toEclipseVariable(tmp, null) != null) {
					var = EclipseVariablesVariableSupplier.getInstance().getVariable(name);
				}
			}
		}
		return var;
	}

	@Override
	public ICdtVariable[] getMacros() {
		Collection<ICdtVariable> macros = getMap().values();
		return macros.toArray(new ICdtVariable[macros.size()]);
	}

	@Override
	public ICdtVariable deleteMacro(String name) {
		if (fIsReadOnly)
			throw ExceptionFactory.createIsReadOnlyException();

		if (name == null || "".equals(name = name.trim())) //$NON-NLS-1$
			return null;

		ICdtVariable macro = getMap().remove(name);
		if (macro != null) {
			fIsDirty = true;
			fIsChanged = true;
		}

		return macro;
	}

	@Override
	public boolean deleteAll() {
		if (fIsReadOnly)
			throw ExceptionFactory.createIsReadOnlyException();
		Map<String, ICdtVariable> map = getMap();
		if (map.size() > 0) {
			fIsDirty = true;
			fIsChanged = true;
			map.clear();
			return true;
		}
		return false;
	}

	@Override
	public boolean contains(ICdtVariable var) {
		ICdtVariable curVar = getMacro(var.getName());
		if (curVar == null)
			return false;

		if (new VarKey(curVar, false).equals(new VarKey(var, false)))
			return true;

		return false;

	}
}

Back to the top