r-lib / pkgbuild

Find tools needed to build R packages
https://pkgbuild.r-lib.org
Other
66 stars 35 forks source link

`-fdiagnostics-color=always` can sometimes be added when it isn't supported #141

Closed DavisVaughan closed 1 year ago

DavisVaughan commented 2 years ago

When running pkgbuild::build(), if your C compiler supports this flag then it will get unconditionally added to an internal Makevars that it used while compiling the package. This can be problematic if your C++ compiler doesn't support this flag.

This situation arose when testing cpp11. We have one GHA workflow that tests on Ubuntu against an old gcc version, in particular, gcc 4.8. Note that support for this flag was added in gcc 4.9. The workflow was setting:

echo $'CXX1X=g++-4.8\nCXX11=g++-4.8' >> ~/.R/Makevars

which meant the C++ compiler was using gcc 4.8, but the C compiler was still using the default, which was a much newer version.

When the package was being built, the FAQ vignette which had {cpp11} knitr chunks in it failed to compile those chunks, because it tried to do so with the -fdiagnostics-color=always flag set, which wasn't recognized on this old version of gcc.

This prompted me to patch this in cpp11 by adding CC=gcc-4.8 to the Makevars as well, so it also looks like we are using an old C compiler https://github.com/r-lib/cpp11/pull/279#discussion_r953861486, but in theory I wouldn't have to do that.

build() sets the compiler flags here: https://github.com/r-lib/pkgbuild/blob/80762db02d9273997ed6234f654633359269cc9e/R/build.R#L54

And here is where compiler_flags() adds it https://github.com/r-lib/pkgbuild/blob/80762db02d9273997ed6234f654633359269cc9e/R/compiler-flags.R#L51-L59

has_compiler_colored_diagnostics() returns TRUE if the C file that is compiled by has_compiler() compiles successfully while CFLAGS = "-fdiagnostics-color=always" is set https://github.com/r-lib/pkgbuild/blob/80762db02d9273997ed6234f654633359269cc9e/R/compiler-flags.R#L76

I wonder if it should only return TRUE if both a C and C++ file can successfully be compiled with color turned on? But I guess we also have to worry about exactly which version of C++ we are testing with (CXX11 vs CXX14, etc), so that makes this a little tough to check ahead of time.

gaborcsardi commented 2 years ago

I wonder if it should only return TRUE if both a C and C++ file can successfully be compiled with color turned on? But I guess we also have to worry about exactly which version of C++ we are testing with (CXX11 vs CXX14, etc), so that makes this a little tough to check ahead of time.

Yeah, we should do this. I would not worry too much about CXX11 etc. I think it would be pretty weird if CXX supports this flag and the other CXXxx compilers don't.

DavisVaughan commented 2 years ago

Would that have been the case above though? i.e. where we set

echo $'CXX1X=g++-4.8\nCXX11=g++-4.8' >> ~/.R/Makevars

But we didn't set CXX (which I'm assuming defaulted to a much newer version?)

gaborcsardi commented 2 years ago

Why don't you set CXX? I get it that it is not used by the package in this case, but I would guess that most people use the same compiler for everything, no?

DavisVaughan commented 2 years ago

I assume they mainly targeted CXX11 because cpp11 has SystemRequirements: C++11? I'm not sure why they didn't just use CXX instead if that would have also worked

DavisVaughan commented 2 years ago

Some notes here https://github.com/r-lib/cpp11/pull/78#discussion_r466397307

gaborcsardi commented 2 years ago

Anyway, we can check CXX11 as well. Or check what the package uses.

gaborcsardi commented 1 year ago

It is not trivial to check for each CXX* compiler separately and it is also quite slow. So we are not going to do that now.

We can have an env var to opt out, that is probably enough to work around cases where a mixture of compiler versions is used.