re-enable RcppSpdlog, use it and only it to avoid stderr

pkharchenko commented 4 years ago

Thank you, Dirk!

evanbiederstedt commented 4 years ago

Thanks @eddelbuettel---I appreciate the help.

You are correct. This is how I intended to use spdlog::r_sink_mt() instead of spdlog::stdout_logger_mt() to avoid the CRAN warnings on the use of stderr.

You get an R CMD check warning on use of stderr. That is understandable as you included upstream spdlog. It will do that. Once you instrument it the way RcppSpdlog does, this issue goes away. The change is minimal as we can simply create the 'r_sink_mt' instance we need in the Hnsw constructor. I will send you a PR for that in a minute.

This is exactly right, thank you :)

Now, with regards to the compile error I see using devtools::check_win_devel() for the Windows i386/32-bit architecture, I think your solution here will work as well, i.e. try explicitly using int64_t.

My confusion was that I didn't see the compilation error until using LinkingTo: Rcpp, RcppEigen, RcppSpdlog and it was RcppEigen related.

I'll try this, and keep everyone updated. Thanks, Evan

eddelbuettel commented 4 years ago

My cunning plan of trying either win-builder or rhub was felled by the lack of RcppSpdlog -- which is kinda sad given that it has been on CRAN for a few days now. But no r-release binary package yet, so no use of those facilities. And I don't have a real Windows machine anymore.

evanbiederstedt commented 4 years ago

My cunning plan of trying either win-builder or rhub was felled by the lack of RcppSpdlog -- which is kinda sad given that it has been on CRAN for a few days now. But no r-release binary package yet, so no use of those facilities. And I don't have a real Windows machine anymore.

I've been using VMware Fusion on my Mac, and downloaded a 32-bit Windows ISO. It's clunky, but works.

Yes, I also had to install RcppSpdlog from source here, as the binaries weren't available.

We'll see how it works :)

evanbiederstedt commented 4 years ago

(re-pasting some comments here from mailing list, just to keep track)

The thing that remains extremely puzzling to me is that using Linking to: Rcpp, RcppEigen (without the RcppSpdlog calls), it compiles correctly on i386. I've created a branch here: https://github.com/kharchenkolab/N2R/tree/without_RcppSpdlog

https://win-builder.r-project.org/04120gdc336K/ It compiles correctly, and the package works as intended.

It's strange that the Linking to: Rcpp, RcppEigen, RcppSpdlog resulst in these 32-bit/64-bit errors, in particular that RcppEigen error...

eddelbuettel commented 4 years ago

That can happen as the RcppSpdlog / spdlog headers seem to have a side-effect. Of course, it should not, but I don't yet know what causes it. We'll have a RcppSpdlog binary in a day or two, maybe that will help.

In any event it also doesn't really help you as you are back to square one of the stderr issue (apart from the nuisance CRAN may make over re-including spdlog besides n2 etc pp).

eddelbuettel commented 4 years ago

A few things to try:

RcppEigen is only included in n2knn.cpp, and it is first of the includes. That means other headers may change its definitions You could try moving its #include past the one for n2/hnsw.h
The // [[Rcpp::depends(RcppEigen)]] is not needed in a package, DESCRIPTION does that. It is unlikedly to cause harm but I would still remove it
using namespace std; using namespace Rcpp is risky too

The first could be it, it could be something else. Do you want to give that a try?

evanbiederstedt commented 4 years ago

(Agree to keep things here, and then summarize on the list for the archives.)

In any event it also doesn't really help you as you are back to square one of the stderr issue (apart from the nuisance CRAN may make over re-including spdlog besides n2 etc pp).

Yes, to clarify: I don't plan on reintroducing spdlog for the CRAN submission, due to the stderr issue. My thinking now is RcppSpdlog (preferable!) or strip it from the package. Showing the above compilation is part of my efforts to figure out what could be going on here....

