Merge pull request #79 from bbockelm/modernize_cmake

.github/workflows/test.yml

@@ -13,7 +13,7 @@ on:
 
 env:
   # Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.)
-  BUILD_TYPE: Release
+  BUILD_TYPE: Debug
 
 jobs:
   build:
@@ -52,7 +52,7 @@ jobs:
       # Note the current convention is to use the -S and -B options here to specify source
       # and build directories, but this is only available with CMake 3.13 and higher.
       # The CMake binaries on the Github Actions machines are (as of this writing) 3.12
-      run: cmake $GITHUB_WORKSPACE -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DXROOTD_PLUGINS_BUILD_UNITTESTS=yes -DXROOTD_PLUGINS_EXTERNAL_GTEST=${{ matrix.external-gtest }}
+      run: cmake $GITHUB_WORKSPACE -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DBUILD_TESTING=yes -DXROOTD_PLUGINS_EXTERNAL_GTEST=${{ matrix.external-gtest }}
 
     - name: Build
       working-directory: ${{runner.workspace}}/build

.gitmodules

@@ -1,7 +0,0 @@
-[submodule "vendor/gtest"]
-	path = vendor/gtest
-	url = https://github.com/google/googletest.git
-
-[submodule "vendor/tinyxml2"]
-	path = vendor/tinyxml2
-	url = https://github.com/leethomason/tinyxml2.git

CMakeLists.txt

@@ -1,26 +1,18 @@
-cmake_minimum_required( VERSION 3.13 )
+cmake_minimum_required( VERSION 3.14 )
 
 project( xrootd-http/s3 )
 
-option( XROOTD_PLUGINS_BUILD_UNITTESTS "Build the xrootd-s3-http unit tests" OFF )
 option( XROOTD_PLUGINS_EXTERNAL_GTEST "Use an external/pre-installed copy of GTest" OFF )
 option( VALGRIND "Run select unit tests under valgrind" OFF )
 option( ASAN "Build the plugin with the address sanitizer" OFF )
 
 set( CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake )
 set( CMAKE_BUILD_TYPE Debug )
 
-find_package( Xrootd REQUIRED )
+find_package( XRootD REQUIRED COMPONENTS UTILS SERVER )
 find_package( CURL REQUIRED )
 find_package( Threads REQUIRED )
-
-include (FindPkgConfig)
-pkg_check_modules(LIBCRYPTO REQUIRED libcrypto)
-
-if(NOT XROOTD_PLUGIN_VERSION)
-  exec_program(${XROOTD_BIN}/xrootd-config ARGS "--plugin-version" OUTPUT_VARIABLE XROOTD_PLUGIN_VERSION RETURN_VALUE RETVAR)
-  set(XROOTD_PLUGIN_VERSION ${XROOTD_PLUGIN_VERSION} CACHE INTERNAL "")
-endif()
+find_package( OpenSSL REQUIRED )
 
 if(VALGRIND)
   find_program(VALGRIND_BIN valgrind REQUIRED)
@@ -32,26 +24,10 @@ if(ASAN)
   set(CMAKE_LINKER_FLAGS "${CMAKE_LINKER_FLAGS} -fsanitize=address")
 endif()
 
-macro(use_cxx17)
-  if (CMAKE_VERSION VERSION_LESS "3.1")
-    if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
-      set (CMAKE_CXX_FLAGS "-std=c++17 ${CMAKE_CXX_FLAGS}")
-    endif ()
-  else ()
-    set (CMAKE_CXX_STANDARD 17)
-  endif ()
-
-  if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
-    if(CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 8.0)
-      if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS 9.0)
-        link_libraries(stdc++fs)
-      endif()
-    endif()
-  endif()
-endmacro(use_cxx17)
-use_cxx17()
+set( CMAKE_CXX_STANDARD 17 )
+set( CMAKE_CXX_STANDARD_REQUIRED ON )
 
-if( CMAKE_COMPILER_IS_GNUCXX )
+if( CMAKE_BUILD_TYPE STREQUAL Debug )
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Werror" )
 endif()
 
@@ -60,11 +36,28 @@ if(NOT APPLE)
   SET( CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} -Wl,--no-undefined")
 endif()
 
+# Our custom Filesystem module creates the std::filesystem library target for
+# any special dependencies needed for using std::filesystem.
+find_package( Filesystem REQUIRED )
+
 if( NOT XROOTD_EXTERNAL_TINYXML2 )
