Breaking change: Flip default behavior for handling cmake dependencies.

Instead of fetching dependencies by default, we will first look for a local installation and only fetch as a fallback. Two new options are added for forcing either of these behaviors. protobuf_FORCE_FETCH_DEPENDENCIES will always fetch dependencies, and protobuf_PREVENT_FETCH_DEPENDENCIES will never do so. #test-continuous PiperOrigin-RevId: 693898394
protocolbuffers · Nov 7, 2024 · 9cc685e · 9cc685e
1 parent a59cfa4
commit 9cc685e
Show file tree

Hide file tree

Showing 9 changed files with 114 additions and 111 deletions.
diff --git a/.github/workflows/test_cpp.yml b/.github/workflows/test_cpp.yml
@@ -141,17 +141,19 @@ jobs:
       fail-fast: false   # Don't cancel all jobs if one fails.
       matrix:
         include:
-          - flags: -Dprotobuf_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=17  -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF
+          - flags: -Dprotobuf_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=17
           - name: Ninja
-            flags: -G Ninja -DCMAKE_CXX_STANDARD=17 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF
+            flags: -G Ninja -DCMAKE_CXX_STANDARD=17
             continuous-only: true
           - name: Shared
-            flags: -Dprotobuf_BUILD_SHARED_LIBS=ON -Dprotobuf_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=17 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF
+            flags: -Dprotobuf_BUILD_SHARED_LIBS=ON -Dprotobuf_BUILD_EXAMPLES=ON -DCMAKE_CXX_STANDARD=17
             continuous-only: true
           - name: C++20
-            flags: -DCMAKE_CXX_STANDARD=20 -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF
+            flags: -DCMAKE_CXX_STANDARD=20
+          - name: Package
+            flags: -DCMAKE_CXX_STANDARD=17 -Dprotobuf_LOCAL_DEPENDENCIES_ONLY=ON
           - name: Fetch
-            flags: -DCMAKE_CXX_STANDARD=17
+            flags: -DCMAKE_CXX_STANDARD=17 -Dprotobuf_FORCE_FETCH_DEPENDENCIES=ON
 
     name: ${{ matrix.continuous-only && inputs.continuous-prefix || '' }} Linux CMake ${{ matrix.name}}
     runs-on: ubuntu-latest
@@ -188,9 +190,10 @@ jobs:
           # Set defaults
           - type: package
             name: Install
-            flags: -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF
+            flags: -Dprotobuf_LOCAL_DEPENDENCIES_ONLY=ON
           - type: fetch
             name: Install (Fetch)
+            flags: -Dprotobuf_FORCE_FETCH_DEPENDENCIES=ON
             continuous-only: true
     name: ${{ matrix.continuous-only && inputs.continuous-prefix || '' }}Linux CMake ${{ matrix.name }}
     runs-on: ubuntu-latest
@@ -252,7 +255,7 @@ jobs:
           credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }}
           command: >-
             /install.sh -DCMAKE_CXX_STANDARD=17 ${{ env.SCCACHE_CMAKE_FLAGS }}
-            -Dprotobuf_USE_EXTERNAL_GTEST=ON -Dprotobuf_FETCH_DEPENDENCIES=OFF
+            -Dprotobuf_LOCAL_DEPENDENCIES_ONLY=OFF
             -Dprotobuf_BUILD_EXAMPLES=OFF \&\&
             mkdir examples/build \&\&
             cd examples/build \&\&
@@ -320,7 +323,7 @@ jobs:
       - name: Run tests
         uses: protocolbuffers/protobuf-ci/docker@v3
         with:
-          image: us-docker.pkg.dev/protobuf-build/containers/test/linux/32bit@sha256:56548bef786201330017eae685cc3d2fdb564fd2ca3b88e30e28d84572e4c5dd
+          image: us-docker.pkg.dev/protobuf-build/containers/test/linux/32bit@sha256:d6028ab408c49932836cdc514116f06886d7f6868a4d430630aa52adc5aee2fc
           platform: linux/386
           credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }}
           command: >-
@@ -426,8 +429,7 @@ jobs:
           - name: Windows CMake Install
             os: windows-2022
             install-flags: >-