RcppEigen is only included in n2knn.cpp, and it is first of the includes. That means other headers may change its definitions You could try moving its #include past the one for n2/hnsw.h

The // [[Rcpp::depends(RcppEigen)]] is not needed in a package, DESCRIPTION does that. It is unlikedly to cause harm but I would still remove it

using namespace std; using namespace Rcpp is risky too

These are good ideas! I gave that a try here: https://github.com/kharchenkolab/N2R/tree/with_RcppSpdlog https://github.com/kharchenkolab/N2R/commit/188074dec469065ce574d563bf120e7a14670705

Installs elsewhere, but there's still this strange 32-bit Windows error we're struggling with: https://win-builder.r-project.org/inZQfgIm5KIm/

Hmmmm...

eddelbuettel commented 4 years ago

(Thanks for keep pushing the can down the road and trying this!)

The Eigen line noise is so annoying. You could add these to the compiler flags in src/Makevars "for now" -- we can't keep them in for CRAN but it would suppress a lot: -Wno-ignored-attributes -Wno-unused

The long long int absolutely rings a bell but I can't quite put two and two together. One aspect was/is that long long int "does not exist" on 32bit under the older standard C. We used to even comment it out (e.g. in RcppGSL). With C++11 we have long long everywhere so I seem to have forgotten what trick there was.

eddelbuettel commented 4 years ago

If you search for 'error' in https://win-builder.r-project.org/inZQfgIm5KIm/ a few earlier hits pop up that are also very suspicious and not something I have seen. That could be at the root of this.

evanbiederstedt commented 4 years ago

(Thanks for keep pushing the can down the road and trying this!)

I'm pretty fascinated now! :) And I very much appreciate the help.

You could add these to the compiler flags in src/Makevars "for now" -- we can't keep them in for CRAN but it would suppress a lot: -Wno-ignored-attributes -Wno-unused

I should have done this earlier, thanks.

Now I think we're getting somewhere!

Updated with flags here: -Wno-ignored-attributes -Wno-unused: https://github.com/kharchenkolab/N2R/tree/with_RcppSpdlog And here is the build: https://win-builder.r-project.org/D7wYeTSJAb1w/

In file included from D:/RCompile/recent/R/include/R.h:91,
                 from d:/RCompile/CRANpkg/lib/4.1/Rcpp/include/Rcpp/r/headers.h:71,
                 from d:/RCompile/CRANpkg/lib/4.1/Rcpp/include/RcppCommon.h:29,
                 from d:/RCompile/CRANpkg/lib/4.1/Rcpp/include/Rcpp.h:27,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/logger-inl.h:15,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/logger.h:365,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/details/registry-inl.h:12,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/details/registry.h:111,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/spdlog.h:13,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/RcppSpdlog:4,
                 from ./n2/include/n2/hnsw.h:29,
                 from n2knn.cpp:7:
D:/RCompile/recent/R/include/R_ext/RS.h:55: warning: "ERROR" redefined
 #define ERROR   ),error(R_problem_buf);}

In file included from D:/Compiler/rtools40/mingw32/i686-w64-mingw32/include/windows.h:71,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/fmt/bundled/format-inl.h:26,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/fmt/bundled/format.h:3643,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/fmt/fmt.h:21,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/common.h:36,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/spdlog/spdlog.h:12,
                 from d:/RCompile/CRANpkg/lib/4.1/RcppSpdlog/include/RcppSpdlog:4,
                 from ./n2/include/n2/hnsw.h:29,
                 from n2knn.cpp:7:
D:/Compiler/rtools40/mingw32/i686-w64-mingw32/include/wingdi.h:75: note: this is the location of the previous definition
 #define ERROR 0