-	set(CMAKE_POSITION_INDEPENDENT_CODE ON)
-	add_subdirectory(vendor/tinyxml2)
+  include(FetchContent)
+  # Allow a locally-found tinyxml2 tarball to be used; provides the ability for packagers
+  # to build this without any external network connectivity (as in the case of Koji).
+  set( TINYXML2_URL "${CMAKE_CURRENT_SOURCE_DIR}/tinyxml2-10.0.0.tar.gz" )
+  if( NOT EXISTS "${TINYXML2_URL}" )
+    set( TINYXML2_URL "https://github.com/leethomason/tinyxml2/archive/refs/tags/10.0.0.tar.gz" )
+  endif()
+  cmake_policy( SET CMP0135 NEW )
+  FetchContent_Declare(
+    tinyxml2
+    URL "${TINYXML2_URL}"
+    URL_HASH SHA256=3bdf15128ba16686e69bce256cc468e76c7b94ff2c7f391cc5ec09e40bff3839
+    )
+  set(CMAKE_POSITION_INDEPENDENT_CODE ON CACHE INTERNAL "Force tinyxml2 to use PIC")
+  FetchContent_MakeAvailable( tinyxml2 )
 else()
-	find_package(tinyxml2)
+  find_package( tinyxml2 REQUIRED )
 endif()
 
 ##
@@ -76,38 +69,31 @@ endif()
 #
 add_definitions( -D_FILE_OFFSET_BITS=64 )
 
-include_directories(${XROOTD_INCLUDES} ${CURL_INCLUDE_DIRS} ${LIBCRYPTO_INCLUDE_DIRS})
-
 # On Linux, we hide all the symbols for the final libraries, exposing only what's needed for the XRootD
 # runtime loader.  So here we create the object library and will create a separate one for testing with
 # the symbols exposed.
 add_library(XrdS3Obj OBJECT src/CurlUtil.cc src/S3File.cc src/S3Directory.cc src/S3AccessInfo.cc src/S3FileSystem.cc src/AWSv4-impl.cc src/S3Commands.cc src/HTTPCommands.cc src/TokenFile.cc src/stl_string_utils.cc src/shortfile.cc src/logging.cc)
-target_include_directories(XrdS3Obj INTERFACE tinyxml2::tinyxml2)
 set_target_properties(XrdS3Obj PROPERTIES POSITION_INDEPENDENT_CODE ON)
-target_link_libraries(XrdS3Obj -ldl ${XROOTD_UTILS_LIB} ${XROOTD_SERVER_LIB} ${CURL_LIBRARIES} ${LIBCRYPTO_LIBRARIES} tinyxml2::tinyxml2 Threads::Threads)
+target_include_directories(XrdS3Obj PRIVATE ${XRootD_INCLUDE_DIRS})
+target_link_libraries(XrdS3Obj ${XRootD_UTILS_LIBRARIES} ${XRootD_SERVER_LIBRARIES} CURL::libcurl OpenSSL::Crypto tinyxml2::tinyxml2 Threads::Threads std::filesystem)
 
 add_library(XrdS3 MODULE "$<TARGET_OBJECTS:XrdS3Obj>")
 target_link_libraries(XrdS3 XrdS3Obj)
 
 add_library(XrdHTTPServerObj OBJECT src/CurlUtil.cc src/HTTPFile.cc src/HTTPFileSystem.cc src/HTTPCommands.cc src/TokenFile.cc src/stl_string_utils.cc src/shortfile.cc src/logging.cc)
 set_target_properties(XrdHTTPServerObj PROPERTIES POSITION_INDEPENDENT_CODE ON)
-target_link_libraries(XrdHTTPServerObj -ldl ${XROOTD_UTILS_LIB} ${XROOTD_SERVER_LIB} ${CURL_LIBRARIES} ${LIBCRYPTO_LIBRARIES} Threads::Threads)
+target_include_directories(XrdHTTPServerObj PRIVATE ${XRootD_INCLUDE_DIRS})
+target_link_libraries(XrdHTTPServerObj ${XRootD_UTILS_LIBRARIES} ${XRootD_SERVER_LIBRARIES} CURL::libcurl OpenSSL::Crypto Threads::Threads std::filesystem)
 
 add_library(XrdHTTPServer MODULE "$<TARGET_OBJECTS:XrdHTTPServerObj>")
 target_link_libraries(XrdHTTPServer XrdHTTPServerObj)
 
