premake / premake-core

Premake
https://premake.github.io/
BSD 3-Clause "New" or "Revised" License
3.13k stars 611 forks source link

Premake system toolset selection issue #2207

Open tritao opened 2 months ago

tritao commented 2 months ago

Problem

Lets take a simple of a Premake script with a library and toolset "gcc" and generate with gmake2 action.

The generated Makefile will generate with the default $(CC) and $(CXX) Make implicit variables.

From the Make documentation:

CC Program for compiling C programs; default cc. CXX Program for compiling C++ programs; default g++.

Notice is uses g++ instead of what I would expect `c++``.

On my system cc and c++ are mapped to Clang, so I end up with a weird build mixing both Clang for C and GCC for C++.

This also ends up being a problem with the premake-export-compile-commands extension. https://github.com/tritao/premake-export-compile-commands/commit/495800463587cc8fe817aefcae2d41a10c1e8b89

Even though it uses toolset.gettoolname, it ends up generating the JSON file as: command": "cxx -MD -MP

Which is wrong, since it will look for system includes with the cxx command which is Clang, while the build uses the implicit g++.

Proposed Solution

If I choose toolset "gcc", then gmake action should generate the Makefile with C = gcc and CXX = g++.

For the system compiler, a new toolset "system", can be added, default to C = ccandCXX = c++`.

Basically the gmake action should always generate explicit C and CXX rules or we may end up mixing toolsets.

@samsinsane Any thoughts on this?

samsinsane commented 2 months ago

If I choose toolset "gcc", then gmake action should generate the Makefile with C = gcc and CXX = g++.

I agree, but even if you don't explicitly pick GCC, if Premake uses GCC it should output that stuff. Otherwise problems like this will persist.

For the system compiler, a new toolset "system", can be added, default to C = ccandCXX = c++`.

I like the idea, but I'm not sure I agree with the usage of cc and c++. Clang and GCC don't support the same set of flags (there is considerable overlap, but still different) and using cc and c++ would prevent users from sharing their Makefiles (which Premake does for the source releases), so we would need to know what they point to and emitting the actual toolset names would allow users to continue sharing their Makefiles.

Jarod42 commented 2 months ago

My UT https://github.com/Jarod42/premake-sample-projects/tree/master/projects/project-toolset doesn't catch it directly :-/ but "default" msys2 doesn't have cc so cannot compile C-files with default toolset.

tritao commented 2 months ago

If I choose toolset "gcc", then gmake action should generate the Makefile with C = gcc and CXX = g++.

I agree, but even if you don't explicitly pick GCC, if Premake uses GCC it should output that stuff. Otherwise problems like this will persist.

OK, so is the fix here to default CC to gcc explicitly in the generated Makefile?

For the system compiler, a new toolset "system", can be added, default to C = ccandCXX = c++`.

I like the idea, but I'm not sure I agree with the usage of cc and c++. Clang and GCC don't support the same set of flags (there is considerable overlap, but still different) and using cc and c++ would prevent users from sharing their Makefiles (which Premake does for the source releases), so we would need to know what they point to and emitting the actual toolset names would allow users to continue sharing their Makefiles.

Agreed, that was just to keep the existing behavior and allow defaulting to the system toolset. I don't see much the point of it, just use either gcc or clang, as instructed by the toolset.

samsinsane commented 1 month ago

OK, so is the fix here to default CC to gcc explicitly in the generated Makefile?

I could be wrong, but I think the issue is that we prevent gcc from being emit unless it has a prefix or version. Removing that code will probably be the fix for this?

Agreed, that was just to keep the existing behavior and allow defaulting to the system toolset. I don't see much the point of it, just use either gcc or clang, as instructed by the toolset.

Perhaps an explicit "system" value doesn't have much point, but I do think that this should be changed to actually detect the system compiler instead of assuming GCC. Probably being a helper function in tools.lua so that all actions can use the same detection logic and modules can extend that logic as needed.

Jarod42 commented 1 month ago

OK, so is the fix here to default CC to gcc explicitly in the generated Makefile?

I could be wrong, but I think the issue is that we prevent gcc from being emit unless it has a prefix or version. Removing that code will probably be the fix for this?

I think fixing gcc.gettoolname to not return nil would indeed be the right fix. (Also apply to toolset msc BTW)

In premake-ninja, I "hack" gcc.gettoolname call with

cfg.gccprefix = cfg.gccprefix or ""

So prefix is not nil, and function returns expected name.

For the system compiler, a new toolset "system", can be added, default to C = cc and CXX = c++.

I dislike the naming, "cc" seems more appropriate (so just another toolset).