diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 9c91ab8..bf5c12a 100644 --- a/.github/workflows/test.yml +++ b/.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 diff --git a/.gitmodules b/.gitmodules index 0c2d89b..e69de29 100644 --- a/.gitmodules +++ b/.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 diff --git a/CMakeLists.txt b/CMakeLists.txt index b17c8a1..39c79dc 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,8 +1,7 @@ -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 ) @@ -10,17 +9,10 @@ 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_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_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_link_libraries(XrdS3Testing XrdS3Obj) + target_include_directories(XrdS3Testing INTERFACE ${XRootD_INCLUDE_DIRS}) add_library(XrdHTTPServerTesting SHARED "$") 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/ -#) diff --git a/cmake/FindFilesystem.cmake b/cmake/FindFilesystem.cmake new file mode 100644 index 0000000..f759f1e --- /dev/null +++ b/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 + #include + + 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() diff --git a/cmake/FindXrootd.cmake b/cmake/FindXrootd.cmake deleted file mode 100644 index 744de0c..0000000 --- a/cmake/FindXrootd.cmake +++ /dev/null @@ -1,39 +0,0 @@ -FIND_PATH(XROOTD_BIN xrootd-config - HINTS - ${XROOTD_DIR} - $ENV{XROOTD_DIR} - /usr - /opt/xrootd/ - PATH_SUFFIXES bin -) - -FIND_PATH(XROOTD_INCLUDES XrdVersion.hh - HINTS - ${XROOTD_DIR} - $ENV{XROOTD_DIR} - /usr - /opt/xrootd/ - PATH_SUFFIXES include/xrootd - PATHS /opt/xrootd -) - -FIND_LIBRARY(XROOTD_UTILS_LIB XrdUtils - HINTS - ${XROOTD_DIR} - $ENV{XROOTD_DIR} - /usr - /opt/xrootd/ - PATH_SUFFIXES lib -) - -FIND_LIBRARY(XROOTD_SERVER_LIB XrdServer - HINTS - ${XROOTD_DIR} - $ENV{XROOTD_DIR} - /usr - /opt/xrootd/ - PATH_SUFFIXES lib -) - -INCLUDE(FindPackageHandleStandardArgs) -FIND_PACKAGE_HANDLE_STANDARD_ARGS(Xrootd DEFAULT_MSG XROOTD_UTILS_LIB XROOTD_SERVER_LIB XROOTD_INCLUDES) diff --git a/src/HTTPFileSystem.cc b/src/HTTPFileSystem.cc index 7a5cbad..536cabe 100644 --- a/src/HTTPFileSystem.cc +++ b/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; diff --git a/src/HTTPFileSystem.hh b/src/HTTPFileSystem.hh index c888463..ac21e90 100644 --- a/src/HTTPFileSystem.hh +++ b/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, diff --git a/src/S3File.cc b/src/S3File.cc index 67a7f39..dec979e 100644 --- a/src/S3File.cc +++ b/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; diff --git a/src/S3FileSystem.cc b/src/S3FileSystem.cc index ae016fe..0765208 100644 --- a/src/S3FileSystem.cc +++ b/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."); diff --git a/src/S3FileSystem.hh b/src/S3FileSystem.hh index a7c45e2..ba5a223 100644 --- a/src/S3FileSystem.hh +++ b/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 diff --git a/src/stl_string_utils.cc b/src/stl_string_utils.cc index 9c3e3dc..09e4b10 100644 --- a/src/stl_string_utils.cc +++ b/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)) // '_.-~/' { diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 49676d6..09c30c9 100644 --- a/test/CMakeLists.txt +++ b/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} diff --git a/vendor/.clang-format b/vendor/.clang-format deleted file mode 100644 index 47a38a9..0000000 --- a/vendor/.clang-format +++ /dev/null @@ -1,2 +0,0 @@ -DisableFormat: true -SortIncludes: Never diff --git a/vendor/gtest b/vendor/gtest deleted file mode 160000 index 5a37b51..0000000 --- a/vendor/gtest +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 5a37b517ad4ab6738556f0284c256cae1466c5b4 diff --git a/vendor/tinyxml2 b/vendor/tinyxml2 deleted file mode 160000 index 312a809..0000000 --- a/vendor/tinyxml2 +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 312a8092245df393db14a0b2427457ed2ba75e1b