-
Notifications
You must be signed in to change notification settings - Fork 949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid linking std <filesystem> library if not needed #987
Conversation
Clang and GCC both originally required users to link an additional library to be able to use most (all?) of the <filesystem> header. This support was later folded into the main stdlib libraries when the respective <filesystem> implementations stabilized. cpr takes this into account, but it currently only supports linking in libstdc++fs, which is the <filesystem> 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 <filesystem>), which can result in compilation errors due to being unable to find the support library[^0]. This commit changes <filesystem> 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 <filesystem> 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 <filesystem> 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 <filesystem> 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 <filesystem> 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=<version> (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 <fileystem> support. This can be fixed by passing -DCMAKE_CXX_FLAGS="-L<version-specific-lib-dir>", 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 <filesystem> 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]: libcpr#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]: Homebrew/homebrew-core#114607 [^7]: gcc-mirror/gcc@d1201db [^8]: gcc-mirror/gcc@7137ae4 [^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
c9c48db
to
9795673
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on it and investing the time!
I left some comments but in general I'm happy with your stuff.
Everything except fedora is failing right now. Could you please take a look at it? |
Took a glance at the failing checks. Quick summary before I start diving in:
[0]:
|
So running the macos-clang-darwinssl configuration locally I get more failed tests than the CI:
In addition, if I build the master branch I get a slightly different set of failed tests:
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 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. |
I've pushed a change which should hopefully fix the Ubuntu 18.04 builds. I've also applied your suggestions. |
The CI builds using Ubuntu 18.04 were failing to make it past the CMake configuration stage due to being unable to find <filesystem> support. This is because Ubuntu 18.04 ships GCC 7, for which <filesystem> support was still experimental. As a result, <experimental/filesystem> was present, not <filesystem>, which causes the CMake check to error out as the test program #includes <filesystem>. The fix is simple - use __has_include to include the appropriate header and add a namespace alias to account for std::experimental. This is sufficient to get the CMake configuration to succeed using GCC 7 on macOS, but configuration still fails on Ubuntu 18.04 due to the linker being unable to find the definition for current_path() [0]. This error seems a bit weird since the linker command line clearly includes -lstdc++fs, and that library definitely defines current_path(). It turns out the Ubuntu 18.04 linker is order-sensitive, and -lstdc++fs was present on the command line before the object file for the test program, so the linker doesn't pull in the current_path() definition. ld64 on macOS, on the other hand, doesn't seem to be sensitive to library order [1]. Fixing this is fairly simple - use the LINK_LIBRARIES try_compile option instead of LINK_OPTIONS, so CMake puts the flag in the right place. I should have done that in the first place, but I somehow glossed over that option when first writing the new handling. [0]: [ 50%] Linking CXX executable cmTC_6f62e /root/cmake-3.27.8-linux-x86_64/bin/cmake -E cmake_link_script CMakeFiles/cmTC_6f62e.dir/link.txt --verbose=1 /usr/bin/c++ -Wall -Wextra -Wpedantic -Werror -lstdc++fs CMakeFiles/cmTC_6f62e.dir/std_fs_support_test.cpp.o -o cmTC_6f62e CMakeFiles/cmTC_6f62e.dir/std_fs_support_test.cpp.o: In function `main': std_fs_support_test.cpp:(.text+0x1f): undefined reference to `std::experimental::filesystem::v1::current_path[abi:cxx11]()' collect2: error: ld returned 1 exit status [1]: [100%] Linking CXX executable cmTC_fb65e /usr/local/Cellar/cmake/3.27.7/bin/cmake -E cmake_link_script CMakeFiles/cmTC_fb65e.dir/link.txt --verbose=1 /usr/local/opt/gcc@7/bin/g++-7 -Wall -Wextra -Wpedantic -Werror -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX11.sdk -mmacosx-version-min=10.12 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -lstdc++fs CMakeFiles/cmTC_fb65e.dir/std_fs_support_test.cpp.o -o cmTC_fb65e ld: warning: dylib (/usr/local/Cellar/gcc@7/7.5.0_4/lib/gcc/7/libstdc++.dylib) was built for newer macOS version (10.17.5) than being linked (10.12) ld: warning: object file (/usr/local/Cellar/gcc@7/7.5.0_4/lib/gcc/7/libstdc++fs.a(dir.o)) was built for newer macOS version (10.17) than being linked (10.12) ld: warning: object file (/usr/local/Cellar/gcc@7/7.5.0_4/lib/gcc/7/libstdc++fs.a(ops.o)) was built for newer macOS version (10.17) than being linked (10.12) ld: warning: object file (/usr/local/Cellar/gcc@7/7.5.0_4/lib/gcc/7/libstdc++fs.a(path.o)) was built for newer macOS version (10.17) than being linked (10.12) ld: warning: object file (/usr/local/Cellar/gcc@7/7.5.0_4/lib/gcc/7/libstdc++fs.a(cow-dir.o)) was built for newer macOS version (10.17) than being linked (10.12) ld: warning: object file (/usr/local/Cellar/gcc@7/7.5.0_4/lib/gcc/7/libstdc++fs.a(cow-ops.o)) was built for newer macOS version (10.17) than being linked (10.12) ld: warning: object file (/usr/local/Cellar/gcc@7/7.5.0_4/lib/gcc/7/libstdc++fs.a(cow-path.o)) was built for newer macOS version (10.17) than being linked (10.12) [100%] Built target cmTC_fb65e
Suggested by @COM8 Co-authored-by: Fabian Sauter <[email protected]>
Suggested by @COM8 Co-authored-by: Fabian Sauter <[email protected]>
688e76d
to
d72ebf1
Compare
Thanks for taking a look at it. |
(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():
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:
[0]: #982
[1]:
[2]:
[3]:
[4]:
[5]:
[6]: Homebrew/homebrew-core#114607
[7]: gcc-mirror/gcc@d1201db
[8]: gcc-mirror/gcc@7137ae4
[9]: