Skip to content
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

Add example source code for client-side detection of CiftiLib #15

Open
ghisvail opened this issue Nov 8, 2016 · 11 comments
Open

Add example source code for client-side detection of CiftiLib #15

ghisvail opened this issue Nov 8, 2016 · 11 comments

Comments

@ghisvail
Copy link
Contributor

ghisvail commented Nov 8, 2016

Perhaps something that the description in README or USAGE is currently lacking is a code example for discovering and using the library from a client project.

I have used the following snippet in my integration tests:

find_package(PkgConfig)
pkg_check_modules(PC_CIFTI REQUIRED CiftiLib)
add_library(PkgConfig::Cifti INTERFACE IMPORTED)
set_target_properties(PkgConfig::Cifti PROPERTIES
	INTERFACE_INCLUDE_DIRECTORIES "${PC_CIFTI_INCLUDE_DIRS}"
	INTERFACE_LINK_LIBRARIES "${PC_CIFTI_LIBRARIES}"
	INTERFACE_COMPILE_OPTIONS "${PC_CIFTI_CFLAGS_OTHER}")
[...]
add_executable(myexecutable myexecutable.cxx)
target_link_libraries(myexecutable PkgConfig::Cifti)

Feel free to copy or adapt this code if you wish.

@coalsont
Copy link
Member

coalsont commented Nov 8, 2016

I added the first two lines of that. I'm not that familiar with the imported target syntax of cmake.

@ghisvail
Copy link
Contributor Author

ghisvail commented Nov 9, 2016

I added the first two lines of that.

👍

I'm not that familiar with the imported target syntax of cmake.

It has got a lot of benefits and is worth investing some time on. The ideal usage from a client standpoint would be something like:

find_package(CiftiLib REQUIRED)
add_executable(myexecutable myexecutable.cxx)
target_link_libraries(myexecutable CiftiLib::Cifti)

whereby CiftiLib::Cifti would be imported from a CiftiLibConfig.cmake and would automatically carry the boost, zlib and libxml++ or qt4 or qt5 dependencies.

FYI, I should be submitting ciftilib v1.5.1 to Debian very soon.

@coalsont
Copy link
Member

coalsont commented Nov 9, 2016

whereby CiftiLib::Cifti would be imported from a CiftiLibConfig.cmake

I looked for how QT5 delivers its cmake module, and it has this unnerving quote:

The easiest way to use CMake is to set the CMAKE_PREFIX_PATH environment variable to the install prefix of Qt 5.

http://doc.qt.io/qt-5/cmake-manual.html

Are they doing it wrong, or is that aimed at custom installs from source? Where do other libraries deliver their cmake modules when installed via debian?

FYI, I should be submitting ciftilib v1.5.1 to Debian very soon.

Cool!

@ghisvail
Copy link
Contributor Author

http://doc.qt.io/qt-5/cmake-manual.html

Quoting the first snippet of your link:

# Find the QtWidgets library
find_package(Qt5Widgets)
# Tell CMake to create the helloworld executable
add_executable(helloworld WIN32 main.cpp)
# Use the Widgets module from Qt 5.
target_link_libraries(helloworld Qt5::Widgets)

This is the syntax for using imported target. Qt5 is the namespace, Widgets is one of the many targets Qt5 defines.

Where do other libraries deliver their cmake modules when installed via debian?

If you look at the file listing of the qtbase5-dev package, you'll find a list of *Config*.cmake files which are the one find_package looks for.

So in your case, you could generate a CiftiLibConfig.cmake, install it under ${CMAKE_INSTALL_LIBDIR}/cmake/CiftiLib and let client code use find_package(CiftiLib) instead of pkg-config.

@coalsont
Copy link
Member

# Find the QtWidgets library
find_package(Qt5Widgets)

I read that part, what I was worried about is that the later part sounded like cmake may not search other directories for such config files, and you would have to run cmake like this to get it to work that way:

cmake <sourcedir> -D CMAKE_PREFIX_PATH="/usr/lib/x86_64-linux-gnu/cmake/Qt5"

However, I tested this on an ubuntu 14.04 VM, and it seems as though qt5 is found without setting CMAKE_PREFIX_PATH (there is no qt5 cmake file in /usr/share/cmake-2.8/Modules). After some searching in the cmake man page, it appears to look in "/usr/lib/x86_64-linux-gnu/cmake/<name>*", among other locations, when doing a find_package, so that should work.

Note, however, that I wrote my own Findglib.cmake and similar for when it is compiled without using QT. Those packages deliver pkg-config files, but not cmake config files. It could get messy to handle this with a single cmake config file, perhaps it simply shouldn't install a cmake config file when it is not compiled against QT? I suppose I could have it add <prefix>/cmake/CiftiLib/cmake/Modules to the module path and install those extra modules when not compiled against QT...

@ghisvail
Copy link
Contributor Author

it appears to look in "/usr/lib/x86_64-linux-gnu/cmake/*", among other locations, when doing a find_package, so that should work.

As stated in the find_package documentation

It could get messy to handle this with a single cmake config file

Not if you use imported targets.

I suppose I could have it add <prefix>/cmake/CiftiLib/cmake/Modules

Don't.

The solution here is for both the Qt and LibXml++ paths to define an imported interface library (see code snippet in the opening post) with whatever detection mechanism you have in place (find modules, pkg-config or config package) and link it with your CiftiLib target.

The exported CiftiLib target and corresponding CMake config file will handle all the dependencies (include paths, library paths, libraries and compile definitions) for you. The resulting CiftiLibConfig.cmake should be usable standalone.

