Avoid linking std <filesystem> library if not needed

ts826848 commented 7 months ago

(GitHub kind of mangled my commit message. Tried to fix it up, so hopefully it isn't totally illegible)

Hopefully fixes #982

Clang and GCC both originally required users to link an additional library to be able to use most (all?) of the header. This support was later folded into the main stdlib libraries when the respective implementations stabilized.

cpr takes this into account, but it currently only supports linking in libstdc++fs, which is the library for GCC's stdlib, libstdc++. In addition, sometimes libstdc++fs is requested even when doing so is neither required nor supported (e.g., current Clang on macOS, which uses libc++ and does not require linking in an additional library for ), which can result in compilation errors due to being unable to find the support library[0].

This commit changes support handling to avoid requesting the support library when it's not needed and to try to link in the correct support library when required. The change itself is fairly straightforwards, albeit rather verbose.

The new support handling was tested using GCC 8, Clang 8, GCC 13, and Clang 17. Clang 8 was also tested using GCC 8's libstdc++. GCC 8 and Clang 8 are the last releases where support was not baked into the main stdlib implementation, and GCC 13 and Clang 17 are the most recent releases as of this commit.

The testing was done by using a driver script[1] to attempt to configure the project using the tested compilers and by adding the following CMake output after the new filesystem support handling to show the outcome of the try_compiles():

message(FATAL_ERROR "  CMAKE_BINARY_DIR = ${CMAKE_BINARY_DIR}\n"
                    "  STD_FS_SUPPORTED = ${STD_FS_SUPPORTED}\n"
                    "STDCXXFS_SUPPORTED = ${STDCXXFS_SUPPORTED}\n"
                    "   CXXFS_SUPPORTED = ${CXXFS_SUPPORTED}\n")

The test program used to test for support turned out to be surprisingly simple. Some manual testing showed that calling std::filesystem::current_path() requires linking in the additional filesystem support library on Clang 8[2] or GCC 8[3] while Clang 17 and GCC 13 do not require the additional library and compile the test program with no complaints. This works perfectly for testing for support.

Handling was tested on macOS, so there are some quirks in the driver script that may not be required and/or sufficient to do something similar on other platforms:

Both Clang 8 and GCC 8 fail CMake's compiler sanity checks because CMake passes -mmacosx-version-min= (12.6 in my case) by default. Clang 8 rejects the version number[4], while GCC 8 appears to fall back to some old version and tries to link in a library that isn't available[5]. Passing -DCMAKE_OSX_DEPLOYMENT_TARGET="10.12" or some other sufficiently old version allows the compiler checks to pass.
When using libc++, both Clang 8 and 17 will link the system libc++ if I don't explicitly tell the linker to search the version-specific lib directory first. This causes incorrect test results for Clang 8 since the system libc++ includes support. This can be fixed by passing -DCMAKE_CXX_FLAGS="-L", but that in turn causes the libc++fs check to fail. This is because CMake appears to first compile the test file to an object file, then links the object file. The linker flag is unused for the first step, which triggers -Wunused-command-line-argument, which is then turned into an error through -Werror. This can be bypassed by passing -Wno-unused-command-line-argument into CMAKE_CXX_FLAGS
GCC 8's header has a name collision with a symbol in the macOS 13 <sys/cdefs.h> header[6]. GCC upstream fixed this[7] and backported the change to GCC 10[8], but it appears GCC 8 was not fixed. Using the macOS 11 SDK by setting CMAKE_OSX_SYSROOT solves this issue.
Clang 8 needs a bunch of extra flags to use libstdc++. I feel there should be a way to point Clang at GCC's root to get it to use libstdc++, but I was unable to find the correct flag. As a result, I just manually pointed Clang at the directories that contained the libstdc++ headers/libraries. I also had to disable the -Wstdlibcxx-not-found warning to avoid spurious failures, and also had to specify CMAKE_OSX_SYSROOT to avoid what appears to be some other conflict with the macOS 13 SDK[9].

[0]: https://github.com/libcpr/cpr/issues/982

[1]:

#!/bin/sh

rm -rf build/
mkdir build/

mkdir build/llvm-8/
cd build/llvm-8 || exit
cmake \
    ../.. \
    -DCMAKE_CXX_COMPILER=/usr/local/opt/llvm@8/bin/clang++ \
    -DCMAKE_CXX_FLAGS="-L/usr/local/opt/llvm@8/lib -Wno-unused-command-line-argument" \
    -DCMAKE_OSX_DEPLOYMENT_TARGET="10.12" \
    || true
cd ../.. || exit

mkdir build/llvm-8-libstdcxx/
cd build/llvm-8-libstdcxx || exit
cmake \
    ../.. \
    -DCMAKE_CXX_COMPILER=/usr/local/opt/llvm@8/bin/clang++ \
    -DCMAKE_CXX_FLAGS="-stdlib=libstdc++ -isystem /usr/local/opt/gcc@8/include/c++/8.5.0 -isystem /usr/local/opt/gcc@8/include/c++/8.5.0/x86_64-apple-darwin21 -L/usr/local/opt/gcc@8/lib/gcc/8 -Wno-stdlibcxx-not-found -Wno-unused-command-line-argument" \
    -DCMAKE_OSX_DEPLOYMENT_TARGET="10.12" \
    -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX11.sdk \
    || true
cd ../.. || exit

mkdir build/gcc-8/
cd build/gcc-8 || exit
cmake \
    ../.. \
    -DCMAKE_CXX_COMPILER=/usr/local/opt/gcc@8/bin/g++-8 \
    -DCMAKE_OSX_DEPLOYMENT_TARGET="10.12" \
    -DCMAKE_OSX_SYSROOT=/Library/Developer/CommandLineTools/SDKs/MacOSX11.sdk \
    || true
cd ../.. || exit

mkdir build/llvm-17/
cd build/llvm-17 || exit
cmake \
    ../.. \
    -DCMAKE_CXX_COMPILER=/usr/local/opt/llvm/bin/clang++ \
    -DCMAKE_CXX_FLAGS="-L/usr/local/opt/llvm/lib -Wno-unused-command-line-argument" \
    || true
cd ../.. || exit

mkdir build/gcc-13/
cd build/gcc-13 || exit
cmake \
    ../.. \
    -DCMAKE_CXX_COMPILER=/usr/local/opt/gcc/bin/g++-13 \
    || true
cd ../.. || exit

[2]:

cmake % /usr/local/opt/llvm@8/bin/clang++ -std=c++17 -Wl,-L/usr/local/opt/llvm@8/lib std_fs_support_test.cpp
ld: warning: dylib (/usr/local/opt/llvm@8/lib/libc++.dylib) was built for newer macOS version (12.0) than being linked (10.17)
ld: warning: dylib (/usr/local/opt/llvm@8/lib/libunwind.dylib) was built for newer macOS version (12.0) than being linked (10.17)
ld: warning: dylib (/usr/local/opt/llvm@8/lib/libunwind.dylib) was built for newer macOS version (12.0) than being linked (10.17)
Undefined symbols for architecture x86_64:
  "std::__1::__fs::filesystem::__current_path(std::__1::error_code*)", referenced from:
      std::__1::__fs::filesystem::current_path() in std_fs_support_test-efcd38.o
ld: symbol(s) not found for architecture x86_64
clang-8: error: linker command failed with exit code 1 (use -v to see invocation)
// Note that specifying a search path for the linker is required
// here to prevent the linker from finding the system libc++, which
// is from a newer Clang that includes <filesystem> support

[3]:

cmake % /usr/local/opt/gcc@8/bin/g++-8 -std=c++17 std_fs_support_test.cpp
Undefined symbols for architecture x86_64:
  "std::filesystem::current_path[abi:cxx11]()", referenced from:
      _main in ccsCb1G1.o
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status

[4]:

CMake Error at /usr/local/Cellar/cmake/3.27.7/share/cmake/Modules/CMakeTestCXXCompiler.cmake:60 (message):
  The C++ compiler

    "/usr/local/opt/llvm@8/bin/clang++"

  is not able to compile a simple test program.

  It fails with the following output:

    Change Dir: '/Users/bg/code/cpr/build/llvm-8/CMakeFiles/CMakeScratch/TryCompile-LjarIm'

    Run Build Command(s): /usr/local/Cellar/cmake/3.27.7/bin/cmake -E env VERBOSE=1 /usr/bin/make -f Makefile cmTC_25eef/fast
    /Applications/Xcode.app/Contents/Developer/usr/bin/make  -f CMakeFiles/cmTC_25eef.dir/build.make CMakeFiles/cmTC_25eef.dir/build
    Building CXX object CMakeFiles/cmTC_25eef.dir/testCXXCompiler.cxx.o
    /usr/local/opt/llvm@8/bin/clang++   -Wl,-L/usr/local/opt/llvm@8/lib  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -mmacosx-version-min=12.6 -MD -MT CMakeFiles/cmTC_25eef.dir/testCXXCompiler.cxx.o -MF CMakeFiles/cmTC_25eef.dir/testCXXCompiler.cxx.o.d -o CMakeFiles/cmTC_25eef.dir/testCXXCompiler.cxx.o -c /Users/bg/code/cpr/build/llvm-8/CMakeFiles/CMakeScratch/TryCompile-LjarIm/testCXXCompiler.cxx
    clang-8: warning: -Wl,-L/usr/local/opt/llvm@8/lib: 'linker' input unused [-Wunused-command-line-argument]
    clang-8: error: invalid version number in '-mmacosx-version-min=12.6'
    make[1]: *** [CMakeFiles/cmTC_25eef.dir/testCXXCompiler.cxx.o] Error 1
    make: *** [cmTC_25eef/fast] Error 2

  CMake will not be able to correctly generate this project.
Call Stack (most recent call first):
  CMakeLists.txt:2 (project)

[5]:

CMake Error at /usr/local/Cellar/cmake/3.27.7/share/cmake/Modules/CMakeTestCXXCompiler.cmake:60 (message):
  The C++ compiler

    "/usr/local/opt/gcc@8/bin/g++-8"

  is not able to compile a simple test program.

  It fails with the following output:

    Change Dir: '/Users/bg/code/cpr/build/gcc-8/CMakeFiles/CMakeScratch/TryCompile-VGQ3Hn'

    Run Build Command(s): /usr/local/Cellar/cmake/3.27.7/bin/cmake -E env VERBOSE=1 /usr/bin/make -f Makefile cmTC_08403/fast
    /Applications/Xcode.app/Contents/Developer/usr/bin/make  -f CMakeFiles/cmTC_08403.dir/build.make CMakeFiles/cmTC_08403.dir/build
    Building CXX object CMakeFiles/cmTC_08403.dir/testCXXCompiler.cxx.o
    /usr/local/opt/gcc@8/bin/g++-8   -Wl,-L/usr/local/opt/gcc@8/lib  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -mmacosx-version-min=12.6 -o CMakeFiles/cmTC_08403.dir/testCXXCompiler.cxx.o -c /Users/bg/code/cpr/build/gcc-8/CMakeFiles/CMakeScratch/TryCompile-VGQ3Hn/testCXXCompiler.cxx
    g++-8: warning: '12.6' is not valid for 'mmacosx-version-min'

    Linking CXX executable cmTC_08403
    /usr/local/Cellar/cmake/3.27.7/bin/cmake -E cmake_link_script CMakeFiles/cmTC_08403.dir/link.txt --verbose=1
    /usr/local/opt/gcc@8/bin/g++-8 -Wl,-L/usr/local/opt/gcc@8/lib  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -mmacosx-version-min=12.6 -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/cmTC_08403.dir/testCXXCompiler.cxx.o -o cmTC_08403
    g++-8: warning: '12.6' is not valid for 'mmacosx-version-min'

    ld: library not found for -lgcc_s.10.4
    collect2: error: ld returned 1 exit status
    make[1]: *** [cmTC_08403] Error 1
    make: *** [cmTC_08403/fast] Error 2

[6]: https://github.com/Homebrew/homebrew-core/issues/114607

[7]: https://github.com/gcc-mirror/gcc/commit/d1201dbf55a11d391030914985ba6b443e59baa5

[8]: https://github.com/gcc-mirror/gcc/commit/7137ae4051ae71bb7fc7fd59d956952c53403239

[9]:

/usr/local/opt/llvm@8/bin/clang++   -stdlib=libstdc++ -isystem /usr/local/opt/gcc@8/include/c++/8.5.0 -isystem /usr/local/opt/gcc@8/include/c++/8.5.0/x86_64-apple-darwin21 -L/usr/local/opt/gcc@8/lib/gcc/8 -Wno-stdlibcxx-not-found -Wno-unused-command-line-argument -Wall -Wextra -Wpedantic -Werror -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-nonportable-system-include-path -Wno-exit-time-destructors -Wno-undef -Wno-global-constructors -Wno-switch-enum -Wno-old-style-cast -Wno-covered-switch-default -Wno-undefined-func-template  -std=c++17 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -mmacosx-version-min=10.12 -MD -MT CMakeFiles/cmTC_51f89.dir/std_fs_support_test.cpp.o -MF CMakeFiles/cmTC_51f89.dir/std_fs_support_test.cpp.o.d -o CMakeFiles/cmTC_51f89.dir/std_fs_support_test.cpp.o -c /Users/bg/code/cpr/cmake/std_fs_support_test.cpp
In file included from /Users/bg/code/cpr/cmake/std_fs_support_test.cpp:1:
In file included from /usr/local/opt/gcc@8/include/c++/8.5.0/filesystem:37:
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:122:7: error: unknown type name
      '_S_range_end'
      _S_range_end(_Source) { return {}; }
      ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:122:31: error: expected expression
      _S_range_end(_Source) { return {}; }
                              ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:124:31: error: expected member
      name or ';' after declaration specifiers
    template<typename _CharT, typename _Traits, typename _Alloc>
                              ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:195:28: error: no template named
      '__value_type_is_char'
             typename _Require2 = __value_type_is_char<_Source>>
                                  ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:203:28: error: no template named
      '__value_type_is_char'
             typename _Require2 = __value_type_is_char<_InputIterator>>
                                  ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:428:52: error: expected parameter
      declarator
    _S_convert(value_type* __src, __null_terminated)
                                                   ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:432:58: error: expected parameter
      declarator
    _S_convert(const value_type* __src, __null_terminated)
                                                         ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:446:57: error: expected parameter
      declarator
      _S_convert(_InputIterator __src, __null_terminated)
                                                        ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:469:61: error: expected parameter
      declarator
      _S_convert_loc(_InputIterator __src, __null_terminated,
                                                            ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:184:11: error: use of undeclared
      identifier '_S_range_end'
                               _S_range_end(__source)))
                               ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:1023:64: note: in instantiation of
      function template specialization
      'std::filesystem::__cxx11::path::path<std::__cxx11::basic_string<char,
      std::char_traits<char>, std::allocator<char> >, std::filesystem::__cxx11::path>'
      requested here
  path::compare(const string_type& __s) const { return compare(path(__s)); }
                                                               ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:184:11: error: use of undeclared
      identifier '_S_range_end'
                               _S_range_end(__source)))
                               ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:1026:63: note: in instantiation of
      function template specialization 'std::filesystem::__cxx11::path::path<const char
      *, std::filesystem::__cxx11::path>' requested here
  path::compare(const value_type* __s) const { return compare(path(__s)); }
                                                              ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:184:11: error: use of undeclared
      identifier '_S_range_end'
                               _S_range_end(__source)))
                               ^
