Issue1031

Title Build system does not use default compiler
Priority feature Status resolved
Superseder Nosy List florian, jendrik, malte, victor.paleologue
Assigned To victor.paleologue Keywords
Optional summary
The build system prevents the default compiler of the host to be used, and uses g++ instead. It may lead to potential incompatibilities.

Created on 2021-07-20.10:57:58 by victor.paleologue, last changed by florian.

Summary
The build system prevents the default compiler of the host to be used, and uses g++ instead. It may lead to potential incompatibilities.
Messages
msg10431 (view) Author: florian Date: 2021-09-01.10:57:00
OK, all checks passed on Github as well. I added a change log entry and merged. Thanks for working on this.
msg10430 (view) Author: victor.paleologue Date: 2021-09-01.10:46:22
That works fine for me too.
msg10429 (view) Author: victor.paleologue Date: 2021-09-01.10:31:18
Let me just test it with the Android NDK to confirm. It requires clang and does not include gcc at all.
msg10428 (view) Author: florian Date: 2021-09-01.10:27:14
I tested the current pull request on the grid with different sets of loaded modules and it seems to work. I also tested the environment variables CC/CXX to override the default and that also worked. I did not test clang because I didn't find a module for it but I don't see an issue with this.

I left another minor comment on the pull request. Once this is done, I'm fine with merging this.
msg10425 (view) Author: victor.paleologue Date: 2021-08-31.14:04:36
The compute servers are not publicly available, but we should avoid breaking their configuration. To be safe, we agree on keeping the line we discussed:

find_program(CMAKE_C_COMPILER NAMES $ENV{CC} cc gcc PATHS ENV PATH NO_DEFAULT_PATH)

And add a comment mentioning that we explicitly select a default compiler for when CMake does not respect environment variables such as PATH with some generators. See https://stackoverflow.com/a/45934279 and http://issues.fast-downward.org/issue1031 for details.

I am updating https://github.com/aibasel/downward/pull/61 now.
msg10424 (view) Author: malte Date: 2021-08-31.13:07:25
Regarding the origin of these "find_program" lines: they were added by Jendrik Seipp on October 8, 2015. Here is the git log entry:

=============================================================================
commit 57909cc6102934ff42a81ea421667e2a3cd19dfa
Author: Jendrik Seipp <jendrik.seipp@unibas.ch>
Date:   Thu Oct 8 15:48:36 2015 +0200

    [main] respect the PATH environment variable when searching for compilers
=============================================================================

This was done directly on the main branch, so not as part of an issue, which makes it a bit harder to reconstruct the motivation.

Checking my Fast Downward email folder for the relevant period, I found this message by Jendrik from the same day:

=============================================================================
Hi everyone,

we found that at least on our computer grid, our new CMake toolchain 
didn't respect the PATH environment variable when searching for the C++ 
compiler. This is now fixed in the master repo 
(http://hg.fast-downward.org/rev/ea985d65aee9).

Cheers,
Jendrik
=============================================================================

So it looks like this is (or at least was at the time) necessary to be able to build Fast Downward on our compute servers. Note that such compute servers often use non-standard paths to executables configured dynamically via the "module" command rather than something like the "alternatives" system. The reason for this is that this is meant to be per-user per-job setup rather than something systemwide.
msg10410 (view) Author: florian Date: 2021-08-16.12:30:23
> let us also add a comment
Fine with me.

> how do you set a different CMake generator with ./build.py?
This is not possible with a parameter to build.py. You have to either use a manual build or define a custom build alias: http://www.fast-downward.org/ObtainingAndRunningFastDownward#Manual_and_Custom_Builds
msg10409 (view) Author: victor.paleologue Date: 2021-08-13.10:15:51
By the way, how do you set a different CMake generator with ./build.py?
msg10408 (view) Author: victor.paleologue Date: 2021-08-13.10:07:50
Then let us also add a comment next to that line pointing at the related CMake issue; otherwise someone else will come like me and try to remove it.
msg10406 (view) Author: florian Date: 2021-08-13.09:57:20
Generators are a CMake term. CMake doesn't handle building the code directly, it generates files that handle the build (e.g. a Makefile, an NMake file, a ninja file, a Visual Studio project, an XCode project, ...). You can chose between them by picking a generator (https://cmake.org/cmake/help/latest/manual/cmake-generators.7.html). The default on Unix is a Makefile but you can generate others as well.

We added the line in question to work around the fact that CMake does not handle the default correctly in some of the generators.
msg10405 (view) Author: victor.paleologue Date: 2021-08-13.09:44:36
Sorry for the slow ping pong, I hope that does not bother you too much.

What is a generator? I understood it as a buildfarm job. So I thought this would work if it was job that was fixed, by defining `gcc` as the compiler. In that case removing the CMake line would be sufficient (= "works"), and no notion of fallback would be involved.

Now I am seeing that they're actually components of fast downward. If they do not build, that is too bad, but I do not see how this would work it around:

    find_program(CMAKE_C_COMPILER NAMES $ENV{CC} cc gcc PATHS ENV PATH NO_DEFAULT_PATH)

If cc is set to something else than gcc, gcc will not be used, and that will give the same result as without this line. Also if no compiler is set at the system level, it is not this project's business to find one in particular.
msg10404 (view) Author: florian Date: 2021-08-13.09:23:32
> But putting nothing also works, so I wonder why doing it.

What do you mean with "works"? I already said that it doesn't work with all generators which was the reason why we added that line (please see 
msg10376, msg10392, and msg10395 for that discussion).