@coalsont
Copy link
Member

Looking at https://cmake.org/Wiki/CMake/Tutorials/Exporting_and_Importing_Targets , it has this syntax:

install(TARGETS generator DESTINATION lib/myproj/generators EXPORT myproj-targets)
install(EXPORT myproj-targets DESTINATION lib/myproj)

How magical is this syntax, will it actually take care of turning custom FindBlah.cmake scripts into statements inside that single exports file? If the resulting export will contain a find_package(libxml++) statement, same as the current main CMakeLists.txt file, it should fail for client projects unless <prefix>/cmake/libxml++*/ contains a relevant file (and it is obviously wrong for CiftiLib to install anything to such a directory), or I add an explicit modules path to an install location, and install such a Find file there (which you objected to).

Shorter question: do you have an example of how to deal with a library that doesn't provide a cmake file in this way? If not, I'd currently prefer not installing a cmake file when built against libxml++, because I currently don't see how to do it cleanly (when built against QT, every dependency already provides its own cmake find_package files, so it just works).

@coalsont
Copy link
Member

I did a little more investigation, and started trying to make an imported target for libxml++, and then found that cmake 2.8.7 doesn't understand "INTERFACE" in add_library(). It also doesn't seem to understand the INTERFACE_LINK_LIBRARIES property when using "IMPORTED" in ADD_LIBRARY, and instead insists on using IMPORTED_LOCATION (and writes a "not found" into a makefile without giving a configuration error when it isn't set). Finally, the INSTALL(TARGET ... EXPORT ...) syntax doesn't appear to accept imported targets, and I am under the impression that that is required in order for the INSTALL(EXPORT ...) command to know where to find the dependencies.

So, this isn't going to work nicely on cmake 2.8.7 (boost requires either 2 or 0 libraries, depending on whether QT is in use, both of which are problematic to a single imported target). I'm not sure about requiring a newer cmake (for instance, neurodebian builds connectome-workbench on ubuntu 12.04 and debian wheezy, which package cmake 2.8.7 and 2.8.9). I made a branch, export-experiment, that has my initial attempts in it, for future reference, or if you know some workarounds, or look for places where I went off the rails.

@ghisvail
Copy link
Contributor Author

How magical is this syntax, will it actually take care of turning custom FindBlah.cmake scripts into statements inside that single exports file? If the resulting export will contain a find_package(libxml++) statement, same as the current main CMakeLists.txt file, it should fail for client projects unless /cmake/libxml++*/

Say you have the CiftiLib target linked with a LibXml++ target that you imported with the INTERFACE, then the CiftiLib export will carry the relevant include directories (-I flags), library directories (-L) and compile options (-D and misc.).

Your export should only contain an import statement on the target file, period. On the client-side, there will be no more detection of LibXml++ involved when calling find_package(CiftiLib) because the dependency on LibXml++ (or Qt5) is automatically carried by the target.

this isn't going to work nicely on cmake 2.8.7

I believe the benefits brought by imported targets from a client-side perspective are worth bumping the requirements on the minimum CMake version a little bit.

You mentioned Ubuntu 12.04 and Debian Wheezy as concerns, but both will see their support window end soon. There is also very little chance that someone would deploy any of these 2 today, considering both Ubuntu 14.04 and 16.04 are available, and Debian Jessie has been around for almost 2 years. Finally, both have backports of cmake for at least version 2.8.11 so the NeuroDebian folks should be fine.

boost requires either 2 or 0 libraries, depending on whether QT is in use, both of which are problematic to a single imported target

That's actually where the flexibility of CMake targets shines. You can have multiple dependency paths (like for xml support and boost in your case) and have them handled automatically within the definition of the target without having to tweak anything. You just link the CiftiLib target with whatever other (optional or mandatory) target you defined earlier and don't bother with tweaking the detection mechanism for the client code.

@coalsont
Copy link
Member

coalsont commented Mar 17, 2020

So I came back to this, basically started over from master (the export-experiment2 branch), leaving the boost, etc code mostly as-is, but changing things to target properties of Cifti, and got down to this for what should be needed in the ...Config.cmake file when using Qt5:

include(/usr/local/lib/cmake/CiftiLib/CiftiLib.cmake)
add_definitions("-fPIC")

But, besides the fact that it shouldn't require manually setting -fPIC to compile (adding Qt5::Core to the libs is supposed to magically cause that somehow), it doesn't work at linking:

/usr/bin/ld: cannot find -lQt5::Core

Adding Qt5::Core to the target's libs is supposed to handle the PIC setting (and it works when building CiftiLib itself), but apparently an exported target chokes on it as even being a library? Does my ...Config.cmake file need to find QT again (in a conditional because of supporting libxml++ as a replacement for QT)?

Strangely, doing find_package(Qt5 COMPONENTS Core) instead of find_package(Qt5Core) is also making the imported target fail to find the include dir for QString...

@coalsont
Copy link
Member

coalsont commented Mar 17, 2020

So, something I found online indicated that when you have dependencies that are imported targets, yes, your ...Config.cmake needs to find_package again (apparently newer cmake has find_dependency that handles required/quiet automatically). I think I have nailed down the target-ness of the Cifti library itself, though currently it stuffs the entirety of libxml++ and dependencies straight into the includes/libraries of the Cifti target (because I'm lazy).

I'm not currently using a namespace on the imported target, though. There's really only one library this project provides, so I'm not sure what the best strategy here is.

Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants