Vignette build on windows errors

[eddelbuettel]

Vignette build on windows errors

[LTLA]

Ah, bum. Kind of weird that the package compiles but the vignette does not - I assume knitr calls sourceCpp, but I thought that sourceCpp would use the same build conditions as R itself... Anyway, it looks painful to debug, so, yeah, it's probably easiest to turn off the vignette builds on Windows.

[eddelbuettel]

This has been uploaded, and is currently awaiting the required manual inspection at CRAN as it triggers an UBSAN warning (due to the out-of-bounds vector use that is baked in).

[eddelbuettel]

I am puzzled too, but not too interested in chasing esoteric Windows crankyness. I will try just turning it off.

[eddelbuettel]

Hm, we already have eval=FALSE as a knitr::opts_chunk option. So we may not have that option.

[LTLA]

There are two chunks that do get executed (compilation and Cpp function check), so it should just be a case of setting the remaining eval=TRUE to FALSE on detecting the Windows OS, right?

[eddelbuettel]

Would you mind checking? I also used R Hub's builder and I think I got the same failures there.

I use this wrapper to just call it as a one-liner; should work the same way via Rscript (but you need to get your RHub token into place, see the rhub package documentation).

[LTLA]

I can't even get the package to install in RHub:

 294#> ** libs
 295#> *** arch - i386
 296#> In file included from annoy.cpp:39:0:
 297#> ../inst/include/annoylib.h:43:22: fatal error: sys/mman.h: No such file or directory
 298#> #include
 299#> ^
 300#> compilation terminated.
 301#> make: *** [annoy.o] Error 1
 302#> ERROR: compilation failed for package 'RcppAnnoy'

Seems like _MSC_VER is not defined - am I missing something?

[eddelbuettel]

Ah, crap. I fixed that at home but didn't push yet. Sorry:

1 file changed, 7 insertions(+), 5 deletions(-)
inst/include/annoylib.h | 12 +++++++-----

modified   inst/include/annoylib.h
@@ -35,12 +35,14 @@ typedef unsigned __int64  uint64_t;
 #include <stdint.h>
 #endif

-#ifdef _MSC_VER
-#define NOMINMAX
-#include "mman.h"
-#include <windows.h>
+#ifdef __MINGW32__
+ #ifndef NOMINMAX
+  #define NOMINMAX
+ #endif
+ #include "mman.h"
+ #include <windows.h>
 #else
-#include <sys/mman.h>
+ #include <sys/mman.h>
 #endif

 #include <string.h>

Once we're done with this iteration I'll make sure that also gets to Erik's repo.

(And the condition may need if #defined(_MSC_VER_) || defined(__MINGW32__) -- our Windows builds clearly have the latter.)

[erikbern]

Weird that this issue came up. Looking forward to the PR!

[eddelbuettel]

I think we had that before. For you Windows is always _MSC_VER but R is a little "special" that all Windows builds happens via MinGW's gcc. Not a biggie and I should have caught it. Will send PR once we've squared this.

[eddelbuettel]

@LTLA New idea: Why don't we simplify to where we were earlier and make each code a pure pandoc formatting block -- and forget about knitr?

[LTLA]

I'm up for that, as long as we can compile it regularly somehow, outside of knitr.

Edit: in any case, the modification to annoylib.h means that RcppAnnoy passes Windows check on rhub with eval=!is.win where is.win <- .Platform$OS.type=="windows" in the first chunk.

[eddelbuettel]

Whay do you mean by 'compile it regularly' ?

FWIW what I have (uncommitted) works fine at RHub. It just reverts all code blocks back to

```cpp
// ... stuff here
```

That would be simpler.

[LTLA]

Regarding compilation, the idea is to ensure all of the C++ code in the vignette is valid. This is most easily done by concatenating it all together (along with the echo=FALSE helper chunks in the vignette) and trying to compile it. This doesn't have to be via knitr, but I feel we should have a process to compile it (e.g., a unit test using the script in #31?) to make sure that our instructions are valid. For example, it was clear that the first version wasn't compiled as it had a number of errors.

[eddelbuettel]

Simpler is better. We have tests for tests and I'd be open to adding a C++-based unit tests.

I like my vignettes simple. For Rcpp we did just this for years -- typset and not running. (I know BioConductor has differed on this for years and promotes vignettes, but I as maintainer am the one who has to make sure it all works together, and there I think I prefer a simpler one.)

[eddelbuettel]

So in b4dafb2 I just pushed a simplification that should get us onto CRAN.

[LTLA]

Okey dokey.