> I believe it would be saner to just remove that line.
I would agree with that, if it would work on all generators but it doesn't.


> I agree, but I think defining a fallback is not to be defined by the project.
> It's the system calling the build script that should define which compiler to 
> use, not the other way round (it's a sort of circular dependency).
The fallback is only used in the case that the system calling the build does not define a compiler. You cannot have the system define a fallback for the case where it doesn't define a compiler. This would be in itself a default compiler which already exists on the system level.
msg10403 (view) Author: victor.paleologue Date: 2021-08-12.17:41:52
> Which version of the code are you talking about? The one currently implemented or the one from msg10376? Because the one from msg10376 explicitly prefers the default compiler (cc) over the gnu one (gcc).
Sorry, yes. But putting nothing also works, so I wonder why doing it.

> The desired behavior is to prefer CC/CXX over the system compiler (if the variables are set), and the system compiler over a specific fallback (gcc/g++ in our case).
I agree, but I think defining a fallback is not to be defined by the project. It's the system calling the build script that should define which compiler to use, not the other way round (it's a sort ofcircular dependency).

So yes, your solution will work, but I believe it would be saner to just remove that line. I can update my PR according to your preference, ultimately.
msg10395 (view) Author: florian Date: 2021-08-05.23:15:58
Which version of the code are you talking about? The one currently implemented or the one from msg10376? Because the one from msg10376 explicitly prefers the default compiler (cc) over the gnu one (gcc).

As you said: after updating the alternatives, the default compiler (clang) is defined with a symlink (/usr/bin/cc) leaving CC empty. The line in msg10376 then does not find anything CC, so moves on to the hint "cc" and finds /usr/bin/cc which links to clang.

> Also, which generator fails?
I don't remember. Trying out all combinations of generators and compatible operator systems is very tedious but if you are interested, maybe you can find the information in the tracker. One of them did fail, however, which was the reason why we added that line of code. So this is not just theoretical, we already had this issue and your suggestion of removing that line would revert back to that code.

> If you want to set g++ on a specific platform from your CI, you should do that in the CI scripts, by leveraging CMake options.

I don't agree. Overwriting the system compiler with the environment variables CC/CXX is a normal way of changing the default for a single invocation and should work. If CC/CXX are set, it should not be necessary to use CMake options to use the compilers specified there. The desired behavior is to prefer CC/CXX over the system compiler (if the variables are set), and the system compiler over a specific fallback (gcc/g++ in our case). I think the code in msg10376 achieves this and you have not argued against that specifically yet (only against the way that is implemented in the code currently).
msg10394 (view) Author: victor.paleologue Date: 2021-08-05.18:08:58
Also, which generator fails?
If you want to set g++ on a specific platform from your CI, you should do that in the CI scripts, by leveraging CMake options.
msg10393 (view) Author: victor.paleologue Date: 2021-08-05.18:06:59
So let us say your system's default compiler program is different from gcc, let us say after this kind of change: https://askubuntu.com/questions/1198087/how-to-set-clang-9-as-the-default-c-compiler-on-ubuntu-19-10
The actual compiler is clang, defined via a symbolic link, leaving CC empty.
Your code sees that and sets gcc, in contradiction to the system's default.

If you simply remove this piece of code with find_program(...), everything works just fine.
msg10392 (view) Author: florian Date: 2021-08-05.15:51:57
OK, I understand that the only way to avoid g++ in the current implementation is to force another compiler. I don't see why the proposal in msg10376 does not solve this problem: if no compiler is set with the environment variable CC, then the default compiler cc will be preferred over gcc. In this solution, gcc is only used if cc is not found and CC is not set.

