-
Notifications
You must be signed in to change notification settings - Fork 33
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
-fdiagnostics-color=always
can sometimes be added when it isn't supported
#141
Comments
Yeah, we should do this. I would not worry too much about |
Would that have been the case above though? i.e. where we set
But we didn't set |
Why don't you set |
I assume they mainly targeted |
Some notes here r-lib/cpp11#78 (comment) |
Anyway, we can check CXX11 as well. Or check what the package uses. |
It is not trivial to check for each 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. |
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:
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 r-lib/cpp11#279 (comment), but in theory I wouldn't have to do that.build()
sets the compiler flags here:pkgbuild/R/build.R
Line 54 in 80762db
And here is where
compiler_flags()
adds itpkgbuild/R/compiler-flags.R
Lines 51 to 59 in 80762db
has_compiler_colored_diagnostics()
returnsTRUE
if the C file that is compiled byhas_compiler()
compiles successfully whileCFLAGS = "-fdiagnostics-color=always"
is setpkgbuild/R/compiler-flags.R
Line 76 in 80762db
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.The text was updated successfully, but these errors were encountered: