Personal Makevars is now being ignored

[DavisVaughan]

https://github.com/r-lib/pkgbuild/pull/151 added new debug capabilities, but I think it also accidentally caused personal Makevars to be ignored, just due to how local_makevars() works.

It now seems that my Makevars file is being ignored entirely when I load_all(), and this is what the compilation line looks like (notice the weird O2 along with O0, which don't come from me):

clang -mmacosx-version-min=10.13 -I"/Library/Frameworks/R.framework/Versions/4.2/Resources/include" -DNDEBUG -I./rlang  -I/usr/local/include   -fPIC  -Wall -g -O2  -UNDEBUG -Wall -pedantic -g -O0 -fdiagnostics-color=always -c version.c -o version.o

My personal makevars has this in it, not appearing anywhere anymore

CFLAGS=-O2 -g -fno-common -Wall -pedantic -fdiagnostics-color=always

If i set pkg.build_extra_flags to FALSE it gets respected again, so I can do that as a workaround for now until this gets fixed again

[thomasp85]

I have experienced the same, with the change that my personal Makevars is not ignored but with -fPIC -falign-functions=64 -Wall -g -O2 added to the end (overwriting my own -O0 setting)

This is for C++14 compilation with lldb

setting pkg.build_extra_flags to false as suggested fixes this

[Andrea-Havron-NOAA]

My package includes compiled templated C++ code. Using an optimation level below -O1 on Windows is known to throw errors during compilation. I tested this by calling devtools::load_all() using an updated version of pkgbuild and the debugger successfully ran without throwing an error.

I would prefer not to have to rely on a fork of pkgbuild. I can get around the issue by setting pkg.build_extra_flags to false, but would like to be able to change the -g -O0 flag to -g -O1. Can this setting be made available through pkg.build_extra_flags?

[gaborcsardi]

@Andrea-Havron-NOAA I am sorry, but I don't follow. Can you change the flags in the package's Makevars file? Or you want to use different flags in load_all() and for regular package installation?

[Andrea-Havron-NOAA]

@Andrea-Havron-NOAA I am sorry, but I don't follow. Can you change the flags in the package's Makevars file? Or you want to use different flags in load_all() and for regular package installation?

The package has a Makevars file that controls flags for regular package installation. The issue is that load_all() will still add the -g -O0 flag, which causes an error. I want to use a different flag in load_all(), the -g -O1 instead of the -g -O0 flag.

[gaborcsardi]

I think if you set pkg.build_extra_flags to FALSE, then pkgbuild will not add any extra flags. So if you have -g -O1 in the Makevars, then that's what you get.

[gaborcsardi]

@DavisVaughan Is it intentional that you are overwriting R's default CFLAGS in your Makevars file? I.e. instead of

CFLAGS+=...

you are using

CFLAGS=...

AFAICT for the more natural CFLAGS+= form your personal Makevars config is kept, and pkgbuilds adds its own debug etc. flags to it.

Overwriting R's default flags is in general not a good idea, I think, they change quite often, and sometimes it is important to keep them. E.g.:

❯ R-4.2 CMD config CC
clang -mmacosx-version-min=10.13

~
❯ R-4.3-x86_64 CMD config CC
clang -arch x86_64

[gaborcsardi]

The current semantics, coming from withr::with_makevars() is a bit weird, imo:

If no ‘Makevars’ file exists or the fields in ‘new’ do not exist in the existing ‘Makevars’ file then the fields are added to the new file. Existing fields which are not included in ‘new’ are appended unchanged. Fields which exist in ‘Makevars’ and in ‘new’ are modified to use the value in ‘new’.

I should add that only fields that are set using = are considered as "existing" fields, and fields that are set with other operators, like += are kept. This is why using += in your user defined Makevars works.

It is hard to say what is the right semantics for a generic with_makevars(), but this is at least weird. pkgbuild uses (the reimplementation of) withr::with_makevars() quite specifically, it does not need all the operators, and I'll update it to have "better" semantics, if I manage to figure out what is "better".

[DavisVaughan]

If I don't set PKG_BUILD_EXTRA_FLAGS=false, then:

When compiling clock with =, i.e. CXXFLAGS=-O2 -g -Wall -pedantic -fdiagnostics-color=always, I see

-fPIC  -Wall -g -O2  -UNDEBUG -Wall -pedantic -g -O0

When compiling clock with +=, i.e. CXXFLAGS+=-O2 -g -Wall -pedantic -fdiagnostics-color=always, I see

-fPIC  -Wall -g -O2  -O2 -g -Wall -pedantic -fdiagnostics-color=always -UNDEBUG -Wall -pedantic -g -O0 -fdiagnostics-color=always

It looks like:

• -fPIC -Wall -g -O2 is from R
• -O2 -g -Wall -pedantic -fdiagnostics-color=always is from me
• -UNDEBUG -Wall -pedantic -g -O0 -fdiagnostics-color=always is from pkgbuild

But I think the last O0 "wins"? So I need PKG_BUILD_EXTRA_FLAGS=false otherwise I don't get any optimizations no matter what I set.

[DavisVaughan]

I imagine I originally did = rather than += either:

• out of ignorance
• or I saw -Wall -g -O2 was being repeated between what R added and what I added and wanted to prevent that from occurring

[gaborcsardi]

But I think the last O0 "wins"? So I need PKG_BUILD_EXTRA_FLAGS=false otherwise I don't get any optimizations no matter what I set.

Yes, and that is intentional in load_all(). We want to build with -O0, so that we can debug the package. I understand that this will create a slow build.

Other environments like Rust have a notion of debug and release builds, maybe we should also have this notion in pkgload as well, not just in pkgbuild.

[DavisVaughan]

For clock it dramatically changes the testing time, from around 60sec with O2 -> 215sec with O0 due to the huge amount of templating which I'm assuming isn't being inlined in the unoptimized version.

I'd be in favor of some kind of debug vs release flags

[gaborcsardi]

I'd be in favor of some kind of debug vs release flags

OK, we can think about how to do it in load_all() and/or in the IDE, and also add some support for changing this per package (and per OS?) in pkgbuild.

For now, I'll update pkgbuild to always append the debug flags in debug builds, so using = in Makevars will also work.

@DavisVaughan of course you'll still need to use PKG_BUILD_EXTRA_FLAGS to avoid the debug flags.