Skip to content
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

Merged
merged 51 commits into from
Jun 17, 2024
Merged

Update vendored abseil-cpp #258

merged 51 commits into from
Jun 17, 2024

Conversation

paleolimbot
Copy link
Collaborator

@paleolimbot paleolimbot commented Jun 2, 2024

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 requires cmake). Abseil is available on MacOS via brew install abseil, apt-get install libabsl-dev on Ubuntu (maybe only since 22.04 LTS), and abseil-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).

@paleolimbot paleolimbot changed the title Update vendord abseil-cpp Update vendored abseil-cpp Jun 2, 2024
@paleolimbot paleolimbot marked this pull request as draft June 2, 2024 03:14
@paleolimbot paleolimbot marked this pull request as ready for review June 15, 2024 16:18
@paleolimbot
Copy link
Collaborator Author

@edzer I think this is ready...what do you think?

@edzer
Copy link
Member

edzer commented Jun 16, 2024

Wow, that looks like a massive overhaul, great!

We now get the NOTE

* checking C++ specification ... NOTE
  Specified C++14: please drop specification unless essential

did you try without specifying C++14, or shall I try to do so?

@paleolimbot
Copy link
Collaborator Author

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).

@edzer
Copy link
Member

edzer commented Jun 16, 2024

This should be using C++17...which platform is giving the C++14 note?

Sorry, I hadn't checked out the branch correctly.

@paleolimbot
Copy link
Collaborator Author

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).

@edzer
Copy link
Member

edzer commented Jun 16, 2024

I now see:

* checking C++ specification ... OK
  Not all R platforms support C++17

which is not a NOTE but only a message (but may bite use nevertheless on some CRAN platform), but also

* checking for GNU extensions in Makefiles ... NOTE
GNU make is a SystemRequirements.

which you tried to address in DESCRIPTION, so let's leave it like it is. I'm running it through win-builder / r-devel now.

@edzer
Copy link
Member

edzer commented Jun 17, 2024

Fantastic, good to go!!

@edzer
Copy link
Member

edzer commented Jun 17, 2024

FYI - on Ubuntu 22.04, after apt-get installing libabsl-dev, I see at the end:

   g++ -std=gnu++17 -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -flto=auto -Wl,-z,relro -o s2.so cpp-compat.o s2-accessors.o s2-bounds.o s2-cell.o s2-cell-union.o s2-constructors-formatters.o s2-predicates.o s2-transformers.o init.o RcppExports.o s2-geography.o s2-lnglat.o s2-matrix.o wk-impl.o -Ls2 -ls2static -Wl -labsl_cord -labsl_raw_hash_set -labsl_hashtablez_sampler -labsl_exponential_biased -labsl_synchronization -labsl_graphcycles_internal -labsl_stacktrace -Wl -labsl_symbolize -labsl_malloc_internal -labsl_time -labsl_civil_time -labsl_time_zone -labsl_hash -labsl_city -labsl_bad_optional_access -labsl_bad_variant_access -labsl_wyhash -labsl_str_format_internal -labsl_strings -labsl_strings_internal -labsl_int128 -labsl_throw_delegate -labsl_demangle_internal -Wl -labsl_base -labsl_spinlock_wait -labsl_debugging_internal -labsl_raw_logging_internal -labsl_log_severity -l:libssl.so.3 -l:libcrypto.so.3 -L/usr/lib/R/lib -lR
   g++: error: unrecognized command-line option ‘-Wl’; did you mean ‘-W’?
   g++: error: unrecognized command-line option ‘-Wl’; did you mean ‘-W’?
   g++: error: unrecognized command-line option ‘-Wl’; did you mean ‘-W’?
   make[1]: *** [/usr/share/R/share/make/shlib.mk:10: s2.so] Error 1
   make[1]: Leaving directory '/home/edzer/git/s2/src'
   ERROR: compilation failed for package ‘s2’

and earlier:

ℹ Re-compiling s2 (debug build)
── R CMD INSTALL ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
─  installing *source* package ‘s2’ ...
   ** using staged installation
   Found pkg-config cflags and libs!
   Testing compiler using PKG_CFLAGS=
   -Wl -labsl_base -labsl_raw_logging_internal -labsl_log_severity -labsl_spinlock_wait
   ** Using abseil-cpp from pkg-config
   :/home/edzer/git/s2/tools/pkgconfig
   Using PKG_LIBS=-Wl -labsl_cord -labsl_raw_hash_set -labsl_hashtablez_sampler -labsl_exponential_biased -labsl_synchronization -labsl_graphcycles_internal -labsl_stacktrace -Wl -labsl_symbolize -labsl_malloc_internal -labsl_time -labsl_civil_time -labsl_time_zone -labsl_hash -labsl_city -labsl_bad_optional_access -labsl_bad_variant_access -labsl_wyhash -labsl_str_format_internal -labsl_strings -labsl_strings_internal -labsl_int128 -labsl_throw_delegate -labsl_demangle_internal -Wl -labsl_base -labsl_spinlock_wait -labsl_debugging_internal -labsl_raw_logging_internal -labsl_log_severity -l:libssl.so.3 -l:libcrypto.so.3
   Using PKG_CFLAGS=  -DOPENSSL_SUPPRESS_DEPRECATED -DIS_LITTLE_ENDIAN

this is

Get:1 http://de.archive.ubuntu.com/ubuntu jammy/main amd64 libabsl-dev amd64 0~20210324.2-2 [918 kB]

maybe a too old version of absl?

@paleolimbot
Copy link
Collaborator Author

Not all R platforms support C++17

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.

GNU make is a SystemRequirements.

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 tools/build_absl.sh path/to/somwhere + R_S2_ABSL_HOME=path/to/somewhere as a workaround.

@paleolimbot
Copy link
Collaborator Author

maybe a too old version of absl?

Probably...I should add a version check. What does pkg-config absl_base --libs get you? (It seems like it's getting you something invalid)

@edzer
Copy link
Member

edzer commented Jun 17, 2024

What does pkg-config absl_base --libs get you?

$ pkg-config absl_base --libs
-Wl -labsl_base -labsl_raw_logging_internal -labsl_log_severity -labsl_spinlock_wait

@paleolimbot
Copy link
Collaborator Author

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.

@paleolimbot
Copy link
Collaborator Author

Here goes nothing!

@paleolimbot paleolimbot merged commit 29c11b4 into main Jun 17, 2024
10 checks passed
@paleolimbot paleolimbot deleted the update-vendored-absl branch June 17, 2024 16:47
paleolimbot added a commit that referenced this pull request Sep 25, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants