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

WIP: Move CLUE algo in external library #17

Closed
wants to merge 6 commits into from

Conversation

ebrondol
Copy link
Collaborator

BEGINRELEASENOTES

  • The CLUE algorithm (both CPU and GPU implementations) are now an external library
  • Currently the library was created with the files included in this package
  • In the future the library will be created and updated using the standalone repository

ENDRELEASENOTES

@ebrondol
Copy link
Collaborator Author

ebrondol commented Jan 27, 2022

It would be awesome if @fdplacido and @vvolkl can have a look, given their experience in cmake and external libraries. In particular it is not clear to me how to treat now the clueLib folder, if there is some content to be removed and if these lines are still necessary.

@ebrondol ebrondol requested review from fdplacido and vvolkl January 27, 2022 17:46
set_property(TARGET CLUE APPEND PROPERTY IMPORTED_CONFIGURATIONS NOCONFIG)
set_target_properties(CLUE PROPERTIES
IMPORTED_LINK_INTERFACE_LANGUAGES_NOCONFIG "CXX"
IMPORTED_LOCATION_NOCONFIG "${_IMPORT_PREFIX}/lib64/libCLUE.a"
Copy link
Contributor

Choose a reason for hiding this comment

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

this lib needs to be compiled first, and linked by using target_link_libraries

@@ -0,0 +1,4 @@
## -*- ascii -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove config generated files

Copy link
Contributor

@fdplacido fdplacido left a comment

Choose a reason for hiding this comment

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

The current PR won't compile.
The general approach would be to compile clueLib as a library target using add_library as you are doing, compile the k4Clue (the wrapper) as the executable, and then target_link_library clueLib to k4Clue.
If in the future the lib lives in a different repo we would need to deal with it as a submodule (or using FetchContent which I would favor), and compiler in a similar way.

For the clueLib folder I would remove the lib, lib64, python and share folders.

For the source_group I don't think you need it, and I would also favor including all the files explicitly, or doing include_directories.

This would be:

  • Remove generated stuff in clueLib
  • Create a CMakeLists.txt for clueLib that compiles it as a lib, and call add_subdirectoryfor clueLib first
  • In k4Clue link to the clueLib
  • Consider for the future if using git submodule or FetchContent

@ebrondol
Copy link
Collaborator Author

ebrondol commented Feb 3, 2022

Thanks for the feedback. I am going to close this PR and create a new one.

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

Successfully merging this pull request may close these issues.

Move CLUE algo in external library
2 participants