patch sent by Tomas Kalibera

[edzer]

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ff89074) 93.98% compared to head (1c6cdf6) 93.98%.

:exclamation: Current head 1c6cdf6 differs from pull request most recent head 5cc3093. Consider uploading reports for the commit 5cc3093 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #256   +/-   ##
=======================================
  Coverage   93.98%   93.98%           
=======================================
  Files          46       46           
  Lines        3522     3522           
=======================================
  Hits         3310     3310           
  Misses        212      212           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[paleolimbot]

I think you might want to keep the existing Makevars.win and move the patch to Makevars.ucrt. That would apply the patch for R >4.2 but keep compatibility with R 4.0 and 4.1.

[edzer]

Thanks! Is it time to drop the CXX11 requirement?

[paleolimbot]

I think so, except there might be compiler warning issues (you might have to set it to CXX14).

The CMake things that I tried earlier are currently blocked by the fact that CMake is not supplied with Rtools (to my knowledge). I can try harder on that front but it would be helpful to have some clarity on how to build CMake projects on CRAN before spending too much time.

[edzer]

Great, I will try without CXX11 on win-builder; @kalibera, do you have intel on cmake in rtools?

[kalibera]

I think you might want to keep the existing Makevars.win and move the patch to Makevars.ucrt. That would apply the patch for R >4.2 but keep compatibility with R 4.0 and 4.1.

Yes, that is fine if you want to support R < 4.2.

[kalibera]

Great, I will try without CXX11 on win-builder; @kalibera, do you have intel on cmake in rtools?

There is cmake in Rtools (that is, even cmake for native compilation). It is documented in "Building R* and packages on Windows", e.g. https://cran.r-project.org/bin/windows/base/howto-R-devel.html.

However, I would not recommend using it unless you are embedding sources of software that only supports cmake to build (and even there might be other option - contribute that software to MXE/Rtools, as it might be easier to use cmake during cross-compilation on Linux).

A number of libraries included in MXE/Rtools does not use the "modern" way to write cmake files that makes them relocatable. Also, often, the detection of other libraries (dependencies) in external software is broken. You could easily run to some problems with this. I've added some hot-fixes of generated cmake files (sometimes contributed externally), but surely there will be more problems. Of course you can debug them, identify, and fix for others.

[paleolimbot]

Thank you! I will try a few things. I imagine Abseil already has an MXE port, in which case building S2 is probably not difficult (maybe even without cmake!).

[kalibera]

Abseil-cpp is in a recent update of Rtools43. See https://cran.r-project.org/bin/windows/Rtools/rtools43/news.html (not yet in MXE).

[paleolimbot]

@edzer I'll resurrect my cmake refactor in the next few weeks, which will let us update the internal S2/Abseil to a sane version and remove the hacks I put in to avoid cmake.

[edzer]

Awaiting @paleolimbot 's refactoring, I released s2 1.1.7 with the only change, relative to 1.1.6, to merge this makefile and push CXX11 to CXX14 in Makevars.in. I did that locally, not on GitHub, to not destroy the refactoring.

[paleolimbot]

Thanks!

The refactoring works everywhere except Windows, where I'm still sorting missing Abseil symbols resulting from the S2 version upgrade 😬 .

[paleolimbot]

Closing...I believe this is now released!