define STRICT_R_HEADERS to avoid definition of Free, ... #898
Comments
|
I am fairly certain we looked into this at some point early on when we did the header cleanup. And I am also pretty sure that I too had been bitten by it but I don't recall now why we are not doing it. That said, "it's late now" as we have 1400+ reverse depends so change may not be easy. One way around it is of course to include Rcpp.h last but that is not always possible. Can you (for now) define |
|
Of course I can and I am. I don’t need this, i was just sharing the information. |
|
And I started a reverse depends check to see how much breakage this may introduce. I am running a full "baseline" run first so this will take a moment. |
|
I'd be on-board with setting |
|
Yes. The non-"Namespace" (in quotes, because C ...) protected identifiers are a pain, and the clashes are a nuisance. We should promote better programming and packaging practice where possible. We'll see what the rev.deps say, but at a minimum we could even try to phase it in. |
|
Would be less aggressive than undef them, Rcpp could offer an opt-in instead so that packages that really need these macros can still have them. |
|
What I was thinking is that we could possibly But let's first see what damage (if any) a definition may cause. |
|
Does not look too good. Baseline run against Rcpp 0.12.18 (once I move two new ones to skip): where 'the usual' set of packages fails. But with Details in RcppCore/rcpp-logs@f05b3e4 So for the time being this may be a documentation-only thing. Edited: Had a copy and paste error reporting the initial run twice. |
|
Dang, that's more than I expected 😞 Given the number of failures, I don't think we should force the issue -- client packages can (as arrow does) just provide the define themselves, e.g. #define STRICT_R_HEADERS
#include <Rcpp.h>(or perhaps even better enforce in |
|
Yes and yes. But I am thinking we should still try to get this in. I was thinking of two approaches, the second idea is a little fresher and less mature 😀 First Approach: Contact MaintainersThis is old gum-shoe work. Email all 60+ maintainers. Ask for a change. Give a reasonable window, say one year. Second Approach:
|
|
Using the suppressMessages({
library(RSQLite)
library(data.table)
})
run1 <- dbConnect(SQLite(), "~/git/rcpp-logs/queues/Rcpp_2018-09-07/queuefile.sqlite")
run2 <- dbConnect(SQLite(), "~/git/rcpp-logs/queues/Rcpp_2018-09-08/queuefile.sqlite")
res1 <- setDT(dbGetQuery(run1, "select * from results"))
res2 <- setDT(dbGetQuery(run2, "select * from results"))
## correct res1 for the two which were later added to skip list
res1[ package %in% c("bgsmtr", "EstMix") , result:=2]
## find the ones in run2 that failed
cands <- res2[ result==1, 1:3 ]
## inner join with run1, extract the ones that worked there --> new failures
fails <- res1[cands[,1:3], 1:3, on="package"][result==0]gets us R> fails
package version result
1: basad 0.2.0 0
2: bayesImageS 0.5-3 0
3: bcp 4.0.3 0
4: bfa 0.4 0
5: biglasso 1.3-6 0
6: bigmemory 4.5.33 0
7: bigReg 0.1.2 0
8: binnednp 0.1.0 0
9: biwavelet 0.20.17 0
10: blockmodels 1.1.1 0
11: castor 1.3.4 0
12: cbinom 1.3 0
13: circumplex 0.1.2 0
14: CorReg 1.2.8 0
15: CoxPlus 1.1.1 0
16: DGM 1.7.2 0
17: emIRT 0.0.8 0
18: EMVS 1.0 0
19: ETAS 0.4.4 0
20: facilitation 0.5.2 0
21: fasteraster 1.1.1 0
22: fastGHQuad 0.2 0
23: extraDistr 1.8.9 0
24: fDMA 2.2.3.1 0
25: frailtySurv 1.3.5 0
26: gcKrig 1.1.2 0
27: GAS 0.2.8 0
28: geogrid 0.1.0.1 0
29: glmBfp 0.0-51 0
30: Gmisc 1.6.2 0
31: HardyWeinberg 1.6.1 0
32: icenReg 2.0.8 0
33: lidR 1.6.1 0
34: lpme 1.1.1 0
35: MAINT.Data 1.2.3 0
36: mcmcse 1.3-2 0
37: medfate 0.2.2 0
38: meteoland 0.7.5 0
39: minimaxdesign 0.1.3 0
40: MSGARCH 2.3 0
41: multdyn 1.6 0
42: MultiFit 0.1.2 0
43: mwaved 1.1.5 0
44: netdiffuseR 1.20.0 0
45: nngeo 0.2.1 0
46: npsf 0.4.1 0
47: packcircles 0.3.2 0
48: PAFit 1.0.0.6 0
49: partialCI 1.1.1 0
50: pense 1.2.0 0
51: precrec 0.9.1 0
52: recmap 0.5.37 0
53: ReIns 1.0.7 0
54: rmgarch 1.3-0 0
55: robustlmm 2.2-1 0
56: rococo 1.1.5 0
57: rotations 1.5 0
58: rtdists 0.9-0 0
59: serrsBayes 0.3-13 0
60: unmarked 0.12-2 0
61: updog 1.0.1 0
62: vegclust 1.7.4 0
package version result
R> or R> dput(fails[,1:2])
structure(list(package = c("basad", "bayesImageS", "bcp", "bfa",
"biglasso", "bigmemory", "bigReg", "binnednp", "biwavelet", "blockmodels",
"castor", "cbinom", "circumplex", "CorReg", "CoxPlus", "DGM",
"emIRT", "EMVS", "ETAS", "facilitation", "fasteraster", "fastGHQuad",
"extraDistr", "fDMA", "frailtySurv", "gcKrig", "GAS", "geogrid",
"glmBfp", "Gmisc", "HardyWeinberg", "icenReg", "lidR", "lpme",
"MAINT.Data", "mcmcse", "medfate", "meteoland", "minimaxdesign",
"MSGARCH", "multdyn", "MultiFit", "mwaved", "netdiffuseR", "nngeo",
"npsf", "packcircles", "PAFit", "partialCI", "pense", "precrec",
"recmap", "ReIns", "rmgarch", "robustlmm", "rococo", "rotations",
"rtdists", "serrsBayes", "unmarked", "updog", "vegclust"), version = c("0.2.0",
"0.5-3", "4.0.3", "0.4", "1.3-6", "4.5.33", "0.1.2", "0.1.0",
"0.20.17", "1.1.1", "1.3.4", "1.3", "0.1.2", "1.2.8", "1.1.1",
"1.7.2", "0.0.8", "1.0", "0.4.4", "0.5.2", "1.1.1", "0.2", "1.8.9",
"2.2.3.1", "1.3.5", "1.1.2", "0.2.8", "0.1.0.1", "0.0-51", "1.6.2",
"1.6.1", "2.0.8", "1.6.1", "1.1.1", "1.2.3", "1.3-2", "0.2.2",
"0.7.5", "0.1.3", "2.3", "1.6", "0.1.2", "1.1.5", "1.20.0", "0.2.1",
"0.4.1", "0.3.2", "1.0.0.6", "1.1.1", "1.2.0", "0.9.1", "0.5.37",
"1.0.7", "1.3-0", "2.2-1", "1.1.5", "1.5", "0.9-0", "0.3-13",
"0.12-2", "1.0.1", "1.7.4")), class = c("data.table", "data.frame"
), row.names = c(NA, -62L), .internal.selfref = <pointer: 0x55a6d42ba0b0>)
R> |
|
I'm not a fan of just screening these packages in "Contact the maintainers" seems a better way forward. We might even consider a brief message to CRAN (Uwe) just to see if they agree the change would be warranted given the potential for a bit of disruption. (This seems like the sort of thing CRAN might want to enforce globally some day) |
|
Yes, screening is kinda hackish. Another attempt may be to email all maintainers now, say we plan to do this "in due course" but with suitable warning, say in 12 months, and then revisit in 3, 6, 0, ... months as we have time. Hopefully the list whittles down... |
|
New plan. I will email all 50+ maintainers, explaining that their package failed with strict headers and asking that they update within the year. I will also email And come September 2019, we add the CodeWith db <- setDT(tools::CRAN_package_db())
maints <- db[ fails[, .(package, version)], .(Package, Version, Maintainer) ]
## need one special case: one package without accent in name, two with
maints[Maintainer == "Miquel De Caceres <miquelcaceres@gmail.com>",
Maintainer := "Miquel De Cáceres <miquelcaceres@gmail.com>" ]
setkey(maints, Maintainer)
maints[ , .(pkgs=paste(Package, collapse=", ")), by=Maintainer]get us these maintainers: R> maints[ , .(pkgs=paste(Package, collapse=", ")), by=Maintainer]
Maintainer pkgs
1: Abdollah Jalilian <jalilian@razi.ac.ir> ETAS
2: Alexander W Blocker <ablocker@gmail.com> fastGHQuad
3: Alexios Ghalanos <alexios@4dscape.com> rmgarch
4: Andy Bosyi <andy@bosyi.com> fasteraster
5: Andy Royle <aroyle@usgs.gov> unmarked
6: Bryan Stanfill <bstanfill2003@gmail.com> rotations
7: Chibisi Chima-Okereke <chibisi@active-analytics.com> bigReg
8: Christian Panse <Christian.Panse@gmail.com> recmap
9: Clement THERY <corregeous@correg.org> CorReg
10: Clifford Anderson-Bergman <pistacliffcho@gmail.com> icenReg
11: Dan Dalthorp <ddalthorp@usgs.gov> cbinom
12: Daniel Barreiro Ures <dbu5293@gmail.com> binnednp
13: David Gerard <gerard.1787@gmail.com> updog
14: David Kepplinger <david.kepplinger@gmail.com> pense
15: Dootika Vats <D.Vats@warwick.ac.uk> mcmcse
16: Gemma Moran <gmoran@wharton.upenn.edu> EMVS
17: George Vega Yon <g.vegayon@gmail.com> netdiffuseR
18: Haiming Zhou <zhouh@niu.edu> lpme
19: Henrik Singmann <singmann+rtdists@gmail.com> rtdists
20: Isaac Gravestock <isaac.gravestock@uzh.ch> glmBfp
21: James Lo <lojames@usc.edu> emIRT
22: Jan Graffelman <jan.graffelman@upc.edu> HardyWeinberg
23: Jared Murray <jsmurray@stat.cmu.edu> bfa
24: Jean-Benoist Leger <jbleger@bordeaux.inra.fr> blockmodels
25: Jean-Romain Roussel <jean-romain.roussel.1@ulaval.ca> lidR
26: Jeffrey Girard <me@jmgirard.com> circumplex
27: Jing Peng <pengjing@live.com> CoxPlus
28: Jonas Rende <jonas.rende@fau.de> partialCI
29: Justin Rory Wishart <justin.wishart@mq.edu.au> mwaved
30: Keven Bluteau <Keven.Bluteau@unine.ch> MSGARCH
31: Krzysztof Drachal <kdrachal@wne.uw.edu.pl> fDMA
32: Leopoldo Catania <leopoldo.catania@econ.au.dk> GAS
33: Mali Salles <marinacs@gmail.com> facilitation
34: Manuel Koller <koller.manuel@gmail.com> robustlmm
35: Matt Moores <mmoores@gmail.com> bayesImageS, serrsBayes
36: Max Gordon <max@gforge.se> Gmisc
37: Michael Bedward <michael.bedward@gmail.com> packcircles
38: Michael Dorman <dorman@post.bgu.ac.il> nngeo
39: Michael J. Kane <bigmemoryauthors@gmail.com> bigmemory
40: Miquel De Cáceres <miquelcaceres@gmail.com> medfate, meteoland, vegclust
41: ORPHANED geogrid
42: Oleg Badunenko <oleg.badunenko@port.ac.uk> npsf
43: Pedro Duarte Silva <psilva@porto.ucp.pt> MAINT.Data
44: Qingyan Xiang <qxiang@illinois.edu> basad
45: S. Gorsky <s.gorsky@duke.edu> MultiFit
46: Simon Mak <smak6@gatech.edu> minimaxdesign
47: Simon Schwab <schw4b@gmail.com> DGM, multdyn
48: Stilianos Louca <louca@zoology.ubc.ca> castor
49: Takaya Saito <takaya.saito@outlook.com> precrec
50: Tarik C. Gouhier <tarik.gouhier@gmail.com> biwavelet
51: Thong Pham <thongpham@thongpham.net> PAFit
52: Tom Reynkens <tomreynkens@hotmail.com> ReIns
53: Tymoteusz Wolodzko <twolodzko+extraDistr@gmail.com> extraDistr
54: Ulrich Bodenhofer <bodenhofer@bioinf.jku.at> rococo
55: Vinnie Monaco <contact@vmonaco.com> frailtySurv
56: Xiaofei Wang <xiaofei.wang@yale.edu> bcp
57: Yaohui Zeng <yaohui-zeng@uiowa.edu> biglasso
58: Zifei Han <hanzifei1@gmail.com> gcKrig
Maintainer pkgs
R> |
|
Sounds good to me! Thanks for spearheading the effort. |
|
I wrote up something as basis for the email to rcpp-devel, CRAN and the maintainers which I just sent to rcpp-core (and CC to @romainfrancois). If you are reasing this here and there, please peruse. Thanks! |
|
Sorry guys, maybe I'm stupid, but I replaced DBL_EPSILON in my package (rococo) by DOUBLE_EPS in conjunction with setting -DSTRICT_R_HEADERS in Makevars, but I get a compiler error that DOUBLE_EPS is not defined. Have I misunderstood something? |
|
I think it is the other way around: I just added The following diff makes it build and install for me. (As an aside, you still have a lot of edd@rob:/tmp/rococo$ diff -ru rococo.orig/ rococo |head -40
diff -ru rococo.orig/src/rcor_bm.h rococo/src/rcor_bm.h
--- rococo.orig/src/rcor_bm.h 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_bm.h 2018-10-11 09:35:25.110306046 -0500
@@ -1,6 +1,7 @@
#ifndef _RCOR_BM_H
#define _RCOR_BM_H
+#define STRICT_R_HEADERS
#include <Rcpp.h>
RcppExport SEXP rcor_matrix_classical(SEXP vx, SEXP r);
diff -ru rococo.orig/src/rcor.cpp rococo/src/rcor.cpp
--- rococo.orig/src/rcor.cpp 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor.cpp 2018-10-11 09:35:07.942507260 -0500
@@ -1,3 +1,4 @@
+#define STRICT_R_HEADERS
#include <Rcpp.h>
#include "tnorms.h"
#include "rcor.h"
diff -ru rococo.orig/src/rcor_exacttest.cpp rococo/src/rcor_exacttest.cpp
--- rococo.orig/src/rcor_exacttest.cpp 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_exacttest.cpp 2018-10-11 09:39:52.815183485 -0500
@@ -1,4 +1,6 @@
+#define STRICT_R_HEADERS
#include <Rcpp.h>
+#include <limits>
#include "tnorms.h"
#include "rcor.h"
@@ -139,7 +141,7 @@
do
{
get_sums(mat_x, mat_y, perm, tnorm_fp, &c, &d);
- gamma[0] = (fabs(c + d) <= DBL_EPSILON ? 0 : (c - d) / (c + d));
+ gamma[0] = (fabs(c + d) <= std::numeric_limits<double>::epsilon() ? 0 : (c - d) / (c + d));
if (numGamma) permGamma[i] = gamma[0];
diff -ru rococo.orig/src/rcor_exacttest.h rococo/src/rcor_exacttest.h
--- rococo.orig/src/rcor_exacttest.h 2017-07-04 12:43:02.000000000 -0500
--- rococo.orig/src/rcor_exacttest.h 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_exacttest.h 2018-10-11 09:35:36.466173026 -0500
@@ -1,6 +1,7 @@
#ifndef _RCOR_EXACTTEST_H
#define _RCOR_EXACTTEST_H
+#define STRICT_R_HEADERS
#include <Rcpp.h>
using namespace Rcpp;
diff -ru rococo.orig/src/rcor.h rococo/src/rcor.h
--- rococo.orig/src/rcor.h 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor.h 2018-10-11 09:35:44.330080946 -0500
@@ -1,6 +1,7 @@
#ifndef _RCOR_H
#define _RCOR_H
+#define STRICT_R_HEADERS
#include <Rcpp.h>
using namespace Rcpp;
diff -ru rococo.orig/src/rcor_permtest.cpp rococo/src/rcor_permtest.cpp
--- rococo.orig/src/rcor_permtest.cpp 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_permtest.cpp 2018-10-11 09:41:16.726209321 -0500
@@ -1,4 +1,6 @@
+#define STRICT_R_HEADERS
#include <Rcpp.h>
+#include <limits>
#include "tnorms.h"
#include "rcor.h"
#include "rcor_permtest.h"
@@ -66,7 +68,7 @@
shuffle_in_place(perm);
get_sums(mat_x, mat_y, perm, tnorm_fp, &c, &d);
- gamma[0] = (fabs(c + d) <= DBL_EPSILON) ? 0 : (c - d) / (c + d);
+ gamma[0] = (fabs(c + d) <= std::numeric_limits<double>::epsilon()) ? 0 : (c - d) / (c + d);
if (numGamma) permGamma[i] = gamma[0];
diff -ru rococo.orig/src/rcor_permtest.h rococo/src/rcor_permtest.h
--- rococo.orig/src/rcor_permtest.cpp 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_permtest.cpp 2018-10-11 09:41:16.726209321 -0500
@@ -1,4 +1,6 @@
+#define STRICT_R_HEADERS
#include <Rcpp.h>
+#include <limits>
#include "tnorms.h"
#include "rcor.h"
#include "rcor_permtest.h"
@@ -66,7 +68,7 @@
shuffle_in_place(perm);
get_sums(mat_x, mat_y, perm, tnorm_fp, &c, &d);
- gamma[0] = (fabs(c + d) <= DBL_EPSILON) ? 0 : (c - d) / (c + d);
+ gamma[0] = (fabs(c + d) <= std::numeric_limits<double>::epsilon()) ? 0 : (c - d) / (c + d);
if (numGamma) permGamma[i] = gamma[0];
diff -ru rococo.orig/src/rcor_permtest.h rococo/src/rcor_permtest.h
--- rococo.orig/src/rcor_permtest.h 2017-07-04 12:43:02.000000000 -0500
+++ rococo/src/rcor_permtest.h 2018-10-11 09:35:54.513961738 -0500
@@ -1,6 +1,7 @@
#ifndef _RCOR_PERMTEST_H
#define _RCOR_PERMTEST_H
+#define STRICT_R_HEADERS
#include <Rcpp.h>
using namespace Rcpp;
edd@rob:/tmp/rococo$ |
|
Thanks a lot, Dirk, that solved the issue! I will upload a new version to CRAN in due course. Regarding your remark about the use of SEXP: yes, I am aware of this. I am actually using C++ types in my newer work, but I currently do not have the time to update my older packages as long as they still work well. |
|
Great! And sorry for the false lead earlier. I too thought the define was present was seemingly not. But the |
|
I just revisited this issue with two full reverse depends checks from current HEAD:
So this still seems like a no-go despite the excellent help by those who made changes. But 62 affected packages is not something we can swing by CRAN I am afraid. Details here: RcppCore/rcpp-logs@441af5d |
|
I am a newbie to the Rcpp world and I ran into this issue with Free. It wasn't too hard to figure the issue out. I saw R_STRICT_HEADERS and just added it and it fixed my issue. It would be great to have this as the default to lower the burden on new Rcpp users, but I understand the compatibility issue. |
|
It looks like we had planned to switch to Rcpp/inst/include/Rcpp/r/headers.h Lines 28 to 38 in a80908f but that transition wasn't made due to breakage in downstream packages. Maybe worth considering for a future Rcpp release (ideally after all the issues with the R 4.0.0 release have been ironed out)? |
|
Oh, if someone wants to volunteer testing this, coordinating will all affected downstream packages as well as CRAN ... then I surely won't stand in the way. But I am also unlikely to lead it anytime soon. |
|
I revisited the issue of possibly adding
The net is that we are now looking at about 52 failures, as opposed to 62 when we ran in July 2019 (see comment above). I would like to have this on. I think we could (slowly) tackle providing PRs to 50-some package. The bigger question is: 50-some out 2260. Is that enough cause for concern regarding code not on CRAN that might break? Or is it a risk worth taking? |
|
Maybe we could add this define when |
|
That is a good idea, but what bugs me a little about it is that we have several classes:
So in order to get to the first group we also yell at the second group. Can we do better? |
|
I think that the only leverage point we have is the moment in which RcppExports are created. Rcpp could then scan the code for cases in the first group and emit that warning only for them. After some time, some releases, that warning could become an error. |
|
Here's an example of what I'm proposing: 222a17b. Running The message can be improved, but that's the idea: a warning with a link to this issue and a list of files and macros to replace. |
|
Having it a phase-in is a nice approach. We could even have it as a message for one release (~ six months), then a warning for one release (~ six months) and then turn to an error. Any thoughts on how hard it would be to do the same things via @jjallaire Any word of wisdom as to whether it would foolish (:disappointed: ) or feasible (:smiley: ) to do this? Having support for |
|
I wouldn't do anything with regex's here. I do however think it would be good to take the approach you suggested with the additional step of emailing the maintainers now, in 6 months, and then again in 12 months. My guess is we'd get ~ 50/62 packages on board in that timeframe, and then given this level of effort by us CRAN might find the change more acceptable. |
|
Yes, that was my original plan -- but I got a little distracted but I think I will just sit down and open a new ticket with the list of 50 to 60 packages I had, and then between us (and any volunteers, hah!) we can pick those off and prepare a (simple) PR every couple of days (for packages on GH). Coupled with the usual emails. We can take the long view here, it doesn't have to be done by this summer but it would be great to get it in two cycles. Thanks thanks for the well-reasoned thumbs-up. Having Inaki's commit may still help for a run on the test machine to get clearly identified error messages. |
|
That sounds right to me! |
|
Pass one complete. 51 potential changes turned to 50 (as one apparently had been addressed in the meantime or didn't reproduce). All fifty have gotten a PR, or a patch via email, and seventeen have already responded. |
|
Following all the work logged in #1158 this now arrived on CRAN in version 1.0.8. |
When this macro is not defined, R headers do define macros with very generic names, such as
Freefor S-PLUS back compatibility:https://github.com/wch/r-source/blob/trunk/src/include/R_ext/RS.h#L70
https://github.com/wch/r-source/blob/trunk/src/include/R_ext/RS.h#L49
And https://github.com/wch/r-source/blob/trunk/src/include/R_ext/Constants.h#L35
For example the
Freemacro was making noise in arrow: romainfrancois/arrow@79c5001#diff-65a2f88d0196bb912b5ba669f5792492R20So, should Rcpp have
define STRICT_R_HEADERSsomewhere ?The text was updated successfully, but these errors were encountered: