Refactor to use 'cpp11' (instead of 'Rcpp', C or 'cpp4r')#269
Open
pachadotdev wants to merge 65 commits into
Open
Refactor to use 'cpp11' (instead of 'Rcpp', C or 'cpp4r')#269pachadotdev wants to merge 65 commits into
pachadotdev wants to merge 65 commits into
Conversation
clean Makefile a bit
Author
|
@estebanlp here is one of the modifications I made to 'later' so that the server works ok and without the ABI issue |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dear 'later' maintainers:
I hope you are doing well.
This new PR is essentially #268 with ctrl+f to replace "cpp4r" with "cpp11". The previous flow was C, then 'cpp4r', 'cpp11'.
I agree that writing C code can be more problematic, I have been using C to fix a few R bugzilla issues over the last year (e.g., pachadotdev/r-source@a390ce7 / https://bugs.r-project.org/show_bug.cgi?id=18693).
I honestly have no idea of the tradeoffs. I thought that C would be less subjective, then the comments to the previous PR suggested 'cpp11' for being more well known, so therefore 'cpp11' is the good choice here.
To test the changes I did the following.
make checkto run adevtools::check(), if that passes the it installs 'later'make checkloops anotherdevtools::check()on 'latertest', a side pkg that like this 'later' version also uses 'cpp11'. The loop runs all the pairs between {GCC, Clang} and {CXX17, CXX20, CXX23}, for example, to detect anything that Mac won't likeI hope this makes sense. This change was particularly straightforward because I created 'cpp4r' with an idea of "cpp4r is to cpp11 what MariaDB is to MySQL".
Regarding some silly changes (tabulation, include order, etc), this is not really my decision. At some point I started copying the approach from 'cpp11' (https://github.com/r-lib/cpp11/blob/main/Makefile#L15), so when I run
make format, it applies a styler very similar tostyler::style_pkg()on C++ side. I hope this removes some subjectivity, the only substantive changes I made to the includes are making those not redundant, and follow a DRY approach.There is a tradeoff when moving compiled code tests to a side package 'latertest'. This is also borrowed from https://github.com/r-lib/cpp11/tree/main/cpp11test, after all any R/C++ gaps I had after grad school C++ were filled by following what Jim Hester was doing with 'cpp11' and I heavily borrow from his style. The advantage of this approach is that it allows to test the 'later' API in the same way that packages that use that API would use it. The disadvantage is that compiling snippets is much more convenient as an immediate solution.
Now the subjective side of why this PR. I know 'later' is not my project, but right now I have a very large number of users that use a Shiny app I run on a university cluster that runs RHEL7. This move to 'cpp11' is merely to avoid multiple ABI issues I've encountered and that tend to repeat. I am aware this is not a Posit issue as RHEL7 is severely outdates but so it seems that other universities and labs have the same issue. My Shiny app has only R code, and inspecting the Shiny Server logs motivated me to try to fix this permanently.
I hope it is useful. I remain fully flexible to adapt anything needed. What I do not want to do is to create a 'later2' or something like that just to patch my dependencies, as it would only add more fragmentation and FOSS is already super fragmented.
Please keep me posted. This has given me nightmares, so I might go back to work on the 'httpuv' changes tonight if I stay restless about this. The issue has been repeated to the point that IT gave me sudo access to the cluster because of multiple emails from users that want immediate fixes. This has also been a source of extra testing, as I have installed this modified 'later' version and I see 20 sessions in the cluster (including my own) with quiet error logs.
Resources I used:
Be safe and well,
MVS