fix: Use system Abseil on MacOS if available, improve isolation of bundled linking, fix some deprecation warnings

NEWS.md

@@ -4,9 +4,9 @@
 
 # s2 1.1.9
 
-* Implement serialization support for `s2_geography` vectors. This not only 
-  allows data containing s2 geography to be safely saved and restored across 
-  R sessions, but also enables using `s2` in parallel algorithms such as 
+* Implement serialization support for `s2_geography` vectors. This not only
+  allows data containing s2 geography to be safely saved and restored across
+  R sessions, but also enables using `s2` in parallel algorithms such as
   `mclapply()` (#283)
 * Fix code to help gcc-ubsan understand the region coverer (#275)
 * Inspect `S2_FORCE_BUNDLED_ABSEIL` in `conifigure`: if non-empty, any system

configure

@@ -15,6 +15,7 @@ PKG_RPM_NAME="openssl-devel"
 PKG_BREW_NAME="openssl"
 PKG_LIBS="-lssl -lcrypto"
 PKG_CFLAGS=""
+PKG_CPPFLAGS=""
 
 # Use pkg-config if available
 pkg-config ${PKG_CONFIG_NAME} --atleast-version=1.0  2>/dev/null
@@ -69,8 +70,12 @@ export PKG_CONFIG_PATH="${PKG_CONFIG_PATH}:`pwd`/tools/pkgconfig"
 
 echo "** Using PKG_CONFIG_PATH=${PKG_CONFIG_PATH}"
 
-if [ -z "$S2_FORCE_BUNDLED_ABSEIL" ] && [ "`uname -s`" = "Linux" ]; then
-  S2_FORCE_BUNDLED_ABSEIL=true
+if [ -z "$S2_FORCE_BUNDLED_ABSEIL" ]; then
+  if [ "`uname -s`" = "Linux" ]; then
+    S2_FORCE_BUNDLED_ABSEIL=true
+  else
+    S2_FORCE_BUNDLED_ABSEIL=false
+  fi
 fi
 
 if [ "$S2_FORCE_BUNDLED_ABSEIL" = "false" ] && pkg-config absl_s2 --libs >/dev/null 2>/dev/null; then
@@ -88,7 +93,7 @@ else
   # directory so that R CMD check does not inspects its static libraries for disallowed
   # symbols. Otherwise, we install into the package source (more conventional).
   if [ "$S2_FORCE_BUNDLED_ABSEIL" != "false" ]; then
-    CMAKE_INSTALL_PREFIX="/tmp/s2_absl_$$"
+    CMAKE_INSTALL_PREFIX="${TMPDIR:-/tmp}/s2_absl_$$"
   else
     CMAKE_INSTALL_PREFIX="`pwd`/tools/dist"
   fi
@@ -116,13 +121,40 @@ else
   R_S2_ABSL_HOME="${CMAKE_INSTALL_PREFIX}"
   export PKG_CONFIG_PATH="${R_S2_PKG_CONFIG_PATH}:${PKG_CONFIG_PATH}"
   echo "** Using PKG_CONFIG_PATH=${PKG_CONFIG_PATH}"
-  PKGCONFIG_LIBS=`pkg-config --libs absl_s2`
+
+  # Resolve only against the pkg-config files for the copy we just built. The
+  # absl_s2 file is maintained by this package; its dependencies are installed
+  # alongside the vendored Abseil libraries.
+  R_S2_BUNDLED_PKG_CONFIG_PATH="${R_S2_PKG_CONFIG_PATH}:`pwd`/tools/pkgconfig"
+  PKGCONFIG_LIBS=`PKG_CONFIG_PATH="${R_S2_BUNDLED_PKG_CONFIG_PATH}" PKG_CONFIG_LIBDIR="${R_S2_BUNDLED_PKG_CONFIG_PATH}" pkg-config --libs absl_s2`
   if [ -z "${PKGCONFIG_LIBS}" ]; then
     echo "** Failed to resolve built vendored copy of Abseil using pkg-config"
     exit 1
   fi
 
-  PKG_CFLAGS="-I${R_S2_ABSL_HOME}/include ${PKG_CFLAGS}"
+  # The vendored build contains static libraries. Replace its -labsl_* flags
+  # with exact archive paths so that the linker cannot select a system Abseil
+  # ahead of the copy whose headers and pkg-config files are used here.
+  R_S2_ABSL_LIBDIR=`PKG_CONFIG_PATH="${R_S2_BUNDLED_PKG_CONFIG_PATH}" PKG_CONFIG_LIBDIR="${R_S2_BUNDLED_PKG_CONFIG_PATH}" pkg-config --variable=libdir absl_base`
+  R_S2_STATIC_LIBS=""
+  for R_S2_LIB in ${PKGCONFIG_LIBS}; do
+    case "${R_S2_LIB}" in
+      -labsl*)
+        R_S2_ARCHIVE="${R_S2_ABSL_LIBDIR}/lib${R_S2_LIB#-l}.a"
+        if [ ! -f "${R_S2_ARCHIVE}" ]; then
+          echo "** Failed to find vendored Abseil archive ${R_S2_ARCHIVE}"
+          exit 1
+        fi
+        R_S2_STATIC_LIBS="${R_S2_STATIC_LIBS} ${R_S2_ARCHIVE}"
+        ;;
+      *)
+        R_S2_STATIC_LIBS="${R_S2_STATIC_LIBS} ${R_S2_LIB}"
+        ;;
+    esac
+  done
+  PKGCONFIG_LIBS="${R_S2_STATIC_LIBS# }"
+
+  PKG_CPPFLAGS="-I${R_S2_ABSL_HOME}/include"
   PKG_LIBS="${PKGCONFIG_LIBS} ${PKG_LIBS}"
 fi
 
