-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update vendored abseil-cpp #258
Conversation
@edzer I think this is ready...what do you think? |
Wow, that looks like a massive overhaul, great! We now get the NOTE
did you try without specifying C++14, or shall I try to do so? |
This should be using C++17...which platform is giving the C++14 note? (I think the newest version of S2 requires C++17 anyway, but one constraint of using "system" abseil is that we have to build with the same C++ standard that Abseil was built against, which we should maybe have a test compile to check for). |
Sorry, I hadn't checked out the branch correctly. |
If you're generally on board, I'll clean this up and merge, then work on updating s2. After all that, we'll probably need some configure script modifications to make sure this works everywhere (and update the install/development instructions to ensure that isn't too painful). |
I now see:
which is not a NOTE but only a message (but may bite use nevertheless on some CRAN platform), but also
which you tried to address in |
Fantastic, good to go!! |
FYI - on Ubuntu 22.04, after apt-get installing
and earlier:
this is
maybe a too old version of absl? |
We can try a few things here if this is a problem...the main thing is that the Abseil dependency and the s2 R package need to use the same C++ standard, and it's easier if we just say it's C++17 everywhere. When we update s2 that might add a constraint as well so I think that issue should be punted until those files are also swapped out.
That's a great point...I think we can fix that by deleting the cmake build directory (which is the tool that generates the offending makefiles). Keeping the build directory was sort of nice for development if you didn't have a system abseil (most stable-track linux distros), but I think I've rigged it so that you can do |
Probably...I should add a version check. What does |
$ pkg-config absl_base --libs
-Wl -labsl_base -labsl_raw_logging_internal -labsl_log_severity -labsl_spinlock_wait |
I've added a minimum version to the .pc file such that it will at least reject the version present on 22.04...it seems strange that pkg-config gives invalid flags but I also don't think that "system" abseil has been around for very long and maybe just nobody uses it (or always uses it via cmake). Before a release we will need to add some docker-compose checks to ensure this will actually work with the system-provided abseil when it's detected. |
Here goes nothing! |
* Update vendored abseil-cpp (#258) * copy some prior art * remove previous absl files * renove abseil files * add vendor in tools/ * it works! * fix some initial check issues * lazier config * remove r 3.6 on windows * see if removing the braces helps * maybe fix substitution * just link it all * try again * helppers * slight updates * better flags * more cmake * just hard code it * pkg-config * add gnu make to makefile * don't link rt * ignore windows dir * possible progress for R 4.2 * more win checks * don't use configure.win * maybe working on R 4.2 * also use cxx17 * check for absl * maybe with quotes * version and news * maybe the right flags * maybe try unifying makevars * maybe work on R 4.2/4/1 * maybe fix makevars.win * maybe fix for R 4.2 * maybe fix R 4.1 * see if we're getting the right if statement * temp fix for old s2 warnings * maybe fix again * try again * document configure.win * maybe fix R 4.2 warning * try again for globals * maybe use different namespace * use pkg-config abseil * fix comment * maybe fix * quality of life improvements * clean news * better cleanup/configure * reasonable path to not awful dev setup without system abseil * enforce minimum version in pkg-config check * start to update * maybe it builds * maybe fix vendored version * maybe on windows * fix define * maybe fix snprintf warning * use union instead of coverage union * don't include testing utils * add status in windows * try the define trick * fix cleanup * maybe fix compilation on windows * remove version-specific override * try again to fix * reapply coding hack * maybe fix stderr references * skip old R on windows for now
This extracts the abseil component of #249 an uses it to update the vendored Abseil version.
The current code vendors a bunch of source files from an old version of abseil-cpp...this has resulted in some challenges since abseil-cpp is not really intended to be built in this way (although it has been delightfully stable for many years). Unfortunately, this prevents us from updating S2 because updated S2 won't build against our very old version of Abseil ( #257 ).
This PR updates the code to use (1) abseil via
pkg-config
(works on MacOS, Linux, and Windows), and (2) by using CMake to build the vendored abseil-cpp. This also works on MacOS, Linux, and Windows, (although it requirescmake
). Abseil is available on MacOS viabrew install abseil
,apt-get install libabsl-dev
on Ubuntu (maybe only since 22.04 LTS), andabseil-cpp-devel
on Fedora (Haven't tested yet).On Windows specifically, the checks that CRAN performs will always use abseil-cpp from RTools. For older versions, it will use cmake to do build the local version (may need some workshopping...it currently works but I am not sure of the details).