-              -G Ninja -Dprotobuf_WITH_ZLIB=OFF
-              -Dprotobuf_BUILD_CONFORMANCE=OFF -Dprotobuf_BUILD_TESTS=OFF
+              -G Ninja -Dprotobuf_WITH_ZLIB=OFF -Dprotobuf_BUILD_CONFORMANCE=OFF
             flags: >-
               -G Ninja -Dprotobuf_WITH_ZLIB=OFF -Dprotobuf_BUILD_CONFORMANCE=OFF
               -Dprotobuf_REMOVE_INSTALLED_HEADERS=ON

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -35,6 +35,8 @@ option(protobuf_BUILD_LIBUPB "Build libupb" ON)
 option(protobuf_DISABLE_RTTI "Remove runtime type information in the binaries" OFF)
 option(protobuf_TEST_XML_OUTDIR "Output directory for XML logs from tests." "")
 option(protobuf_ALLOW_CCACHE "Adjust build flags to allow for ccache support." OFF)
+option(protobuf_FORCE_FETCH_DEPENDENCIES "Force all dependencies to be downloaded from GitHub.  Local installations will be ignored." OFF)
+option(protobuf_LOCAL_DEPENDENCIES_ONLY "Prevent downloading any dependencies from GitHub. If this option is set, the dependency must be available locally as an installed package." OFF)
 
 # We support Unity (Jumbo) builds best-effort.
 option(protobuf_USE_UNITY_BUILD "Enable Unity (Jumbo) build for" OFF)
