The build system prevents the default compiler of the host to be used, and uses g++ instead. It may lead to potential incompatibilities.

OK, all checks passed on Github as well. I added a change log entry and merged. Thanks for working on this.

That works fine for me too.

Let me just test it with the Android NDK to confirm. It requires clang and does not include gcc at all.

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.

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.

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.

> 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

By the way, how do you set a different CMake generator with ./build.py?

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.

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.

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.

> 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.

> 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.

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).

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.

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.

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.

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.

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.

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?

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++)?

Current PR for it: https://github.com/aibasel/downward/pull/44