/usr/local/opt/gcc@8/include/c++/8.5.0/bits/fs_path.h:1030:20: note: in instantiation of
      function template specialization
      'std::filesystem::__cxx11::path::path<std::basic_string_view<char,
      std::char_traits<char> >, std::filesystem::__cxx11::path>' requested here
  { return compare(path(__s)); }
                   ^
12 errors generated.
make[2]: *** [CMakeFiles/cmTC_51f89.dir/std_fs_support_test.cpp.o] Error 1
make[1]: *** [CMakeFiles/cmTC_51f89.dir/all] Error 2
make: *** [all] Error 2

COM8 commented 7 months ago

Everything except fedora is failing right now. Could you please take a look at it? Here's a ref for which runs should succeed: https://github.com/libcpr/cpr/actions/runs/6684425894/job/18197198197

ts826848 commented 7 months ago

Took a glance at the failing checks. Quick summary before I start diving in:

package-ubuntu-latest-amd64: Looks like might be unrelated to the PR changes? The build fails due to a missing file/directory when creating a tarball [0]
CI
- (ubuntu:18.04, ON, Debug), (ubuntu:18.04, OFF, Release), (ubuntu:18.04, OFF, Release): Looks like my CMake check isn't working the way I expected.
- windows-msvc-ssl Release: AsyncRequestsTests.AsyncGetMultipleReflectTest failed
- windows-msvc-openssl, macos-clang-openssl: Could not find <openssl/err.h> when building mongoose. These are also failing in the reference job.
- macos-clang-ssl Debug: UrlEncodedPostTests.UrlPostAsyncSingleTest failed
- macos-clang-darwinssl: Multiple failed tests:
- AsyncRequestsTests.AsyncGetMultipleReflectTest
- AsyncTests.AsyncGetMultipleReflectTest
- OptionsTests.AsyncSpecificUrlTest
- PatchTests.AsyncMultiplePatchUnallowedTest
- MultiperformPerformTests.MultiperformMixedMethodsPerformTest

clang-tidy: All the errors appear to be either missing headers or unnecessarily included headers flagged by `misc-include-cleaner`. Not sure if these should be fixed in this PR since I didn't touch any of the C++ files?

[0]:

cp: cannot create directory 'libcpr_987/merge': No such file or directory
tar: libcpr_987/merge: Cannot stat: No such file or directorytar (child): 
libcpr_987/merge.orig.tar.gz: Cannot open: No such file or directory
tar (child): Error is not recoverable: exiting now

ts826848 commented 7 months ago

So running the macos-clang-darwinssl configuration locally I get more failed tests than the CI:

cpr_post_tests
- UrlEncodedPostTests.UrlPostAsyncSingleTest
cpr_session_tests
- AsyncRequestsTests.AsyncGetMultipleTest
- AsyncRequestsTests.AsyncGetMultipleTemporarySessionTest
- AsyncRequestsTests.AsyncGetMultipleReflectTest
cpr_async_tests
- AsyncTests.AsyncGetMultipleReflectTes
cpr_head_tests
- HeadTests.BasicHeadAsyncTest
cpr_delete_tests
- DeleteTests.AsyncMultipleDeleteUnallowedTest
cpr_options_tests
- OptionsTests.AsyncSpecificUrlTest
cpr_patch_tests
- PatchTests.AsyncMultiplePatchTest
- PatchTests.AsyncMultiplePatchUnallowedTest

