Modernize FindBotan.cmake: Use Imported Targets and Remove Global Variables - #13620
Modernize FindBotan.cmake: Use Imported Targets and Remove Global Variables#13620aollier wants to merge 1 commit into
Conversation
|
On Ubuntu, I have: $ pkg-config --libs --cflags botan-3
-I/usr/include/botan-3 -lbotan-3 -fstack-protector -m64 -pthreadMy change will add the |
- Replace raw variables (BOTAN_LIBRARY, BOTAN_INCLUDE_DIRS) with an imported
target Botan::botan that carries include dirs, compile options, and link flags
- Remove global include_directories() in favor of explicit target_link_libraries()
- Rename internal variables from BOTAN_ to Botan_ for consistency
- Update the rest of the project to depend on Botan::botan instead of ${BOTAN_LIBRARIES}
Tested on Ubuntu; relying on GitHub CI to validate Windows and macOS builds.
|
Did you add compile flags? |
|
No I didn't add any compile flags. These flags are provided by the libbotan-3-dev package: $ dpkg -L libbotan-3-dev | grep \.pc
/usr/lib/x86_64-linux-gnu/pkgconfig/botan-3.pc
$ cat $(!!)
cat $(dpkg -L libbotan-3-dev | grep \.pc)
prefix=/usr/
exec_prefix=${prefix}
libdir=/usr/lib/x86_64-linux-gnu
includedir=${prefix}/include/botan-3
Name: Botan
Description: Crypto and TLS for Modern C++
Version: 3.10.0
Libs: -L${libdir} -lbotan-3 -fstack-protector -m64 -pthread
Libs.private: -lbz2 -llzma -lrt -lsqlite3 -ltspi -lz
Cflags: -I${includedir}This means these flags are provided for Ubuntu. They may be different on Windows and MacOS. |
|
The flags are fetched from pkg-config and propagated to the imported target with this line: On Ubuntu, this results to the |
|
If your question was "Does your change add compile flags to the targets that link against |
|
I dont think that is appropriate at all. We do not want to inherit compile flags that are potentially different across distributions. The executable compile flags absolutely do not need to be the same as the libraries it loads. We already have a lot of security related flags set especially around memory layout and overwrite protection. |
|
The flags that pkg-config provides are not the ones used to compile the library (libbotan here). They are intended to be used by the target that links against. Check the documentation of pkg-config: In particular:
Theses flags are required to link against the libbotan, no matter which flags are used on the other hand by our executables. This information is provided by the maintainer of the application. It is absolutely not related with memory layout or security considerations and does not conflict with. Providing compilation information (include directories with the -I flag, library paths with the -L flag, and library names with the -l flag and other link options) is the very reason pkg-config exists. |
|
|
I am wondering about this line: keepassxc/cmake/FindBotan.cmake Line 39 in 79c3c37 Isn't meant to focus on MSVC? In this case, we can change it to: if(MSVC)which is more appropriate. |
|
No that was for windows builds using vcpkg. I disagree completely with the compiler flags. I think you misread the pkg_conf docs. It is saying those are the flags it used to build the library. Not what flags you need to use the library. Either way, we already set stack protection and the others. I do not want a vector where "random" compile flags are added to our builds. |
|
No I am absolutely sure about what I am saying. Here are some extracts of the documentation (https://people.freedesktop.org/~dbn/pkg-config-guide.html): Importants words are bolded.
The most important sentence is: They will define the metadata used by external projects to compile and link with the library. This is clear and non ambiguous. As an exemple, with botan-3: $ pkg-config --libs --cflags botan-3
-I/usr/include/botan-3 -lbotan-3 -fstack-protector -m64 -pthreadlibbotan-3.so can't have been built with the -lbotan-3 flag, since it instructs to link with the libbotan-3. Here we can clearly see that it's a vicious cycle. libbotan-3.so have been compiled with much more compiler flags. The other flags provided by pkg-config (-fstack-protector -m64 -pthread) are a subset of the ones used to compile this library. These are the ones needed for link editing that guarantees the compatibility and stability of the executable being linked to. If pkg-config provided all the flags used for compilation, there would be many more, and more importantly, there would be no point in exposing them. What would be the benefit? I hope I've convinced you this time, otherwise what can we do to determine who is right? |
|
Hmm fair enough, I was wrong. I still do not agree with auto adding compiler flags from an external source. |
|
I understand the desire to keep total control over compiler flags. However, discarding the Here is why consuming the complete
These flags affect only the translation units that include this third-party dependency. They cannot bleed into the rest of KeePassXC's codebase or alter global optimization flags. As a regular KeePassXC user myself, my goal is certainly not to break the application, but on the contrary to make its build system more reliable and future-proof. If you are concerned about rogue flags in edge cases, we can explicitly print the imported flags during the CMake configuration step ( Would you be open to reviewing the PR with target-scoped flags enabled and CMake status visibility? |
|
To move forward pragmatically, since the CI pipeline hasn't run yet, would you be open to letting the CI build and test this PR across all supported platforms (Linux, macOS, Windows)? If you have a moment to build and run this branch locally, you can also verify firsthand that it causes no regressions or runtime instability. Testing it in practice rather than in theory seems like the safest way to ensure that scoped |
|
Hello @droidmonkey, I marked this PR as draft since a build failed for an unknown and obscure reason and should be restarted. Since I don't have Windows or MacOS at home, I'm not sure it will compile successfully on those systems. Hence separate commits, the first serving to verify that the compilation succeeds and the tests pass in the current configuration, the second serving to limit the scope of the imported library to the strict minimum necessary. |
|
In all technicality you dont need to include Botan library everywhere. Every built executable includes keepassxc core which includes botan. |
|
The botan library is required to compile not only the final executables, but also the intermediate libraries themselves. For exemple, if I remove the Botan::botan target from because the compiler does not find botan/mem_ops.h. It was also necessary to add it for
As we can see, src/keys/PasswordKey.h is included indirectly from several libraries, but src/keys does not provide any target. For the others libraries, botan is a direct dependency: $ git grep -l 'botan/' -- '*.h' '*.cpp'
src/browser/BrowserMessageBuilder.cpp
src/browser/BrowserPasskeys.cpp
src/browser/BrowserPasskeys.h
src/core/Alloc.cpp
src/crypto/Crypto.cpp
src/crypto/CryptoHash.cpp
src/crypto/Random.cpp
src/crypto/Random.h
src/crypto/SymmetricCipher.cpp
src/crypto/kdf/Argon2Kdf.cpp
src/fdosecrets/objects/SessionCipher.cpp
src/format/BitwardenReader.cpp
src/format/OpVaultReader.cpp
src/keeshare/KeeShareSettings.cpp
src/keeshare/ShareExport.cpp
src/keys/FileKey.h
src/keys/PasswordKey.h
src/keys/drivers/YubiKey.h
src/quickunlock/Polkit.cpp
src/sshagent/OpenSSHKey.cpp
src/sshagent/OpenSSHKeyGen.cpp
tests/TestBrowser.cpp
tests/TestPasskeys.cpp
tests/TestPasskeys.h
tests/TestSharing.cpp |
|
That is because you removed the global include directory on line 485 in the main CMakeLists.txt |
|
Yes, I know. The Botan include directory was always included, even when not needed, thereby polluting the global namespace. |
|
The current situation is completely contradictory: you tell me there's no need to include the library everywhere, yet on the other hand, the Botan include directory is present everywhere due to the global inclusion. |
|
Having a search path set is very different from including a library in a build. Including excessive libraries tends to increase linker time because it has to deconflict symbols over and over again. |
|
Since we are building a static library target ( The binary library linking ( Therefore, using |
|
I think there was a misunderstanding: I wasn't talking about adding new links to the Botan library throughout the project, but rather replacing the existing PUBLIC links with PRIVATE ones—except for keepassxc_core. |
|
Gotcha OK |
|
Now that we finally understand each other, could you please rerun the failed build to ensure that all tests are passing at this stage? Once that's done, I'll push the second commit to make the links PUBLIC to PRIVATE. Thank you very much. |
|
@droidmonkey Since I am modifying the cmake/FindBotan.cmake file, does that affect its license? Here is a sample about the COPYING file:
|
Modernize FindBotan.cmake: Use Imported Targets and Improve Portability
This Pull Request refactors the
FindBotan.cmakemodule to align with modern CMake best practices, as outlined in the official CMake documentation. The changes improve maintainability, portability, and cleanliness of the build system while ensuring backward compatibility with the rest of the project.Key Improvements
Leverages
PkgConfigfor hints:Uses
pkg_search_moduleto automatically detect Botan's include and library paths viaPkgConfig. These paths are then passed asHINTStofind_pathandfind_library, ensuring robust detection without relying solely onPkgConfig.→ Combines the efficiency of
PkgConfigwith the reliability of CMake's built-in commands.Creates a modern imported target (
Botan::botan):Replaces global variables (
BOTAN_LIBRARY,BOTAN_INCLUDE_DIRS) with a modern imported target, following CMake’s recommended practices.→ Enables clean integration with
target_link_libraries()and prevents dependency leaks.Manages Debug and Release configurations:
Uses
SelectLibraryConfigurationsto handle separate Debug and Release builds of Botan (e.g.,botan-3vs.botand-3on Windows).→ Ensures correct linking for multi-configuration generators like MSVC.
Automatic version extraction:
Extracts the Botan version from
botan/build.hand validates it usingfind_package_handle_standard_args.→ Ensures compatibility with the required Botan version.
Propagates platform-specific flags:
Uses
INTERFACE_LINK_OPTIONSto propagate platform-specific flags (e.g.,-pthread,-fstack-protector) fromPkgConfigto the imported target.→ Ensures correct linking behavior without manual flag duplication.
Removes global
include_directories:Replaces
include_directories(SYSTEM ${BOTAN_INCLUDE_DIR})with explicit dependencies viatarget_link_libraries(Botan::botan).→ Prevents unnecessary inclusion of Botan headers and reduces global namespace pollution.
Replaces
${BOTAN_LIBRARIES}withBotan::botan:All occurrences of
${BOTAN_LIBRARIES}have been replaced with the imported target, in line with CMake’s dependency management recommendations.Falls back gracefully:
If
PkgConfigis unavailable,find_pathandfind_librarywill still search standard system paths (e.g.,/usr/include,/usr/lib).→ No explicit fallback logic is needed, as the module already handles this case.
Impact on the Project
${BOTAN_LIBRARIES}withBotan::botan(already done).Points to Verify or Improve for the Maintainer
Testing on Windows and macOS:
The module has been tested on Ubuntu, but it is recommended to verify its behavior on Windows (via vcpkg or MSYS2) and macOS (via Homebrew).
→ If adjustments are needed for these platforms, I’m ready to implement them.
Version consistency check:
If
PkgConfigdetects a version of Botan (e.g.,2.19.1), butfind_pathlocates abuild.hwith a different version, consider adding a warning:→ Helps avoid inconsistencies between detected and actual versions.
Debugging messages:
Adding a status message to indicate whether
PkgConfigwas used could help with debugging:→ Useful for troubleshooting but not mandatory.
Type of change