We want to keep the option of forcing another (non-default) compiler. As I mentioned in msg10376, leaving out the find_program line and relying on CMake's default behavior (as in your pull request) does not work for all generators.
msg10391 (view) Author: victor.paleologue Date: 2021-08-05.14:30:18
I think you misunderstood my use of the word "force".
Let me rephrase: the default compiler setting is ignored, and replaced by g++.
So the only way to avoid using g++ is to force another compiler.
Your solution does not fix this issue.
Please re-read my proposed solution with that rephrasing in mind.
msg10376 (view) Author: florian Date: 2021-07-20.16:28:58
I don't agree that this forces gcc. The environment variables CC/CXX have preference over gcc/g++ in this find_program call. For example clang can be used with
  CC=clang CXX=clang++ ./build.py.

As for ignoring the default compiler, that is the point: we want to be able to overwrite the default compiler with the environment variables.

The stackoverflow answer you cited mentions that using the environment variables CC/CXX is not guaranteed to work for all generators (under "Method 1" in the top answer), which IIRC was the reason why we added that line that looks into the environment variables explicitly.

My suggested fix here would be to replace 
  find_program(CMAKE_C_COMPILER NAMES $ENV{CC} gcc PATHS ENV PATH NO_DEFAULT_PATH)
with
  find_program(CMAKE_C_COMPILER NAMES $ENV{CC} cc gcc PATHS ENV PATH NO_DEFAULT_PATH)
And of course analogously for the c++ compiler.

This way would give highest priority to whatever is set in CC, second highest priority to cc and only if those two do not exist, it would use gcc.
msg10373 (view) Author: victor.paleologue Date: 2021-07-20.12:47:31
With the following CMake code in the project:
find_program(CMAKE_C_COMPILER NAMES $ENV{CC} gcc PATHS ENV PATH NO_DEFAULT_PATH)
you ignore the default compiler and force gcc. IMO this is not ok.

To support the specific compiler for each workflow, you do not need to write CMake code, because CMake already checks the CC and CXX variables: https://stackoverflow.com/questions/45933732/how-to-specify-a-compiler-in-cmake

In theory, at least. What is it, in practice, that goes wrong when you do not override the default compiler with gcc?
msg10371 (view) Author: florian Date: 2021-07-20.12:03:52
The build system uses gcc and g++ only as a fallback. The first choice are the environment variables CC and CXX, so for example clang can be used with
  CC=clang CXX=clang++ ./build.py.

We use this in our automated tests (https://github.com/aibasel/downward/blob/main/.github/workflows/ubuntu.yml) where we explicitly want to use a non-default compiler, i.e., force the compiler to a specific one instead of using the system default. Maybe a better fix would be to add cc and c++ as a secondary choice (after the environment variables CC/CXX but before gcc/g++)?
msg10369 (view) Author: victor.paleologue Date: 2021-07-20.10:57:58
Current PR for it: https://github.com/aibasel/downward/pull/44
History
Date User Action Args
2021-09-01 10:57:04floriansetstatus: in-progress -> resolved
messages: + msg10431
2021-09-01 10:46:22victor.paleologuesetmessages: + msg10430
2021-09-01 10:31:18victor.paleologuesetmessages: + msg10429
2021-09-01 10:27:14floriansetmessages: + msg10428
2021-08-31 14:04:36victor.paleologuesetmessages: + msg10425
2021-08-31 13:07:26maltesetmessages: + msg10424
2021-08-30 19:56:07maltesetnosy: + malte, jendrik
2021-08-16 12:30:23floriansetmessages: + msg10410
2021-08-13 10:15:52victor.paleologuesetmessages: + msg10409
2021-08-13 10:07:50victor.paleologuesetmessages: + msg10408
2021-08-13 09:57:20floriansetmessages: + msg10406
2021-08-13 09:44:36victor.paleologuesetmessages: + msg10405
2021-08-13 09:23:32floriansetmessages: + msg10404
2021-08-12 17:41:53victor.paleologuesetmessages: + msg10403
2021-08-05 23:15:58floriansetmessages: + msg10395
2021-08-05 18:08:58victor.paleologuesetmessages: + msg10394
2021-08-05 18:06:59victor.paleologuesetmessages: + msg10393
2021-08-05 15:51:57floriansetmessages: + msg10392
2021-08-05 14:30:18victor.paleologuesetmessages: + msg10391
summary: The build system forces using g++ instead of the default compiler of the host, leading to potential incompatibilities. -> The build system prevents the default compiler of the host to be used, and uses g++ instead. It may lead to potential incompatibilities.
2021-07-20 16:28:58floriansetmessages: + msg10376
2021-07-20 12:47:31victor.paleologuesetmessages: + msg10373
2021-07-20 12:03:52floriansetnosy: + florian
messages: + msg10371
2021-07-20 10:57:58victor.paleologuecreate