Skip to content
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

quote path in CxxFlags #1189

Merged
merged 4 commits into from
Nov 16, 2021
Merged

quote path in CxxFlags #1189

merged 4 commits into from
Nov 16, 2021

Conversation

kevinushey
Copy link
Contributor

Pull Request Template for Rcpp

Closes #1188.

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Preferably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

R/RcppLdpath.R Outdated Show resolved Hide resolved
R/RcppLdpath.R Outdated Show resolved Hide resolved
@eddelbuettel
Copy link
Member

It's a wee bit circular but I just approved the changes I made ... after requesting them first :) Will let it sit for a bit and merge if nobody complains...

@kevinushey
Copy link
Contributor Author

Thanks! I approve of your changes as well, not that you needed it here. :-)

@eddelbuettel eddelbuettel merged commit c1f868d into master Nov 16, 2021
@eddelbuettel eddelbuettel deleted the feature/cxx-flags-quote branch December 1, 2021 02:03
@stefanoborini
Copy link

stefanoborini commented Mar 17, 2023

Is it possible that this change has compromised building on macOS monterey and R 4.0.4? I am seeing a failure on 1.0.10 that was not occurring on 1.0.7:

clang++ -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG   -I/usr/local/include  `/Library/Frameworks/R.framework/Resources/bin/Rscript -e "Rcpp:::CxxFlags()"` -fPIC  -Wall -g -O2  -UNDEBUG -Wall -pedantic -g -O0 -c src/myfile.cc -o myfile.o
Using environment default (R version: 4.0.4, platform: x86_64-apple-darwin17.0)
src/myfile.cc:2:10: fatal error: 'Rcpp.h' file not found
#include <Rcpp.h>
         ^~~~~~~~

The content of src/Makevars is as follows:

## Use the R_HOME indirection to support installations of multiple R version
PKG_LIBS = `$(R_HOME)/bin/Rscript -e "Rcpp:::LdFlags()"`
PKG_CXXFLAGS=`$(R_HOME)/bin/Rscript -e "Rcpp:::CxxFlags()"`

I am unsure if this is due to the addition of the quotes, but it seems so. In 1.0.7 I obtained:

$ clang++ -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG   -I/usr/local/include  `/Library/Frameworks/R.framework/Resources/bin/Rscript -e "Rcpp:::CxxFlags()"` -fPIC  -Wall -g -O2  -UNDEBUG -Wall -pedantic -g -O0 -c src/myfile.cc -o myfile.o
Using environment default (R version: 4.0.4, platform: x86_64-apple-darwin17.0)
In file included from src/myfile.cc:2:
In file included from <redacted>/.envs/default/lib/Rcpp/include/Rcpp.h:27:
In file included from <redacted>/.envs/default/lib/Rcpp/include/RcppCommon.h:167:
<redacted>/.envs/default/lib/Rcpp/include/Rcpp/internal/r_coerce.h:255:7: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

<compilation continues and finishes>

@eddelbuettel
Copy link
Member

eddelbuettel commented Mar 17, 2023

Why do you use Rscript -e "Rcpp:::CxxFlags()" which was deprecated around a decade ago? If you do non-standard things, and it breaks, you get to keep the pieces. Maybe try a quick Rcpp.package.skeleton("newpackage") and compare the basic setup to what you use, you are likely better off using what everybody else is using as it is more tested.

Edit And PKG_LIBS = $(R_HOME)/bin/Rscript -e "Rcpp:::LdFlags()"` has been unused / not recommended for a decade. If you prefer the setup that was current then, maybe just a commensurate Rcpp version such as, say, Rcpp 0.10.0 from 2012. As you may be aware, CRAN tests quite rigorously, and there is a reasonably large user base. So if something breaks "just for you" maybe it was a commit from last November you now point at.

@stefanoborini
Copy link

Because the package was developed a decade ago.

@stefanoborini
Copy link

but that's beside the point... it used to work until 1.0.7. I am not saying I won't change the process. I am just puzzled that it broke for this.

@eddelbuettel
Copy link
Member

eddelbuettel commented Mar 17, 2023

You appear to want to have it both ways: Use your code from ten years ago unchanged, yet rely an brand-new contributed code? Is see a disconnect here. Many packages among the 2600+ on CRAN that use Rcpp also started a long time ago, yet managed to update their processes.

In any event, we (and CRAN) test rigourously, and I think most people find that playing with (rather than against) that setup works for them. If you manage to isolate a breakage, please try to reproduce it in a minimal setup on current versions.

@stefanoborini
Copy link

I don't deploy on CRAN. All I know is that I get dumped a 10 years old package on my lap and have to make it work, and all I know is that with 1.0.7 of Rcpp it worked and with a patchlevel version bump it doesn't.

@eddelbuettel
Copy link
Member

eddelbuettel commented Mar 17, 2023

So maybe just use an Rcpp version from ten years ago as I suggested earlier?

Rcpp versions 1.0.8 and 1.0.9 and 1.0.10 all got released after 1.0.7. You have plenty of choices to pin the "historic" Rcpp-using package you work with to other versions that most likely once worked with it. There is a subculture out there that thinks pinning is a solution; it is particularly popular with users of other languages that do not have a system like CRAN and fail to appreciate how to work with CRAN (and benefit from it). So be it -- it's a big tent out there and people can do as they please.

But Rcpp is a CRAN package, and one that works hard to be in good standing at CRAN. And which has a 15-year history of being in good standing. You are more than welcome to ignore that body of work and use random code combinations, but you cannot expect us to appreciate such undertakings. We prefer to focus on other things which also happen to be meaningful to other users.

@RcppCore RcppCore locked and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CxxFlags (and friends) should quote their library paths on Unix
3 participants