In addition, if I build the master branch I get a slightly different set of failed tests:

cpr_session_tests
- AsyncRequestsTests.AsyncGetMultipleTest
- AsyncRequestsTests.AsyncGetMultipleTemporarySessionTest
- AsyncRequestsTests.AsyncGetMultipleReflectTest
cpr_async_tests
- AsyncTests.AsyncGetMultipleReflectTest
cpr_head_tests
- HeadTests.BasicHeadAsyncTest
cpr_delete_tests
- DeleteTests.AsyncMultipleDeleteTest
- DeleteTests.AsyncMultipleDeleteUnallowedTest
cpr_put_tests
- PutTests.AsyncMultiplePutUnallowedTest
cpr_options_tests
- OptionsTests.AsyncBaseUrlTest
- OptionsTests.AsyncSpecificUrlTest
cpr_patch_tests
- PatchTests.AsyncMultiplePatchUnallowedTest

After running the master branch tests a few times it seems the set of failing tests can differ very slightly from run to run. The failed tests are almost always returning an empty string or 0 instead of a proper value as well, though there's one test where <06-00 00-00> is returned instead of <00-00 00-00> (UrlEncodedPostTests.UrlPostAsyncSingleTest, post_tests.cpp:607). This pattern also appears to apply to the failing windows-msvc-ssl and macos-clang-ssl tests.

I'm not entirely sure where to go from here. The changing set of failing tests, the failing tests on master, and the manner of failure make me suspicious that I don't have something set up right, and I probably need to fix that first.

I'll see if I can figure out the Ubuntu 18.04 issues first, then I'll come back to the tests.

ts826848 commented 7 months ago

I've pushed a change which should hopefully fix the Ubuntu 18.04 builds. I've also applied your suggestions.

COM8 commented 7 months ago

Thanks for taking a look at it. Yes, only focus on the ubuntu stuff. MacOS tests and packaging are known issues I'm working on.

libcpr / cpr