append makevars vars instead of replacing

[siddharthab]

The current Makevars skeleton file replaces PKG_CXXFLAGS and PKG_LIBS settings in any sitewide or personal Makevars file. The use of += ensures that we pickup the settings in the user's environment but, in case of conflict, with preference to the settings in the skeleton file.

[codecov-io]

Codecov Report

Merging #331 (9abce45) into master (6e7177c) will decrease coverage by 4.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
- Coverage   36.26%   32.09%   -4.18%     
==========================================
  Files          67       63       -4     
  Lines        2418     2287     -131     
==========================================
- Hits          877      734     -143     
- Misses       1541     1553      +12     

Impacted Files	Coverage Δ
inst/include/armadillo_bits/memory.hpp	73.33% <0.00%> (-26.67%)	⬇️
inst/include/armadillo_bits/arrayops_meat.hpp	50.00% <0.00%> (-22.23%)	⬇️
inst/include/armadillo_bits/podarray_meat.hpp	11.11% <0.00%> (-13.89%)	⬇️
inst/include/armadillo_bits/debug.hpp	23.88% <0.00%> (-9.93%)	⬇️
inst/include/armadillo_bits/glue_solve_meat.hpp	26.31% <0.00%> (-7.81%)	⬇️
inst/include/armadillo_bits/subview_meat.hpp	15.68% <0.00%> (-6.14%)	⬇️
inst/include/armadillo_bits/eop_core_meat.hpp	69.23% <0.00%> (-4.11%)	⬇️
inst/include/armadillo_bits/Mat_meat.hpp	58.20% <0.00%> (-3.77%)	⬇️
inst/include/armadillo_bits/mul_gemv.hpp	42.85% <0.00%> (-2.60%)	⬇️
inst/include/armadillo_bits/op_pinv_meat.hpp	80.43% <0.00%> (-2.59%)	⬇️
... and 25 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e7177c...9abce45. Read the comment docs.

[Enchufa2]

The R Installation and Administration guide recommends using CXXFLAGS, etc., to customize package compilation, not the PKG_* versions.

[eddelbuettel]

Sorry, but where would PKG_CXXFLAGS be set outside of the package? The variables to be set in src/Makevars{,.in} are prefixed by PKG_ because they are local here.

System-wide settings in CXXFLAGS (and alike) get picked by R.

Having a number of repos here, I just did a casual grep 'PKG_.*FLAGS' git/*/src/Makevars.in and I see precisely one package doing what you suggests versuse lots using =. (Moreover, CRAN has a tendency to cry uncle when we use make extensions so I want to be careful here...). A closer look at that one package even shows that it first assigns with = and only later in a second statement uses += to modify.

@kevinushey The one package I see using += is RcppParallel. Any war stories from your end?

[kevinushey]

That's my understanding as well -- the PKG_ prefixed flags are generally reserved for use by package authors; users normally shouldn't be setting or changing them in their own Makevars. That said, sometimes users want (or need) to do this to munge package-specific flags for their own bespoke machine / compiler configurations.

[eddelbuettel]

Even then, I think every discussion I had with the powers-that-be lead to "just mod your own ~/.R/Makevars". So we have limited hooks here (and for example never can undo what is in ${RHOME}/etc/Makevars which could be -g -O2 etc) and I always felt an sed expression hook would be sweet, alas ... no such thing.

[coatless]

Just a side note, but I think moving to += in src/Makevars would trigger an R CMD check error ala:

Found the following file(s) containing GNU extensions:
  src/Makevars

Portable Makefiles do not use GNU extensions such as +=, :=, $(shell),
$(wildcard), ifeq ... endif. See section ‘Writing portable packages’ in
the ‘Writing R Extensions’ manual.

Commonly misused GNU extensions are conditional inclusions (ifeq and the like), ${shell ...}, ${wildcard ...} and similar, and the use of +=69 and :=.

https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Writing-portable-packages

I think this will only show up on the Solaris check due to footnote 69

This was apparently introduced in SunOS 4, and is available elsewhere provided it is surrounded by spaces.

[eddelbuettel]

Agreed, hence my earlier comment

(Moreover, CRAN has a tendency to cry uncle when we use make extensions so I want to be careful here...).

four comments ago.

[kevinushey]

RcppParallel avoids this since we explicitly request GNU Make; given that restriction I agree that this PR is ultimately not a good fit.

@siddharthab, I think the question underlying all this is -- do you really need to set PKG_CXXFLAGS, or would setting CXXFLAGS suffice?

[siddharthab]

Ah, so sorry for the confusion here. I was setting environment variables for these values to set --coverage, etc, e.g. here. But I did this when I was a relative noob in actually understanding how things worked. At that time, I used PKG_ env vars because some of these settings were specific to the package being built, and not generally. For example, user-specified -L flags needed for the specific package being built, but without using an explicit configure script to generate a new Makevars file.

I will change my build process to use LDFLAGS and CXXFLAGS instead, and make sure they work with my site-wide and personal Makevars files. I am withdrawing this PR.

Thank you all for your prompt support and guidance.

[siddharthab]

For posterity, here is what the resolution ended up being for me.

It turns out that I can not use LDFLAGS instead of PKG_LIBS because LDFLAGS places the flags before the object files, and the -l flags need to be posted after the object files on the command line. See the entry for -l library in the gcc manual.

On looking at R source code:

1. install.R: make is called with etc/Makeconf and share/make/shlib.mk with initial variables SHLIB, PKG_LIBS and OBJECTS optionally set on the command line.
2. etc/Makeconf: PKG_LIBS and SHLIB_LIBADD is subsumed into the ALL_LIBS variable.
3. share/make/shlib.mk: ALL_LIBS is the only variable that is placed after the object files on the command line.

So for this use case where I am specifying the libraries to link on the command line instead of editing package src/Makevars, I can use only PKG_LIBS or SHLIB_LIBADD.

For now, I have decided to edit the package src/Makevars and append my values for PKG_LIBS and PKG_CPPFLAGS to the values there (if any), as it looks like editing src/Makevars seems to be the way, and using environment variables is iffy.

[eddelbuettel]

Thanks for the follow-up. I agree that it is not entirely satisfactory -- but it is also really outside the realm of RcppArmadillo as we are playing the hand we're dealt by the R tools. Maybe you could argue your case on the r-package-devel list to see if someone else working with packages had similar issues? Or, even on r-devel?

I have the feeling that this must have been discussed before but I am with you that in the short term editing our sources is your best bet. Medium term I really want an ability to alter some of the etc/Makeconf values on the fly. But I said that for some time ... and I not sure that the R Core powers that be agree. So another voice in the choir would help.

[siddharthab]

Thank you @eddelbuettel. I just posted to r-package-devel.

It turns out that editing the package src/Makevars was not working every time because the configure script can recreate it or modify it in arbitrary ways. But, appending PKG_LIBS += .... line to the site Makevars file works fine.

[eddelbuettel]

Thanks, I saw it already. It's well written, so thank you.

One (hackish yet feasible) approach you could take (and that I have taken in past for e.g. cran2deb bulk builds on CRAN packages) is to associate optional patch files to each package, and then just mod prior to building. I.e. we, for the reasons stated, cannot switch to += in the CRAN version, you could simply do

• untar
• look for optional patch file, apply if found
• build and install

in your process loop. Or maybe you come up with something better still---but the itch is coming from your end so there may not be a much better solution coming forward. But fingers crossed. It is likely not an entirely unique situation.