In file included from d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigen.h:27,
                 from n2knn.cpp:17:
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:209:13: warning: 'typedef' was ignored in this declaration
             typedef typename Eigen::Map<Eigen::Matrix<T, Eigen::Dynamic, 1> > OUT ;
             ^~~~~~~
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:209:13: error: 'struct Eigen::Map<Eigen::Matrix<Scalar, -1, 1> >' redeclared with different access
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:224:13: warning: 'typedef' was ignored in this declaration
             typedef typename Eigen::Map<Eigen::Matrix<T, 1, Eigen::Dynamic> > OUT ;
             ^~~~~~~
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:224:13: error: 'struct Eigen::Map<Eigen::Matrix<T, 1, -1> >' redeclared with different access
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:238:13: warning: 'typedef' was ignored in this declaration
             typedef typename Eigen::Map<Eigen::Array<T, Eigen::Dynamic, 1> > OUT ;
             ^~~~~~~
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:238:13: error: 'struct Eigen::Map<Eigen::Array<T, -1, 1> >' redeclared with different access
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:252:13: warning: 'typedef' was ignored in this declaration
             typedef typename Eigen::Map<Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic> > OUT ;
             ^~~~~~~
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:252:13: error: 'struct Eigen::Map<Eigen::Matrix<LhsScalar, -1, -1, 0> >' redeclared with different access
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:272:13: warning: 'typedef' was ignored in this declaration
             typedef typename Eigen::Map<Eigen::Array<T, Eigen::Dynamic, Eigen::Dynamic> > OUT ;
             ^~~~~~~
d:/RCompile/CRANpkg/lib/4.1/RcppEigen/include/RcppEigenWrap.h:272:13: error: 'struct Eigen::Map<Eigen::Array<T, -1, -1> >' redeclared with different access
make: *** [D:/RCompile/recent/R/etc/i386/Makeconf:244: n2knn.o] Error 1
ERROR: compilation failed for package 'N2R'

n2knn.cpp:7 is where #include "n2/hnsw.h" is defined. That seems harmless though.

n2knn.cpp:17 is where #include <RcppEigen.h> is defined. That's where we see these errors...which it's still not clear why they would occur. (Anything ring a bell....possibly?)

I'll try re-ordering the #include <RcppEigen.h> against #include <RcppSpdlog>. Maybe that's informative.

eddelbuettel commented 4 years ago

(I am a little tied up now so apologies for brevity.)

Two key #defines we want to try:

#define R_NO_MAP as R redefines things like Erorr; afterwards it becomes Rf_Error
Add #define STRICT_R_HEADERS which e.g. hides Calloc() as R_Calloc() etc.

These prevent bad interactions and the Error hint is likely key as I mentioned in my previous comment. You need to set both "earliest" before any other includes pointing at R, Rcpp, RcppEigen, RcppSpdlog, ...

evanbiederstedt commented 4 years ago

(I am a little tied up now so apologies for brevity.)

No problem whatsoever!

So, I declared both #define R_NO_MAP and #define STRICT_R_HEADERS. We get the same error: https://win-builder.r-project.org/DU2jnH9jGdmX/

Then I try putting the #include <RcppEigen.h> header before the #include <RcppSpdlog> header.

