Skip to content

Commit

Permalink
Add conventional name for exported target
Browse files Browse the repository at this point in the history 
QtKeychain defines an exported CMake target (qt5keychain), but the name
does not follow the CMake convention (Foo::Bar). This is confusing at
best, especially given the name is the same as the plain library.

It is not clear to the reader whether `target_link_libraries(foo PRIVATE
qt5keychain)` links to an imported target or a plain library.

Add an alias Qt5KeyChain::Qt5KeyChain with a more conventional name.
This preserves compatibility for users that rely on the qt5keychain
target.
  • Loading branch information
@nicolasfella @frankosterfeld
nicolasfella authored and frankosterfeld committed Apr 16, 2021
1 parent b3a2ec7 commit 70a567a
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions QtKeychainConfig.cmake.in
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
# It defines the following variables
# QTKEYCHAIN_INCLUDE_DIRS - include directories for QtKeychain
# QTKEYCHAIN_LIBRARIES - libraries to link against
# as well as the following imported targets
# qt5keychain / qt6keychain
# Qt5Keychain::Qt5Keychain / Qt6Keychain::Qt6Keychain

@PACKAGE_INIT@

Expand All @@ -17,3 +20,5 @@ endif()

set(QTKEYCHAIN_LIBRARIES "@QTKEYCHAIN_TARGET_NAME@")
get_target_property(QTKEYCHAIN_INCLUDE_DIRS "@QTKEYCHAIN_TARGET_NAME@" INTERFACE_INCLUDE_DIRECTORIES)

add_library(Qt@QTKEYCHAIN_VERSION_INFIX@Keychain::Qt@QTKEYCHAIN_VERSION_INFIX@Keychain ALIAS qt@QTKEYCHAIN_VERSION_INFIX@keychain)

4 comments on commit 70a567a

@krop
Copy link
Contributor

@krop krop commented on 70a567a Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks the build with CMake 3.17:

[  162s] CMake Error at /usr/lib64/cmake/Qt5Keychain/Qt5KeychainConfig.cmake:48 (add_library):
[  162s]   add_library cannot create ALIAS target "Qt5Keychain::Qt5Keychain" because
[  162s]   target "qt5keychain" is imported but not globally visible.

Ideally, qtkeychain should use proper export rather than hacks to inject aliases.

@daschuer
Copy link
Contributor

@daschuer daschuer commented on 70a567a Jan 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires CMake >= 18.0 with:
Kitware/CMake@056489d#diff-0e606b6054240c4fb56973e78d732b70c81ee16c00615af3ab08523313d78ac0
https://gitlab.kitware.com/cmake/cmake/-/issues/20641

Would be a version guard acceptable?

@krop
Copy link
Contributor

@krop krop commented on 70a567a Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicolasfella Thought on

diff --git a/QtKeychainConfig.cmake.in b/QtKeychainConfig.cmake.in
index d849ded..41abb0e 100644
--- a/QtKeychainConfig.cmake.in
+++ b/QtKeychainConfig.cmake.in
@@ -18,7 +18,13 @@ if(UNIX AND NOT APPLE AND NOT ANDROID)
     find_dependency(Qt@QTKEYCHAIN_VERSION_INFIX@DBus)
 endif()
 
-set(QTKEYCHAIN_LIBRARIES "@QTKEYCHAIN_TARGET_NAME@")
+get_target_property(QTKEYCHAIN_LIBRARIES "@QTKEYCHAIN_TARGET_NAME@" LOCATION)
 get_target_property(QTKEYCHAIN_INCLUDE_DIRS "@QTKEYCHAIN_TARGET_NAME@" INTERFACE_INCLUDE_DIRECTORIES)
 
-add_library(Qt@QTKEYCHAIN_VERSION_INFIX@Keychain::Qt@QTKEYCHAIN_VERSION_INFIX@Keychain ALIAS qt@QTKEYCHAIN_VERSION_INFIX@keychain)
+if(NOT TARGET Qt@QTKEYCHAIN_VERSION_INFIX@Keychain::Qt@QTKEYCHAIN_VERSION_INFIX@Keychain)
+    add_library(Qt@QTKEYCHAIN_VERSION_INFIX@Keychain::Qt@QTKEYCHAIN_VERSION_INFIX@Keychain UNKNOWN IMPORTED)
+    set_target_properties(Qt@QTKEYCHAIN_VERSION_INFIX@Keychain::Qt@QTKEYCHAIN_VERSION_INFIX@Keychain PROPERTIES
+        IMPORTED_LOCATION "${QTKEYCHAIN_LIBRARIES}"
+        INTERFACE_INCLUDE_DIRECTORIES "${QTKEYCHAIN_INCLUDE_DIRS}"
+    )
+endif()

@daschuer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have filed an issue here:
#209

Will you do a pull request with the suggested change?

Please sign in to comment.