-# The CMake documentation strongly advises against using these macros; instead, the pkg_check_modules
-# is supposed to fill out the full path to ${LIBCRYPTO_LIBRARIES}.  As of cmake 3.26.1, this does not
-# appear to be the case on Mac OS X.  Remove once no longer necessary!
-target_link_directories(XrdS3Obj PUBLIC ${LIBCRYPTO_LIBRARY_DIRS})
-target_link_directories(XrdHTTPServerObj PUBLIC ${LIBCRYPTO_LIBRARY_DIRS})
-
 if(NOT APPLE)
-  set_target_properties(XrdS3 PROPERTIES OUTPUT_NAME "XrdS3-${XROOTD_PLUGIN_VERSION}" SUFFIX ".so" LINK_FLAGS "-Wl,--version-script=${CMAKE_SOURCE_DIR}/configs/export-lib-symbols")
-  set_target_properties(XrdHTTPServer PROPERTIES OUTPUT_NAME "XrdHTTPServer-${XROOTD_PLUGIN_VERSION}" SUFFIX ".so" LINK_FLAGS "-Wl,--version-script=${CMAKE_SOURCE_DIR}/configs/export-lib-symbols")
+  set_target_properties(XrdS3 PROPERTIES OUTPUT_NAME "XrdS3-${XRootD_PLUGIN_VERSION}" SUFFIX ".so" LINK_FLAGS "-Wl,--version-script=${CMAKE_SOURCE_DIR}/configs/export-lib-symbols")
+  set_target_properties(XrdHTTPServer PROPERTIES OUTPUT_NAME "XrdHTTPServer-${XRootD_PLUGIN_VERSION}" SUFFIX ".so" LINK_FLAGS "-Wl,--version-script=${CMAKE_SOURCE_DIR}/configs/export-lib-symbols")
 else()
-  set_target_properties(XrdS3 PROPERTIES OUTPUT_NAME "XrdS3-${XROOTD_PLUGIN_VERSION}" SUFFIX ".so")
-  set_target_properties(XrdHTTPServer PROPERTIES OUTPUT_NAME "XrdHTTPServer-${XROOTD_PLUGIN_VERSION}" SUFFIX ".so")
+  set_target_properties(XrdS3 PROPERTIES OUTPUT_NAME "XrdS3-${XRootD_PLUGIN_VERSION}" SUFFIX ".so")
+  set_target_properties(XrdHTTPServer PROPERTIES OUTPUT_NAME "XrdHTTPServer-${XRootD_PLUGIN_VERSION}" SUFFIX ".so")
 endif()
 
 SET(LIB_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/lib" CACHE PATH "Install path for libraries")
@@ -117,26 +103,30 @@ install(
   LIBRARY DESTINATION ${LIB_INSTALL_DIR}
 )
 
-if( XROOTD_PLUGINS_BUILD_UNITTESTS )
+if( BUILD_TESTING )
   add_library(XrdS3Testing SHARED "$<TARGET_OBJECTS:XrdS3Obj>")
   target_link_libraries(XrdS3Testing XrdS3Obj)
+  target_include_directories(XrdS3Testing INTERFACE ${XRootD_INCLUDE_DIRS})
   add_library(XrdHTTPServerTesting SHARED "$<TARGET_OBJECTS:XrdHTTPServerObj>")
   target_link_libraries(XrdHTTPServerTesting XrdHTTPServerObj)
+  target_include_directories(XrdHTTPServerTesting INTERFACE ${XRootD_INCLUDE_DIRS})
 
   if( NOT XROOTD_PLUGINS_EXTERNAL_GTEST )
-    include(ExternalProject)
-    ExternalProject_Add(gtest
-      PREFIX external/gtest
-      URL ${CMAKE_CURRENT_SOURCE_DIR}/vendor/gtest
-      BUILD_BYPRODUCTS external/gtest/src/gtest-build/lib/libgtest.a
-      INSTALL_COMMAND :
+    include( FetchContent )
+    set( GTEST_URL "${CMAKE_CURRENT_SOURCE_DIR}/googletest-1.15.2.tar.gz" )
+    if( NOT EXISTS "${GTEST_URL}" )
+        set( GTEST_URL "https://github.com/google/googletest/releases/download/v1.15.2/googletest-1.15.2.tar.gz" )
+    endif()
+    cmake_policy(SET CMP0135 NEW)
+    FetchContent_Declare(GTest
+      URL "${GTEST_URL}"
+      URL_HASH SHA256=7b42b4d6ed48810c5362c265a17faebe90dc2373c885e5216439d37927f02926
+      TEST_COMMAND ""
     )
+    FetchContent_MakeAvailable( GTest )
+  else()
+    find_package(GTest REQUIRED)
   endif()
   enable_testing()
   add_subdirectory(test)
 endif()
-
-#install(
-#  FILES ${CMAKE_SOURCE_DIR}/configs/60-s3.cfg
-#  DESTINATION ${CMAKE_INSTALL_PREFIX}/etc/xrootd/config.d/
-#)

cmake/FindFilesystem.cmake

@@ -0,0 +1,45 @@
+
+# Ideas come from
+#
+#  https://gitlab.kitware.com/cmake/cmake/-/issues/17834
+#
+# Basically, upstream CMake claims the fact that a separate library is
+# needed for std::filesystem support is a short-lived fact (of all the
+# platforms we use, only RHEL 8 uses a compiler where this is needed),
+# hence they don't want a standardized way to detect std::filesystem
+
+include(CheckSourceCompiles)
+
+set( CMAKE_REQUIRED_INCLUDES "${XRootD_INCLUDE_DIR}" )
+set( SAMPLE_FILESYSTEM "#include <cstdlib>
+        #include <filesystem>
+
+        int main() {
+            auto cwd = std::filesystem::current_path();
+            return cwd.empty();
+        }")
+
+
+CHECK_SOURCE_COMPILES( CXX "${SAMPLE_FILESYSTEM}" CXX_FILESYSTEM_NO_LINK_NEEDED )
+
+set( _found FALSE )
+if( CXX_FILESYSTEM_NO_LINK_NEEDED )
+  set( _found TRUE )
+else()
+  # Add the libstdc++ flag
+  set( CMAKE_REQUIRED_LIBRARIES "-lstdc++fs" )
+  CHECK_SOURCE_COMPILES( CXX "${SAMPLE_FILESYSTEM}" CXX_FILESYSTEM_STDCPPFS_NEEDED )
+  set( _found TRUE )
+endif()
+
+add_library( std::filesystem INTERFACE IMPORTED )
+#set_property( TARGET std::filesystem APPEND PROPERTY INTERFACE_COMPILE_FEATURES cxx_std_17 )
+
+if( CXX_FILESYSTEM_STDCPPFS_NEEDED )
+  set_property( TARGET std::filesystem APPEND PROPERTY INTERFACE_LINK_LIBRARIES -lstdc++fs )
+endif()
+
+set( Filesystem_FOUND ${_found} CACHE BOOL "TRUE if we can run a program using std::filesystem" FORCE )
+if( Filesystem_FIND_REQUIRED AND NOT Filesystem_FOUND )
+    message( FATAL_ERROR "Cannot run simple program using std::filesystem" )
+endif()

src/HTTPFileSystem.cc

@@ -41,8 +41,8 @@
 using namespace XrdHTTPServer;
 
 HTTPFileSystem::HTTPFileSystem(XrdSysLogger *lp, const char *configfn,
-							   XrdOucEnv *envP)
-	: m_env(envP), m_log(lp, "httpserver_"), m_token("", &m_log) {
+							   XrdOucEnv * /*envP*/)
+	: m_log(lp, "httpserver_"), m_token("", &m_log) {
 	m_log.Say("------ Initializing the HTTP filesystem plugin.");
 	if (!Config(lp, configfn)) {
 		throw std::runtime_error("Failed to configure HTTP filesystem plugin.");
@@ -130,7 +130,7 @@ bool HTTPFileSystem::Config(XrdSysLogger *lp, const char *configfn) {
 	}
 
 	if (!token_file.empty()) {
-		m_token = std::move(TokenFile(token_file, &m_log));
+		m_token = TokenFile(token_file, &m_log);
 	}
 
 	return true;

src/HTTPFileSystem.hh

@@ -109,7 +109,6 @@ class HTTPFileSystem : public XrdOss {
 	const TokenFile *getToken() const { return &m_token; }
 
   protected:
-	XrdOucEnv *m_env;
 	XrdSysError m_log;
 
 	bool handle_required_config(const std::string &name_from_config,

src/S3File.cc

@@ -758,7 +758,6 @@ ssize_t S3File::S3Cache::Read(S3File &file, char *buffer, off_t offset,
 		m_partial_hit_count += 1;
 	}
 	m_miss_bytes += miss_bytes;
-	unsigned fetch_attempts = 0;
 	while (req1_off != -1) {
 		std::unique_lock lk(m_mutex);
 		m_cv.wait(lk, [&] {
@@ -869,7 +868,6 @@ ssize_t S3File::S3Cache::Read(S3File &file, char *buffer, off_t offset,
 		// No caches serve our requests - we must kick off a new download
 		// std::cout << "Will download data via cache; req1 offset=" << req1_off
 		// << ", req2 offset=" << req2_off << "\n";
-		fetch_attempts++;
 		bool download_a = false, download_b = false, prefetch_b = false;
 		if (!m_a.m_inprogress && m_b.m_inprogress) {
 			m_a.m_off = req1_off / m_cache_entry_size * m_cache_entry_size;

src/S3FileSystem.cc

@@ -45,8 +45,8 @@ bool S3FileSystem::m_dir_marker = true;
 std::string S3FileSystem::m_dir_marker_name = ".pelican_dir_marker";
 
 S3FileSystem::S3FileSystem(XrdSysLogger *lp, const char *configfn,
-						   XrdOucEnv *envP)
-	: m_env(envP), m_log(lp, "s3_") {
+						   XrdOucEnv * /*envP*/)
+	: m_log(lp, "s3_") {
 	m_log.Say("------ Initializing the S3 filesystem plugin.");
 	if (!Config(lp, configfn)) {
 		throw std::runtime_error("Failed to configure S3 filesystem plugin.");

src/S3FileSystem.hh

@@ -138,7 +138,6 @@ class S3FileSystem : public XrdOss {
 	getS3AccessInfo(const std::string &exposedPath, std::string &object) const;
 
   private:
-	XrdOucEnv *m_env;
 	XrdSysError m_log;
 
 	// The filesystem logic can test for an empty object to see if there's

src/stl_string_utils.cc

@@ -151,9 +151,9 @@ std::string urlquote(const std::string input) {
 	std::string output;
 	output.reserve(3 * input.size());
 	for (char val : input) {
-		if ((val >= 48 || val <= 57) ||	 // Digits 0-9
-			(val >= 65 || val <= 90) ||	 // Uppercase A-Z
-			(val >= 97 || val <= 122) || // Lowercase a-z
+		if ((val >= 48 && val <= 57) ||	 // Digits 0-9
+			(val >= 65 && val <= 90) ||	 // Uppercase A-Z
+			(val >= 97 && val <= 122) || // Lowercase a-z
 			(val == 95 || val == 46 || val == 45 || val == 126 ||
 			 val == 47)) // '_.-~/'
 		{

test/CMakeLists.txt

@@ -1,23 +1,13 @@
-add_executable( s3-gtest s3_tests.cc )
-
-add_executable( s3-unit-test s3_unit_tests.cc )
-
-add_executable( http-gtest http_tests.cc )
 
 include(GoogleTest)
 
-if(XROOTD_PLUGINS_EXTERNAL_GTEST)
-    set(LIBGTEST "gtest")
-else()
-    add_dependencies(s3-gtest gtest)
-    add_dependencies(http-gtest gtest)
-    include_directories("${PROJECT_SOURCE_DIR}/vendor/gtest/googletest/include")
-    set(LIBGTEST "${CMAKE_BINARY_DIR}/external/gtest/src/gtest-build/lib/libgtest.a")
-endif()
+add_executable( s3-gtest s3_tests.cc )
+add_executable( s3-unit-test s3_unit_tests.cc )
+add_executable( http-gtest http_tests.cc )
 
-target_link_libraries(s3-gtest XrdS3Testing "${LIBGTEST}" pthread)
-target_link_libraries(s3-unit-test XrdS3Testing "${LIBGTEST}" pthread)
-target_link_libraries(http-gtest XrdHTTPServerTesting "${LIBGTEST}" pthread)
+target_link_libraries(s3-gtest XrdS3Testing GTest::gtest_main Threads::Threads)
+target_link_libraries(s3-unit-test XrdS3Testing GTest::gtest_main Threads::Threads)
+target_link_libraries(http-gtest XrdHTTPServerTesting GTest::gtest_main Threads::Threads)
 
 gtest_add_tests(TARGET s3-unit-test TEST_LIST s3UnitTests)
 set_tests_properties(${s3UnitTests}