It compiles, and works! (I'll paste the devtools winbuilder URL here when I get it.)

Now, I guess that solves our problem here (thank you!) but the question remains why these two headers clash...hmmmm....

C:\Users\evanb\N2R>"C:\Program Files\R\R-4.0.2\bin\R.exe" CMD INSTALL *gz
* installing to library 'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0'
* installing *source* package 'N2R' ...
** using staged installation
** libs
"C:/rtools40/mingw32/bin/"g++  -std=gnu++11 -I"C:/PROGRA~1/R/R-40~1.2/include" -DNDEBUG  -I'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/Rcpp/include' -I'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/RcppSpdlog/include' -I'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/RcppEigen/include'     -I"../inst/include" -I"./n2/include" -fopenmp -Wno-ignored-attributes -Wno-unused   -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign -c RcppExports.cpp -o RcppExports.o
"C:/rtools40/mingw32/bin/"g++  -std=gnu++11 -I"C:/PROGRA~1/R/R-40~1.2/include" -DNDEBUG  -I'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/Rcpp/include' -I'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/RcppSpdlog/include' -I'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/RcppEigen/include'     -I"../inst/include" -I"./n2/include" -fopenmp -Wno-ignored-attributes -Wno-unused   -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign -c check_openmp.cpp -o check_openmp.o
"C:/rtools40/mingw32/bin/"g++  -std=gnu++11 -I"C:/PROGRA~1/R/R-40~1.2/include" -DNDEBUG  -I'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/Rcpp/include' -I'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/RcppSpdlog/include' -I'C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/RcppEigen/include'     -I"../inst/include" -I"./n2/include" -fopenmp -Wno-ignored-attributes -Wno-unused   -O2 -Wall  -mfpmath=sse -msse2 -mstackrealign -c n2knn.cpp -o n2knn.o
(cd n2  && CFLAGS="-O2 -Wall  -std=gnu99 -mfpmath=sse -msse2 -mstackrealign" CXXFLAGS="-O2 -Wall  -mfpmath=sse -msse2 -mstackrealign" MAKE="/usr/bin/make -f \"C:/PROGRA~1/R/R-40~1.2/etc/i386/Makeconf\" -f Makefile" /usr/bin/make -f "C:/PROGRA~1/R/R-40~1.2/etc/i386/Makeconf" -f Makefile lib) || exit 1;
make[1]: Entering directory '/c/Users/evanb/AppData/Local/Temp/RtmpCenc5z/R.INSTALLce863b1264/N2R/src/n2'
make[1]: Nothing to be done for 'lib'.
make[1]: Leaving directory '/c/Users/evanb/AppData/Local/Temp/RtmpCenc5z/R.INSTALLce863b1264/N2R/src/n2'
C:\rtools40\mingw32\bin\nm.exe: 'sublibraries': No such file
C:/rtools40/mingw32/bin/g++ -shared -s -static-libgcc -o N2R.dll tmp.def RcppExports.o check_openmp.o n2knn.o -L/usr/lib/ -L. -lpthread -lstdc++ -ln2 -lm -LC:/PROGRA~1/R/R-40~1.2/bin/i386 -lRlapack -LC:/PROGRA~1/R/R-40~1.2/bin/i386 -lRblas -lgfortran -lm -lquadmath -fopenmp -LC:/PROGRA~1/R/R-40~1.2/bin/i386 -lR
./libn2.a(hnsw.o): duplicate section `.rdata$_ZTVN6spdlog17pattern_formatterE[__ZTVN6spdlog17pattern_formatterE]' has different size
./libn2.a(hnsw.o): duplicate section `.rdata$_ZTVN6spdlog6loggerE[__ZTVN6spdlog6loggerE]' has different size
installing to C:/Users/evanb/OneDrive/Documents/R/win-library/4.0/00LOCK-N2R/00new/N2R/libs/i386
** R
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
  converting help for package 'N2R'
    finding HTML links ... done
    Knn                                     html
    checkOpenMP                             html
    crossKnn                                html
** building package indices
** testing if installed package can be loaded from temporary location
** testing if installed package can be loaded from final location
** testing if installed package keeps a record of temporary installation path
* DONE (N2R)

eddelbuettel commented 4 years ago

Glad you got to it! And ... that just happens sometimes -- we had that with other packages too.

And for example RcppArmadillo has for years explicitly complained when Rcpp.h was already loaded. I can add a check for RcppEigen's "marker" to the RcppSpdlog header.

evanbiederstedt commented 4 years ago

I've added some notes to remember how to compile this correctly. At least I better understand why this wasn't working :)

I appreciate the help, @eddelbuettel! I've added you as a contributor to the package for your help here; you deserve credit for the time invested.

Looking forward to more great packages! Thank you!

eddelbuettel commented 4 years ago

Thank you very much, that is very kind -- and thanks also for reporting back to the rcpp-devel list!

kharchenkolab / N2R

re-enable RcppSpdlog, use it and only it to avoid stderr #2