Build as sub components with defined exports #264
Conversation
|
Ah, looks like from the tests I forgot I was trying some newer cmake features. I think rolling back to 3.5 provides everything I need. 3.12 just provides some handy generator expressions. |
|
I am currently tweaking this against the CI system so once that all passes, I will squash everything but feel free to take a look now if you want. John |
j-medland
force-pushed
the
master
branch
2 times, most recently
from
a36d4a7
to
20a07f4
Compare
|
I think this is now all ready for review (after some rebase/squash commit mistakes on my part). I am happy to hear feedback and make changes as folks see fit. I did not integrate a command-line-interface application as @dkogan suggested in #256 but I believe these changes should make packaging as they suggest require fewer patches. This PR also includes the optional builds of the examples as mentioned in #266. As I mentioned in my comments, managing the exports also allows this library to be build as a DLLs. I have also had success creating a vcpkg portfile based on this commit which would allow easier integration into other projects and address microsoft/vcpkg#12171. Note - it looks |
This must be something that changed recently. This pipeline used to work previously. Can you try to update the |
|
Looking at the CI logs, Since this is a fairly huge PR, I would now wait for the reviews anyway. In the meantime, the issue in the action maybe gets fixed. Otherwise, we can just flag the macOS CI as optional or remove it again. |
|
Can you rebase your PR to fix the CI issue? |
|
|
||
|
|
||
| # Python wrapper | ||
| ## Python wrapper |
There was a problem hiding this comment.
This should be a single # just like in the other cases.
| if(BUILD_EXAMPLES) | ||
| # build examples | ||
| add_subdirectory(example) | ||
| endif() |
There was a problem hiding this comment.
There is a new line missing here.
| # multiple libs sharing same auto-generated header so override default to use apriltag_EXPORTS | ||
| DEFINE_SYMBOL apriltag_EXPORTS | ||
| ) | ||
| endfunction() |
| ) | ||
| endfunction() | ||
|
|
||
| function(set_apriltag_export_all target_name) |
There was a problem hiding this comment.
Can you add a 1~2 line comment to each of the three functions to explain what they are doing?
| ## apriltag-tags (Apriltag's tag libraries) | ||
| ## apriltag-<tag_family_name> e.g apriltag-tagStandard41h12 (Individual libraries for each tag family) |
There was a problem hiding this comment.
Why is there a library containing all tags and individual libraries per tag? Until we can load tags via plugins, I would suggest that one library containing all standard tags is sufficient. I also suggest using single # for comments.
| endforeach() | ||
|
|
||
| # All-Tag-Families library | ||
| add_library(${PROJECT_NAME}-tags ${SRC_TAGS} ${SRC_HEADERS}) |
There was a problem hiding this comment.
I don't see SRC_HEADERS defined anywhere. I think this is empty and can be removed.
There was a problem hiding this comment.
This is great work. I like the ability to only link the core functionality. Also, the new file layout makes it easier to understand the different parts of the repo.
The "apriltag" folder now contains a single CMakeLists.txt defining multiple libraries. To better reflect this split, it would also make sense to split the "apriltag" folder into a file layout that mirrors those libraries with individual source folders and CMakeLists.txt per library. E.g. there could be detector and tags folder containing the sources and CMakeLists.txt for the apriltag-detector and apriltag-tags.
Some sources in the common folder, such as homography.c and workerpool.h, are only used in the detector library. There might be more common sources that are only used in one of the libraries. I think it would make sense to move source that only belong to one of the libraries out of common into the folder for that specific library (detector, tags, ...).
Finally, the apriltag.pc references only the now legacy apriltag library. Can CMake generate new pkg-config files for the libraries (I know meson can do this)? Otherwise, this new improved separation of functionality will not be available outside of CMake.
|
Thanks for the feedback. I'll hopefully have some time over the next couple of weeks to work through the changes you have proposed. I'll update when things are ready for review again. |
| if(BUILD_EXAMPLES) | ||
| # build examples | ||
| add_subdirectory(example) | ||
| endif() |
| endif(OpenCV_FOUND) | ||
|
|
||
| # install example programs | ||
| install(TARGETS apriltag_demo RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}) |
| configure_file(${PROJECT_NAME}.pc.in ${PROJECT_NAME}.pc @ONLY) | ||
| install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}.pc | ||
| DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig | ||
| ) |
|
|
||
| int apriltag_image_write_pnm(image_u8_t * im, const char* path){ | ||
| return image_u8_write_pnm(im,path); | ||
| } |
|
I am still interested and hoping to give it another pass with recent updates and your suggestions soon. Cheers! |
cmake doesn't have a native mechanism to do this, but you can just keep track of library names for dependencies in a second list, and format the pkg-config file as a template. |
My first pass at a PR related to #256
The aim here is to provide sub-component libraries for users who don't need/want all the bundled utility functions.
I have maintained the behaviour of the existing
libapriltag.solibrary for backwards compatibility but added the following libraries (with associated cmake exports)libapriltag-detector.sojust the apriltag detector with the exports limited to the detector APIlibapriltag-utils.sothe utility functions used in the exampleslibapriltag-tags.soall the tag librarieslibapriltag-<tag_family>.soeach tag family as a standalone library for users who don't need every tag libraryI have added the two functions to the apriltag-detector API to compliment the existing
apriltag_to_imagewhich provides minimal functionality to write thatimageto apnmfile and then deallocate it (apriltag_image_write_pnm&apriltag_image_destroy)Control of exports is achieved via macro definition and cmake's
generate_export_headersalthough its usage is slightly complicated by the fact that multiple targets use the sameapriltag_export.h. TheWINDOWS_EXPORT_ALLtarget property (requires cmake 3.4) is used to export all functions from libraries on Windows. Having Windows defines means this library builds usable DLLs on Windows.I have made a big change to the project's structure - separating the library components with their own
CMakeLists.txtshould hopefully make life easier for longer-term maintenance and moving away from globs and placing the API headers (those distributed with the binaries) into aninclude/apriltagshould provide better control for users integrating this code into other projects.Happy to hear your thoughts and discuss any changes.
John