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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial version of coz-profileConfig.cmake #129

Merged
merged 7 commits into from Oct 20, 2019

Conversation

@xd009642
Copy link
Contributor

xd009642 commented Oct 7, 2019

Still figuring out what's idiomatic/best-practice for this. And not fully sure on the installation directory... Any feedback from anyone would be appreciated otherwise I'll update when I'm confident I have something good 馃槃

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 7, 2019

Okay on a test project it's finding the include directories fine just needs a bit more tweaking to find the library!

@Corristo

This comment has been minimized.

Copy link

Corristo commented Oct 7, 2019

You seem to be mixing find modules and config files.

What you provide is a find module (it searches for the headers and the libraries). These are intended for retrofitting find_package support to a library that does not itself provide support for it (e.g. FFMPEG, old versions of boost, etc) and those are NOT provided by the library, but either ship with CMake itself or are part of a project consuming such a library.

Libraries that do want to support inclusion into other projects via find_package are supposed to provide a config file (like you named yours). The difference is that a config file does not need to use find_library and find_path, as it is generated by the library being installed, thus knowing the relative directory structure. It can just create the imported target and set the include directories and imported library based on the relative path to the config file itself. If you need inspiration you can have a look at the config files provided by e.g. fmt after installation.

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 7, 2019

@Corristo it's already been renamed, I'd not committed that change when I pushed. I'll move to setting the paths explicitly though

@Corristo

This comment has been minimized.

Copy link

Corristo commented Oct 7, 2019

@xd009642 My comment wasn't really about the name. Like I said, you seem to be mixing the two concepts. You shouldn't be calling find_path and find_file/find_library in a config file - you already know where everything is relative the the config file itself.

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 7, 2019

Ah okay, I've made the changes now. I use ${CMAKE_CURRENT_LIST_FILE} to get the library path as I'm not sure on if there's a guarantee it will always be lib instead of lib64 and don't want to rely on that and have it break on other machines/in future

@xd009642 xd009642 mentioned this pull request Oct 7, 2019
@xd009642 xd009642 changed the title Initial version of CozConfig.cmake (WIP) Initial version of CozConfig.cmake Oct 8, 2019
@xd009642 xd009642 changed the title Initial version of CozConfig.cmake Initial version of coz-profileConfig.cmake Oct 8, 2019
@ccurtsinger

This comment has been minimized.

Copy link
Member

ccurtsinger commented Oct 8, 2019

Could you provide a simple test case that builds a basic program with coz support using cmake?

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 8, 2019

Will do once I'm home from work

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 8, 2019

Here's the pca benchmark done with cmake, also if you test this on your system I'm using fedora and have some problems with libelfin and dwarf extensions in my gcc version (it links and compiles fine though):

cmake_minimum_required(VERSION 3.14)
project(pca_benchmark)

find_package(coz-profiler REQUIRED)
find_package(Threads)

include_directories(${COZ_INCLUDE_DIR})

add_executable(pca pca-pthread.c)
target_link_libraries(pca ${COZ_LIBRARY} ${CMAKE_THREAD_LIBS_INIT})
xd009642 added 3 commits Oct 7, 2019
Library path still partially worked out as it could be lib or lib64
@xd009642 xd009642 force-pushed the xd009642:cmake_config branch from f4da040 to 8229ecf Oct 8, 2019
@Corristo

This comment has been minimized.

Copy link

Corristo commented Oct 8, 2019

Ah okay, I've made the changes now. I use ${CMAKE_CURRENT_LIST_FILE} to get the library path as I'm not sure on if there's a guarantee it will always be lib instead of lib64 and don't want to rely on that and have it break on other machines/in future

That is exactly why config files are superior to find modules - the build system (in this case the Makefile) already knows where everything goes, whereas a find module has to accommodate all kind of weird installation layouts. You did the right thing here!

Ideally your config file now also provides imported targets for the binary and the library.

The imported target for the library makes it so that users don't need to write both include_directories(${COZ_INCLUDE_DIR}) and target_link_libraries(pca ${COZ_LIBRARY} (where they could misspell one or both of the variables), but could just write target_link_libraries(pca PRIVATE coz::libcoz) instead.
The benefit is that CMake will automatically cause this to error out if someone were to misspell a namespaced target (e.g. if you were to write coz::libczo instead).

The binary target could be used for making a custom target that runs an application under coz.

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 8, 2019

@Corristo I'm now hitting that point where I realise good modern cmake is not what's online I've only ever used variables like those 馃槺 I'll have to find the relevant docs for targets

@Corristo

This comment has been minimized.

Copy link

Corristo commented Oct 8, 2019

@xd009642 If you need some guidance just look at how the FindJPEG.cmake find module that ships with cmake itself creates the JPEG::JPEG target.

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 8, 2019

That was helpful for library and include dir. I was looking at this https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#target-properties for which one to put the coz binary on but nothing stood out to me as correct. BINARY_DIR was closest though but that's a directory..

@Corristo

This comment has been minimized.

Copy link

Corristo commented Oct 8, 2019

The binary needs to become its own target e.g. via add_executable(coz::profiler IMPORTED), and you need to set IMPORTED_LOCATION to the location of the executable.

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 8, 2019

Done! Thanks for the guidance

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 9, 2019

@ccurtsinger would you like the simple test case included in coz? Or is there the assumption that if someone wants to use cmake with a project using coz that they already know the fundamentals of cmake?

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 14, 2019

@ccurtsinger anything else you'd like to see in this PR re documentation etc?

@ccurtsinger

This comment has been minimized.

Copy link
Member

ccurtsinger commented Oct 15, 2019

If you could drop a CMake section into README.md that shows how to use this config then I'd be happy to accept the pull request. Thanks for the nice work!

xd009642 added 2 commits Oct 15, 2019
@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 15, 2019

Done 馃憤

@xd009642

This comment has been minimized.

Copy link
Contributor Author

xd009642 commented Oct 19, 2019

@ccurtsinger are you happy with the readme changes?

@ccurtsinger ccurtsinger merged commit 030f674 into plasma-umass:master Oct 20, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.