@@ -131,9 +163,9 @@ CXX17="`${R_HOME}/bin/R CMD config CXX17`"
 CXX17FLAGS=`"${R_HOME}"/bin/R CMD config CXX17FLAGS`
 CXX17STD=`"${R_HOME}"/bin/R CMD config CXX17STD`
 
-echo "Testing Abseil configuration using PKG_CFLAGS=${PKG_CFLAGS}"
+echo "Testing Abseil configuration using PKG_CPPFLAGS=${PKG_CPPFLAGS} PKG_CFLAGS=${PKG_CFLAGS}"
 
-${CXX17} ${CPPFLAGS} ${PKG_CFLAGS} ${CXX17FLAGS} ${CXX17STD} -E tools/test_absl.cc >/dev/null 2>>configure.log
+${CXX17} ${PKG_CPPFLAGS} ${CPPFLAGS} ${PKG_CFLAGS} ${CXX17FLAGS} ${CXX17STD} -E tools/test_absl.cc >/dev/null 2>>configure.log
 if [ $? -ne 0 ]; then
   echo "Test compile failed!"
   echo "------- Error ---------"
@@ -151,9 +183,10 @@ fi
 
 echo "Using PKG_LIBS=$PKG_LIBS"
 echo "Using PKG_CFLAGS=$PKG_CFLAGS"
+echo "Using PKG_CPPFLAGS=$PKG_CPPFLAGS"
 
 # Write to Makevars
-sed -e "s|@cflags@|$PKG_CFLAGS|" -e "s|@libs@|$PKG_LIBS|" -e "s|@absl_home@|$R_S2_ABSL_HOME|" src/Makevars.in > src/Makevars
+sed -e "s|@cflags@|$PKG_CFLAGS|" -e "s|@cppflags@|$PKG_CPPFLAGS|" -e "s|@libs@|$PKG_LIBS|" -e "s|@absl_home@|$R_S2_ABSL_HOME|" src/Makevars.in > src/Makevars
 
 # Success
 exit 0

src/Makevars.in

@@ -1,4 +1,4 @@
-PKG_CPPFLAGS = -I../src -DSTRICT_R_HEADERS -DRCPP_NO_MASK_RF_ERROR
+PKG_CPPFLAGS = @cppflags@ -I../src -DSTRICT_R_HEADERS -DRCPP_NO_MASK_RF_ERROR
 PKG_LIBS = -Ls2 -ls2static @libs@
 PKG_CXXFLAGS = @cflags@ -pthread
 

src/s2/mutable_s2shape_index.cc

@@ -467,7 +467,7 @@ MutableS2ShapeIndex::MutableS2ShapeIndex(MutableS2ShapeIndex&& b)
       shapes_(std::move(b.shapes_)),
       cell_map_(std::move(b.cell_map_)),
       options_(std::move(b.options_)),
-      pending_additions_begin_(absl::exchange(b.pending_additions_begin_, 0)),
+      pending_additions_begin_(std::exchange(b.pending_additions_begin_, 0)),
       pending_removals_(std::move(b.pending_removals_)),
       index_status_(b.index_status_.exchange(FRESH, std::memory_order_relaxed)),
       mem_tracker_(std::move(b.mem_tracker_)) {}
@@ -480,7 +480,7 @@ MutableS2ShapeIndex& MutableS2ShapeIndex::operator=(MutableS2ShapeIndex&& b) {
   shapes_ = std::move(b.shapes_);
   cell_map_ = std::move(b.cell_map_);
   options_ = std::move(b.options_);
-  pending_additions_begin_ = absl::exchange(b.pending_additions_begin_, 0);
+  pending_additions_begin_ = std::exchange(b.pending_additions_begin_, 0);
   pending_removals_ = std::move(b.pending_removals_);
   index_status_.store(
       b.index_status_.exchange(FRESH, std::memory_order_relaxed),

src/s2/s2cell_iterator.h

@@ -17,8 +17,8 @@
 #define S2_S2CELL_ITERATOR_H_
 
 #include <ostream>
+#include <type_traits>
 
-#include "absl/meta/type_traits.h"
 #include "s2/s2cell_id.h"
 
 // Possible relationships between two S2CellIds in an index.
@@ -86,7 +86,7 @@ class S2CellIterator {
   //       "We require an object implementing the S2CellIterator API.");
   //   }
   template <typename T>
-  using ImplementedBy = std::is_convertible<absl::decay_t<T>*, S2CellIterator*>;
+  using ImplementedBy = std::is_convertible<std::decay_t<T>*, S2CellIterator*>;
 
   S2CellIterator() = default;
   virtual ~S2CellIterator() = default;

src/s2/s2cell_range_iterator.h

@@ -16,7 +16,7 @@
 #ifndef S2_S2CELL_RANGE_ITERATOR_H_
 #define S2_S2CELL_RANGE_ITERATOR_H_
 
-#include "absl/meta/type_traits.h"
+#include <type_traits>
 #include "s2/s2cell_id.h"
 #include "s2/s2cell_iterator.h"
 #include "s2/s2shape_index.h"
@@ -119,7 +119,7 @@ template <typename Iterator,
           typename std::enable_if<S2CellIterator::ImplementedBy<Iterator>{},
                                   bool>::type = true>
 auto MakeS2CellRangeIterator(Iterator&& iter) {
-  return S2CellRangeIterator<absl::decay_t<Iterator>>(
+  return S2CellRangeIterator<std::decay_t<Iterator>>(
       std::forward<Iterator>(iter));
 }
 

src/s2/s2lax_loop_shape.cc

@@ -35,12 +35,12 @@ using ReferencePoint = S2Shape::ReferencePoint;
 
 S2LaxLoopShape::S2LaxLoopShape(S2LaxLoopShape&& other)
     : S2Shape(std::move(other)),
-      num_vertices_(absl::exchange(other.num_vertices_, 0)),
+      num_vertices_(std::exchange(other.num_vertices_, 0)),
       vertices_(std::move(other.vertices_)) {}
 
 S2LaxLoopShape& S2LaxLoopShape::operator=(S2LaxLoopShape&& other) {
   S2Shape::operator=(static_cast<S2Shape&&>(other));
-  num_vertices_ = absl::exchange(other.num_vertices_, 0);
+  num_vertices_ = std::exchange(other.num_vertices_, 0);
   vertices_ = std::move(other.vertices_);
   return *this;
 }
@@ -92,14 +92,14 @@ S2Shape::ReferencePoint S2LaxLoopShape::GetReferencePoint() const {
 
 S2VertexIdLaxLoopShape::S2VertexIdLaxLoopShape(S2VertexIdLaxLoopShape&& other)
     : S2Shape(std::move(other)),
-      num_vertices_(absl::exchange(other.num_vertices_, 0)),
+      num_vertices_(std::exchange(other.num_vertices_, 0)),
       vertex_ids_(std::move(other.vertex_ids_)),
       vertex_array_(std::move(other.vertex_array_)) {}
 
 S2VertexIdLaxLoopShape& S2VertexIdLaxLoopShape::operator=(
     S2VertexIdLaxLoopShape&& other) {
   S2Shape::operator=(static_cast<S2Shape&&>(other));
-  num_vertices_ = absl::exchange(other.num_vertices_, 0);
+  num_vertices_ = std::exchange(other.num_vertices_, 0);
   vertex_ids_ = std::move(other.vertex_ids_);
   vertex_array_ = std::move(other.vertex_array_);
   return *this;

src/s2/s2lax_polygon_shape.cc

@@ -75,9 +75,9 @@ S2LaxPolygonShape::S2LaxPolygonShape(const S2Polygon& polygon) {
 
 S2LaxPolygonShape::S2LaxPolygonShape(S2LaxPolygonShape&& b)
     : S2Shape(std::move(b)),
-      num_loops_(absl::exchange(b.num_loops_, 0)),
+      num_loops_(std::exchange(b.num_loops_, 0)),
       prev_loop_(b.prev_loop_.exchange(0, std::memory_order_relaxed)),
-      num_vertices_(absl::exchange(b.num_vertices_, 0)),
+      num_vertices_(std::exchange(b.num_vertices_, 0)),
       vertices_(std::move(b.vertices_)),
       loop_starts_(std::move(b.loop_starts_)) {}
 
@@ -88,10 +88,10 @@ S2LaxPolygonShape& S2LaxPolygonShape::operator=(S2LaxPolygonShape&& b) {
   // move any of its private state.  This is a little odd since b is in a
   // half-moved state after calling but is ultimately safe.
   S2Shape::operator=(static_cast<S2Shape&&>(b));
-  num_loops_ = absl::exchange(b.num_loops_, 0);
+  num_loops_ = std::exchange(b.num_loops_, 0);
   prev_loop_.store(b.prev_loop_.exchange(0, memory_order_relaxed),
                    memory_order_relaxed);
-  num_vertices_ = absl::exchange(b.num_vertices_, 0);
+  num_vertices_ = std::exchange(b.num_vertices_, 0);
   vertices_ = std::move(b.vertices_);
   loop_starts_ = std::move(b.loop_starts_);
   return *this;
@@ -255,7 +255,7 @@ S2Shape::Chain S2LaxPolygonShape::chain(int i) const {
 
 EncodedS2LaxPolygonShape::EncodedS2LaxPolygonShape(EncodedS2LaxPolygonShape&& b)
     : S2Shape(std::move(b)),
-      num_loops_(absl::exchange(b.num_loops_, 0)),
+      num_loops_(std::exchange(b.num_loops_, 0)),
       prev_loop_(b.prev_loop_.exchange(0, std::memory_order_relaxed)),
       vertices_(std::move(b.vertices_)),
       loop_starts_(std::move(b.loop_starts_)) {}
@@ -266,7 +266,7 @@ EncodedS2LaxPolygonShape& EncodedS2LaxPolygonShape::operator=(
   // move any of its private state.  This is a little odd since b is in a
   // half-moved state after calling but is ultimately safe.
   S2Shape::operator=(static_cast<S2Shape&&>(b));
-  num_loops_ = absl::exchange(b.num_loops_, 0);
+  num_loops_ = std::exchange(b.num_loops_, 0);
   prev_loop_.store(b.prev_loop_.exchange(0, std::memory_order_relaxed),
                    std::memory_order_relaxed);
   vertices_ = std::move(b.vertices_);

src/s2/s2lax_polyline_shape.cc

@@ -37,12 +37,12 @@ using std::make_unique;
 
 S2LaxPolylineShape::S2LaxPolylineShape(S2LaxPolylineShape&& other)
     : S2Shape(std::move(other)),
-      num_vertices_(absl::exchange(other.num_vertices_, 0)),
+      num_vertices_(std::exchange(other.num_vertices_, 0)),
       vertices_(std::move(other.vertices_)) {}
 
 S2LaxPolylineShape& S2LaxPolylineShape::operator=(S2LaxPolylineShape&& other) {
   S2Shape::operator=(static_cast<S2Shape&&>(other));
-  num_vertices_ = absl::exchange(other.num_vertices_, 0);
+  num_vertices_ = std::exchange(other.num_vertices_, 0);
   vertices_ = std::move(other.vertices_);
   return *this;
 }

src/s2/s2loop.cc

@@ -110,8 +110,8 @@ S2Loop::S2Loop() {
 #ifndef SWIG
 S2Loop::S2Loop(S2Loop&& b)
     : S2Region(std::move(b)),
-      depth_(absl::exchange(b.depth_, 0)),
-      num_vertices_(absl::exchange(b.num_vertices_, 0)),
+      depth_(std::exchange(b.depth_, 0)),
+      num_vertices_(std::exchange(b.num_vertices_, 0)),
       vertices_(std::move(b.vertices_)),
       s2debug_override_(std::move(b.s2debug_override_)),
       origin_inside_(std::move(b.origin_inside_)),
@@ -129,8 +129,8 @@ S2Loop::S2Loop(S2Loop&& b)
 
 S2Loop& S2Loop::operator=(S2Loop&& b) {
   S2Region::operator=(static_cast<S2Region&&>(b));
-  depth_ = absl::exchange(b.depth_, 0);
-  num_vertices_ = absl::exchange(b.num_vertices_, 0);
+  depth_ = std::exchange(b.depth_, 0);
+  num_vertices_ = std::exchange(b.num_vertices_, 0);
   vertices_ = std::move(b.vertices_);
   s2debug_override_ = std::move(b.s2debug_override_);
   origin_inside_ = std::move(b.origin_inside_);

src/s2/s2polygon.cc

@@ -138,8 +138,8 @@ S2Polygon::S2Polygon(S2Polygon&& b)
       loops_(std::move(b.loops_)),
       s2debug_override_(std::move(b.s2debug_override_)),
       error_inconsistent_loop_orientations_(
-          absl::exchange(b.error_inconsistent_loop_orientations_, 0)),
-      num_vertices_(absl::exchange(b.num_vertices_, 0)),
+          std::exchange(b.error_inconsistent_loop_orientations_, 0)),
+      num_vertices_(std::exchange(b.num_vertices_, 0)),
       unindexed_contains_calls_(
           b.unindexed_contains_calls_.exchange(0, std::memory_order_relaxed)),
       bound_(std::move(b.bound_)),
@@ -161,8 +161,8 @@ S2Polygon& S2Polygon::operator=(S2Polygon&& b) {
   loops_ = std::move(b.loops_);
   s2debug_override_ = std::move(b.s2debug_override_);
   error_inconsistent_loop_orientations_ =
-      absl::exchange(b.error_inconsistent_loop_orientations_, 0);
-  num_vertices_ = absl::exchange(b.num_vertices_, 0);
+      std::exchange(b.error_inconsistent_loop_orientations_, 0);
+  num_vertices_ = std::exchange(b.num_vertices_, 0);
   unindexed_contains_calls_.store(
       b.unindexed_contains_calls_.exchange(0, std::memory_order_relaxed),
       std::memory_order_relaxed);

src/s2/s2polyline.cc

@@ -78,13 +78,13 @@ S2Polyline::S2Polyline()
 
 S2Polyline::S2Polyline(S2Polyline&& other)
   : s2debug_override_(other.s2debug_override_),
-    num_vertices_(absl::exchange(other.num_vertices_, 0)),
+    num_vertices_(std::exchange(other.num_vertices_, 0)),
     vertices_(std::move(other.vertices_)) {
 }
 
 S2Polyline& S2Polyline::operator=(S2Polyline&& other) {
   s2debug_override_ = other.s2debug_override_;
-  num_vertices_ = absl::exchange(other.num_vertices_, 0);
+  num_vertices_ = std::exchange(other.num_vertices_, 0);
   vertices_ = std::move(other.vertices_);
   return *this;
 }

src/s2/s2shape_index.h

@@ -305,7 +305,7 @@ class S2ShapeIndex {
   //       "We require an S2ShapeIndex.");
   //   }
   template <typename T>
-  using ImplementedBy = std::is_convertible<absl::decay_t<T>*, S2ShapeIndex*>;
+  using ImplementedBy = std::is_convertible<std::decay_t<T>*, S2ShapeIndex*>;
 
   virtual ~S2ShapeIndex() = default;
 

src/s2/util/coding/coder.cc

@@ -32,18 +32,18 @@
 #include "s2/base/port.h"
 
 Encoder::Encoder(Encoder&& other)
-    : buf_(absl::exchange(other.buf_, nullptr)),
-      limit_(absl::exchange(other.limit_, nullptr)),
-      underlying_buffer_(absl::exchange(other.underlying_buffer_, nullptr)),
-      orig_(absl::exchange(other.orig_, nullptr)) {}
+    : buf_(std::exchange(other.buf_, nullptr)),
+      limit_(std::exchange(other.limit_, nullptr)),
+      underlying_buffer_(std::exchange(other.underlying_buffer_, nullptr)),
+      orig_(std::exchange(other.orig_, nullptr)) {}
 
 Encoder& Encoder::operator=(Encoder&& other) {
   if (this == &other) return *this;
   if (ensure_allowed()) DeleteBuffer(underlying_buffer_, capacity());
-  buf_ = absl::exchange(other.buf_, nullptr);
-  limit_ = absl::exchange(other.limit_, nullptr);
-  underlying_buffer_ = absl::exchange(other.underlying_buffer_, nullptr);
-  orig_ = absl::exchange(other.orig_, nullptr);
+  buf_ = std::exchange(other.buf_, nullptr);
+  limit_ = std::exchange(other.limit_, nullptr);
+  underlying_buffer_ = std::exchange(other.underlying_buffer_, nullptr);
+  orig_ = std::exchange(other.orig_, nullptr);
   return *this;
 }
 

src/s2/util/coding/coder.h

@@ -158,7 +158,7 @@ class Encoder {
       S2_DCHECK_LE(enc->buf_, enc->limit_);
     }
 
-    char* skip(ptrdiff_t N) { return absl::exchange(p, p + N); }
+    char* skip(ptrdiff_t N) { return std::exchange(p, p + N); }
 
     void put8(unsigned char v) { *p++ = v; }
     void put16(uint16 v) { LittleEndian::Store16(skip(2), v); }
@@ -490,11 +490,11 @@ inline void DecoderExtensions::FillArray(Decoder* array, int num_decoders) {
   // This is an optimization based on the fact that Decoder(nullptr, 0) sets all
   // structure bytes to 0. This is valid because Decoder is TriviallyCopyable
   // (https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable).
-  static_assert(absl::is_trivially_copy_constructible<Decoder>::value,
+  static_assert(std::is_trivially_copy_constructible<Decoder>::value,
                 "Decoder must be trivially copy-constructible");
-  static_assert(absl::is_trivially_copy_assignable<Decoder>::value,
+  static_assert(std::is_trivially_copy_assignable<Decoder>::value,
                 "Decoder must be trivially copy-assignable");
-  static_assert(absl::is_trivially_destructible<Decoder>::value,
+  static_assert(std::is_trivially_destructible<Decoder>::value,
                 "Decoder must be trivially destructible");
 
   // For R On Windows, this line gives install warning