google / s2geometry

Computational geometry and spatial indexing on the sphere
http://s2geometry.io/
Apache License 2.0
2.29k stars 303 forks source link

fixing error: use of undeclared identifier 'ABSL_INTERNAL_ASSUME' #238

Closed t-brito closed 2 years ago

t-brito commented 2 years ago

Proposed fix for https://github.com/google/s2geometry/issues/237

jmr commented 2 years ago

How long has ABSL_ASSUME existed? It would be good if this worked with the last LTS abseil-cpp release.

t-brito commented 2 years ago

Not that long in fact.

This commit from 24 February 2022 creates ABSL_ASSUME as a replacement to ABSL_INTERNAL_ASSUME with the message:

Make ABSL_ASSUME publicly available

Then on 16 March 2022 ABSL_INTERNAL_ASSUME gets removed completely in the commit mentioned above, with the message:

Remove the internal-only ABSL_INTERNAL_ASSUME now that ABSL_ASSUME is available and ABSL_INTERNAL_ASSUME has no more users.

I'll add a definition which was present in Abseil's code during the transition

jmr commented 2 years ago

Did you test the final version (with any absl is fine)? Moving from travis-ci to github actions is still on my TODO...

t-brito commented 2 years ago

Ran make test with both abseil-cpp lts_2021_11_02 and master (current commit 4dcae40a07...)

Make settings:

cmake \
 -DPython3_EXECUTABLE:FILEPATH=.../bin/python3.7 \
 -DPYTHON_EXECUTABLE=.../bin/python3.7 \
 -DWITH_GFLAGS=ON \
 -DWITH_GLOG=ON \
 -DBUILD_SHARED_LIBS=OFF \
 -DWITH_PYTHON=ON \
 -WITH_GTEST=ON \
 -DGTEST_ROOT=.../googletest-release-1.8.0/googletest \
 -DCMAKE_PREFIX_PATH=abseil_[master|lts] \
 -DOPENSSL_ROOT_DIR=.../openssl@3 \
 -DOPENSSL_INCLUDE_DIR=.../openssl@3/include \
 -DCMAKE_CXX_STANDARD=11 \
 ..

All 107 tests passed with both abseil-cpp versions, some warnings related to C++11 I believe

jmr commented 2 years ago

Great, thanks for the fix!