@@ -106,6 +108,10 @@ string(REGEX REPLACE     "${protobuf_VERSION_REGEX}" "\\3"
 string(REGEX REPLACE     "${protobuf_VERSION_REGEX}" "\\5"
   protobuf_VERSION_PRERELEASE "${protobuf_VERSION_STRING}")
 
+if (protobuf_FORCE_FETCH_DEPENDENCIES AND protobuf_LOCAL_DEPENDENCIES_ONLY)
+  message(FATAL_ERROR "Conflicting options protobuf_FORCE_FETCH_DEPENDENCIES and protobuf_LOCAL_DEPENDENCIES_ONLY both set")
+endif()
+
 # Package version
 set(protobuf_VERSION
   "${protobuf_VERSION_MINOR}.${protobuf_VERSION_PATCH}")
@@ -260,14 +266,6 @@ include_directories(
   ${protobuf_BINARY_DIR}/src
   ${protobuf_SOURCE_DIR}/src)
 
-set(protobuf_FETCH_DEPENDENCIES ON CACHE BOOL "Allow downloading dependencies from GitHub. If this option is not set, the dependency must be available locally as an installed package.")
-
-set(protobuf_ABSL_PROVIDER "fetch" CACHE STRING "Provider of absl library.  `fetch` downloads from GitHub and `package` searches for a local installation")
-set_property(CACHE protobuf_ABSL_PROVIDER PROPERTY STRINGS "package" "fetch")
-
-set(protobuf_JSONCPP_PROVIDER "fetch" CACHE STRING "Provider of jsoncpp library.  `fetch` downloads from GitHub and `package` searches for a local installation")
-set_property(CACHE protobuf_JSONCPP_PROVIDER PROPERTY STRINGS "package" "fetch")
-
 if (protobuf_BUILD_TESTS)
   include(${protobuf_SOURCE_DIR}/cmake/gtest.cmake)
 endif (protobuf_BUILD_TESTS)

diff --git a/cmake/README.md b/cmake/README.md
@@ -110,19 +110,23 @@ Create a temporary *build* folder and change your working directory to it:
      C:\Path\to\build\protobuf>
 
 During configuration you will also be specifying where CMake should expect to
-find your Abseil installation. To do so, first set `-Dprotobuf_ABSL_PROVIDER=package`
-and then set `-DCMAKE_PREFIX_PATH` to the path where you installed Abseil.
+find your Abseil installation. To do so, set `-DCMAKE_PREFIX_PATH` to the path
+where you installed Abseil.
 
 For example:
 
 ```console
 C:\Path\to\build\protobuf> cmake -S. -Bcmake-out \
                            -DCMAKE_INSTALL_PREFIX=/tmp/protobuf \
                            -DCMAKE_CXX_STANDARD=14 \
-                           -Dprotobuf_ABSL_PROVIDER=package \
                            -DCMAKE_PREFIX_PATH=/tmp/absl  # Path to where I installed Abseil
 ```
 
+If the installation of a dependency can't be found, CMake will default to
+downloading and building a copy from GitHub. To prevent this and make it an
+error condition, you can optionally set
+`-Dprotobuf_LOCAL_DEPENDENCIES_ONLY=ON`.
+
 The *Makefile* and *Ninja* generators can build the project in only one configuration, so you need to build
 a separate folder for each configuration.
 
@@ -155,9 +159,8 @@ will be downloaded during CMake configuration.
 Alternately, you may want to use protobuf in a larger set-up, you may want to use that standard CMake approach where
 you build and install a shared copy of Google Test.
 
-After you've built and installed your Google Test copy, you need add the following definition to your *cmake* command line
-during the configuration step: `-Dprotobuf_USE_EXTERNAL_GTEST=ON`.
-This will cause the standard CMake `find_package(GTest REQUIRED)` to be used.
+After you've built and installed your Google Test copy, the standard CMake
+`find_package(GTest)` will use it.
 
 [find_package](https://cmake.org/cmake/help/latest/command/find_package.html) will search in a default location,
 which on Windows is *C:\Program Files*. This is most likely not what you want. You will want instead to search for

diff --git a/cmake/abseil-cpp.cmake b/cmake/abseil-cpp.cmake
@@ -10,27 +10,36 @@ if(protobuf_BUILD_TESTS)
   set(ABSL_FIND_GOOGLETEST OFF)
 endif()
 
-if(TARGET absl::strings)
-  # If Abseil is included already, skip including it.
-  # (https://github.com/protocolbuffers/protobuf/issues/10435)
-elseif (protobuf_FETCH_DEPENDENCIES AND protobuf_ABSL_PROVIDER STREQUAL "fetch")
-  include(${protobuf_SOURCE_DIR}/cmake/dependencies.cmake)
-  include(FetchContent)
-  FetchContent_Declare(
-    absl
-    GIT_REPOSITORY "https://github.com/abseil/abseil-cpp.git"
-    GIT_TAG "${abseil-cpp-version}"
-  )
-  if(protobuf_INSTALL)
-    # When protobuf_INSTALL is enabled and Abseil will be built as a module,
-    # Abseil will be installed along with protobuf for convenience.
-    set(ABSL_ENABLE_INSTALL ON)
+if (NOT TARGET absl::strings)
+  if (NOT protobuf_FORCE_FETCH_DEPENDENCIES)
+    # Use "CONFIG" as there is no built-in cmake module for absl.
+    find_package(absl CONFIG)
+  endif()
+
+  # Fallback to fetching Abseil from github if it's not found locally.
+  if (NOT absl_FOUND AND NOT protobuf_LOCAL_DEPENDENCIES_ONLY)
+    include(${protobuf_SOURCE_DIR}/cmake/dependencies.cmake)
+    message(STATUS "Fallback to downloading Abseil ${abseil-cpp-version} from GitHub")
+
+    include(FetchContent)
+    FetchContent_Declare(
+      absl
+      GIT_REPOSITORY "https://github.com/abseil/abseil-cpp.git"
+      GIT_TAG "${abseil-cpp-version}"
+    )
+    if (protobuf_INSTALL)
+      # When protobuf_INSTALL is enabled and Abseil will be built as a module,
+      # Abseil will be installed along with protobuf for convenience.
+      set(ABSL_ENABLE_INSTALL ON)
+    endif()
+    FetchContent_MakeAvailable(absl)
   endif()
-  FetchContent_MakeAvailable(absl)
-else ()
-  # Use "CONFIG" as there is no built-in cmake module for absl.
-  find_package(absl REQUIRED CONFIG)
 endif()
+
+if (NOT TARGET absl::strings)
+  message(FATAL_ERROR "Cannot find abseil-cpp dependency that's needed to build protobuf.\n")
+endif()
+
 set(_protobuf_FIND_ABSL "if(NOT TARGET absl::strings)\n  find_package(absl CONFIG)\nendif()")
 
 if (BUILD_SHARED_LIBS AND MSVC)
@@ -41,13 +50,8 @@ if (BUILD_SHARED_LIBS AND MSVC)
   # Once https://github.com/abseil/abseil-cpp/pull/1466 is merged and released
   # in the minimum version of abseil required by protobuf, it is possible to
   # always link absl::abseil_dll and absl::abseil_test_dll and remove the if
-  if(protobuf_ABSL_PROVIDER STREQUAL "package")
-    set(protobuf_ABSL_USED_TARGETS absl::abseil_dll)
-    set(protobuf_ABSL_USED_TEST_TARGETS absl::abseil_test_dll)
-  else()
-    set(protobuf_ABSL_USED_TARGETS abseil_dll)
-    set(protobuf_ABSL_USED_TEST_TARGETS abseil_test_dll)
-  endif()
+  set(protobuf_ABSL_USED_TARGETS absl::abseil_dll)
+  set(protobuf_ABSL_USED_TEST_TARGETS absl::abseil_test_dll)
 else()
   set(protobuf_ABSL_USED_TARGETS
     absl::absl_check

diff --git a/cmake/conformance.cmake b/cmake/conformance.cmake
@@ -1,19 +1,31 @@
 # Don't run jsoncpp tests.
 set(JSONCPP_WITH_TESTS OFF)
 
-if (TARGET jsoncpp_lib)
-  # jsoncpp is already present.
-elseif (protobuf_FETCH_DEPENDENCIES AND protobuf_JSONCPP_PROVIDER STREQUAL "fetch")
-  include(${protobuf_SOURCE_DIR}/cmake/dependencies.cmake)
-  include(FetchContent)
-  FetchContent_Declare(
-    jsoncpp
-    GIT_REPOSITORY "https://github.com/open-source-parsers/jsoncpp.git"
-    GIT_TAG "${jsoncpp-version}"
-  )
-  FetchContent_MakeAvailable(jsoncpp)
-else ()
-  find_package(jsoncpp REQUIRED)
+if (NOT TARGET jsoncpp_lib)
+  if (NOT protobuf_FORCE_FETCH_DEPENDENCIES)
+    find_package(jsoncpp)
+  endif()
+
+  # Fallback to fetching Googletest from github if it's not found locally.
+  if (NOT jsoncpp_FOUND AND NOT protobuf_LOCAL_DEPENDENCIES_ONLY)
+    include(${protobuf_SOURCE_DIR}/cmake/dependencies.cmake)
+    message(STATUS "Fallback to downloading jsoncpp ${jsoncpp-version} from GitHub")
+
+    include(FetchContent)
+    FetchContent_Declare(
+      jsoncpp
+      GIT_REPOSITORY "https://github.com/open-source-parsers/jsoncpp.git"
+      GIT_TAG "${jsoncpp-version}"
+    )
+    FetchContent_MakeAvailable(jsoncpp)
+  endif()
+endif()
+
+if (NOT TARGET jsoncpp_lib)
+  message(FATAL_ERROR
+          "Cannot find jsoncpp dependency that's needed to build conformance tests.\n"
+          "If instead you want to skip these tests, run cmake with:\n"
+          "  cmake -Dprotobuf_BUILD_CONFORMANCE=OFF\n")
 endif()
 
 file(MAKE_DIRECTORY ${protobuf_BINARY_DIR}/conformance)
@@ -137,10 +149,6 @@ add_test(NAME conformance_cpp_test
   DEPENDS conformance_test_runner conformance_cpp)
 
 set(JSONCPP_WITH_TESTS OFF CACHE BOOL "Disable tests")
-if(NOT protobuf_FETCH_DEPENDENCIES AND protobuf_JSONCPP_PROVIDER STREQUAL "module")
-  add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/third_party/jsoncpp third_party/jsoncpp)
-  target_include_directories(conformance_test_runner PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/third_party/jsoncpp/include)
-endif()
 
 if(BUILD_SHARED_LIBS)
   target_link_libraries(conformance_test_runner jsoncpp_lib)

diff --git a/cmake/gtest.cmake b/cmake/gtest.cmake
@@ -1,25 +1,28 @@
-option(protobuf_USE_EXTERNAL_GTEST "Use external Google Test (i.e. not the one in third_party/googletest)" OFF)
+if (NOT TARGET GTest::gmock)
+  if (NOT protobuf_FORCE_FETCH_DEPENDENCIES)
+    find_package(GTest CONFIG)
+  endif()
+
+  # Fallback to fetching Googletest from github if it's not found locally.
+  if (NOT GTest_FOUND AND NOT protobuf_LOCAL_DEPENDENCIES_ONLY)
+    include(${protobuf_SOURCE_DIR}/cmake/dependencies.cmake)
+    message(STATUS "Fallback to downloading GTest ${googletest-version} from GitHub")
 
-if (TARGET GTest::gmock)
-  # GTest is already present.
-elseif (protobuf_USE_EXTERNAL_GTEST)
-  find_package(GTest REQUIRED CONFIG)
-else ()
-  if (NOT protobuf_FETCH_DEPENDENCIES)
-    message(FATAL_ERROR
-            "Cannot find local googletest directory that's needed to "
-            "build tests.\n"
-            "If instead you want to skip tests, run cmake with:\n"
-            "  cmake -Dprotobuf_BUILD_TESTS=OFF\n")
+    include(FetchContent)
+    FetchContent_Declare(
+      googletest
+      GIT_REPOSITORY "https://github.com/google/googletest.git"
+      GIT_TAG "v${googletest-version}"
+    )
+    # Due to https://github.com/google/googletest/issues/4384, we can't name this
+    # GTest for use with find_package until 1.15.0.
+    FetchContent_MakeAvailable(googletest)
   endif()
-  include(${protobuf_SOURCE_DIR}/cmake/dependencies.cmake)
-  include(FetchContent)
-  FetchContent_Declare(
-    googletest
-    GIT_REPOSITORY "https://github.com/google/googletest.git"
-    GIT_TAG "v${googletest-version}"
-  )
-  # Due to https://github.com/google/googletest/issues/4384, we can't name this
-  # GTest for use with find_package until 1.15.0.
-  FetchContent_MakeAvailable(googletest)
+endif()
+
+if (NOT TARGET GTest::gmock)
+  message(FATAL_ERROR
+          "Cannot find googletest dependency that's needed to build tests.\n"
+          "If instead you want to skip tests, run cmake with:\n"
+          "  cmake -Dprotobuf_BUILD_TESTS=OFF\n")
 endif()
diff --git a/csharp/src/Google.Protobuf/Reflection/FeatureSetDescriptor.g.cs b/csharp/src/Google.Protobuf/Reflection/FeatureSetDescriptor.g.cs
diff --git a/src/google/protobuf/io/BUILD.bazel b/src/google/protobuf/io/BUILD.bazel
@@ -224,6 +224,7 @@ cc_test(
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/strings:cord",
         "@com_google_absl//absl/strings:str_format",
+        "@com_google_absl//absl/synchronization",
         "@com_google_absl//absl/types:optional",
         "@com_google_googletest//:gtest",
         "@com_google_googletest//:gtest_main",

diff --git a/src/google/protobuf/io/zero_copy_stream_unittest.cc b/src/google/protobuf/io/zero_copy_stream_unittest.cc
@@ -53,6 +53,7 @@
 #include "absl/strings/cord_buffer.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/string_view.h"
+#include "absl/synchronization/mutex.h"
 #include "google/protobuf/io/coded_stream.h"
 #include "google/protobuf/io/io_win32.h"
 #include "google/protobuf/io/zero_copy_stream_impl.h"
@@ -1446,14 +1447,14 @@ TEST_F(IoTest, NonBlockingFileIo) {
       ASSERT_EQ(fcntl(fd[0], F_SETFL, O_NONBLOCK), 0);
       ASSERT_EQ(fcntl(fd[1], F_SETFL, O_NONBLOCK), 0);
 
-      std::mutex go_write;
-      go_write.lock();
+      absl::Mutex go_write;
+      go_write.Lock();
 
       bool done_reading = false;
 
       std::thread write_thread([this, fd, &go_write, i]() {
-        go_write.lock();
-        go_write.unlock();
+        go_write.Lock();
+        go_write.Unlock();
         FileOutputStream output(fd[1], kBlockSizes[i]);
         WriteStuff(&output);
         EXPECT_EQ(0, output.GetErrno());
@@ -1472,7 +1473,7 @@ TEST_F(IoTest, NonBlockingFileIo) {
       // reading thread waits for the data to be available before returning.
       std::this_thread::sleep_for(std::chrono::milliseconds(100));
       EXPECT_FALSE(done_reading);
-      go_write.unlock();
+      go_write.Unlock();
       write_thread.join();
       read_thread.join();
       EXPECT_TRUE(done_reading);