-
Notifications
You must be signed in to change notification settings - Fork 214
Set CC and CXX environment variables for clang toolchain #1169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Thanks to @Kummallinen for the suggestion to set cc environment variables. I'll update this soon with details of how to test. |
build/org.eclipse.cdt.build.gcc.core/src/org/eclipse/cdt/build/gcc/core/ClangToolChain.java
Show resolved
Hide resolved
* set(CMAKE_C_COMPILER clang) | ||
* set(CMAKE_CXX_COMPILER clang++) | ||
*/ | ||
addIfAbsent("cc", "clang", envVarsNew); //$NON-NLS-1$ //$NON-NLS-2$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc and cxx should be upper case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does envVarsNew already include the system environment, or is it just the environment set by other parts of Core Build? If it includes the system environment then we cant use addIfAbsent as they will likely already be set if you're on Linux or macOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does envVarsNew already include the system environment,
No; its the env from the core build and/or any env vars set by the user for a "User Defined Toolchain".
private static IEnvironmentVariable[] addClangEnvVars(IEnvironmentVariable[] envVars) { | ||
List<IEnvironmentVariable> envVarsNew = new ArrayList<>(Arrays.asList(envVars)); | ||
/* | ||
* Set CC and CXX environment variables for clang and clang++. This is equivalent to setting these in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to set the ASM environment variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to set the ASM environment variable?
Good question. I do not think so. CMake appears to set ASM automatically to clang when CC is set to clang.
I tested this using the attached project.
cmakeASM.zip
In the screendshot below, it can be seen that the ASM is set to clang.exe.
Tests Test cases Prerequisites To test The following shows an example of Console build output:
Test case B) Toolchain set to gcc, uses the default gcc compiler automatically. The following shows an example of Console build output:
[1]: Before you begin |
* However, on Windows, existing cc/cxx envVars with lower-case names will have their names changed to upper case; | ||
* the value remains unchanged.<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do this - can you simply use same logic as on Linux? See code comment for why I am asking the question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our meeting on Th 5 Jun, we discussed this and agreed on windows any lower case cc/cxx env vars should be changed to upper case.
Do you no longer think that is a good idea?
Otherwise, yes, the same logic as used on Linux can be used for Windows.
iterator.remove(); | ||
envVars.add(new EnvironmentVariable(name, evOldValue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goes with comment on line 73 and I have a suggestion attached to line 81-111
When you do the remove + add you may be changing the type and logic of the variable. The removed one may not be an EnvironmentVariable
but one of the other implementations of of IEnvironmentVariable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I see that now. Renaming the variable name is tricky. Perhaps we should forget about it.
if (Platform.OS_WIN32.equals(Platform.getOS())) { | ||
// On Windows: case-insensitive check ignores case. | ||
Iterator<IEnvironmentVariable> iterator = envVars.iterator(); | ||
boolean found = false; | ||
|
||
while (iterator.hasNext()) { | ||
IEnvironmentVariable ev = iterator.next(); | ||
if (ev.getName().equalsIgnoreCase(name)) { | ||
found = true; | ||
// Replace if existing name is in lower-case and new name is in upper-case | ||
if (!ev.getName().equals(name) && name.equals(name.toUpperCase())) { | ||
// Original value is respected. | ||
String evOldValue = ev.getValue(); | ||
iterator.remove(); | ||
envVars.add(new EnvironmentVariable(name, evOldValue)); | ||
} | ||
break; | ||
} | ||
} | ||
|
||
if (!found) { | ||
envVars.add(new EnvironmentVariable(name, value)); | ||
} | ||
|
||
} else { | ||
// On non-Windows: case-sensitive check respects case (exact match). | ||
boolean isAbsent = envVars.stream().noneMatch(ev -> ev.getName().equals(name)); | ||
if (isAbsent) { | ||
envVars.add(new EnvironmentVariable(name, value)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (Platform.OS_WIN32.equals(Platform.getOS())) { | |
// On Windows: case-insensitive check ignores case. | |
Iterator<IEnvironmentVariable> iterator = envVars.iterator(); | |
boolean found = false; | |
while (iterator.hasNext()) { | |
IEnvironmentVariable ev = iterator.next(); | |
if (ev.getName().equalsIgnoreCase(name)) { | |
found = true; | |
// Replace if existing name is in lower-case and new name is in upper-case | |
if (!ev.getName().equals(name) && name.equals(name.toUpperCase())) { | |
// Original value is respected. | |
String evOldValue = ev.getValue(); | |
iterator.remove(); | |
envVars.add(new EnvironmentVariable(name, evOldValue)); | |
} | |
break; | |
} | |
} | |
if (!found) { | |
envVars.add(new EnvironmentVariable(name, value)); | |
} | |
} else { | |
// On non-Windows: case-sensitive check respects case (exact match). | |
boolean isAbsent = envVars.stream().noneMatch(ev -> ev.getName().equals(name)); | |
if (isAbsent) { | |
envVars.add(new EnvironmentVariable(name, value)); | |
} | |
} | |
boolean isAbsent = envVars.stream().noneMatch(ev -> { | |
if (Platform.OS_WIN32.equals(Platform.getOS())) { | |
return ev.getName().equalsIgnoreCase(name); | |
} else { | |
return ev.getName().equals(name); | |
} | |
}); | |
if (isAbsent) { | |
envVars.add(new EnvironmentVariable(name, value)); | |
} |
build/org.eclipse.cdt.build.gcc.core/src/org/eclipse/cdt/build/gcc/core/ClangToolChain.java
Show resolved
Hide resolved
680f662
to
88bbfc0
Compare
Hi @jonahgraham , |
Sets the C (CC) and C++ (CXX) compiler environment variables so the choice of toolchain selected in the Build Settings tab is respected. Toolchains supported are GNU GCC and Clang. Added JUnit tests for GCCToolChain and ClangToolChain. Added test document describing manual test steps. Fixes eclipse-cdt#1140
88bbfc0
to
8aa05d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
public void clangExe() throws Exception { | ||
IToolChain tc = new ClangToolChain(null, Paths.get("a/clang.exe"), null, null); | ||
IEnvironmentVariable[] variables = tc.getVariables(); | ||
assertThat(Arrays.asList(variables), hasItems( // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of assertions here 😄
IEnvironmentVariable[] envVars = { new EnvironmentVariable("CC", "CCtestvalue") }; | ||
IToolChain tc = new ClangToolChain(null, Paths.get("a/clang.exe"), null, envVars); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is certainly an interesting case. FTR if an integrator is doing this and they aren't happy with the decision you made here it is on them to resolve it in their code by providing CC and CXX environment variables.
Sets the C and C++ compiler environment variables so CMake uses Clang instead of the system default compiler.
Fixes #1140