Open j-medland opened 1 year ago
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
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 rosdep
is missing from the macOS build and causing it to fail
Note - it looks
rosdep
is missing from the macOS build and causing it to fail
This must be something that changed recently. This pipeline used to work previously.
Can you try to update the ros-tooling/setup-ros@v0.3
action to ros-tooling/setup-ros@v0.4
? Maybe this fixes the issue.
Looking at the CI logs, rosdep
actually gets installed by pip but the command cannot be found. I believe this is a bug in the GitHub action ros-tooling/setup-ros
that is caused by updating the macOS version on the runners (https://github.blog/changelog/2022-10-03-github-actions-jobs-running-on-macos-latest-are-now-running-on-macos-12/).
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?
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.
I am still interested and hoping to give it another pass with recent updates and your suggestions soon.
Cheers!
Finally, the
apriltag.pc
references only the now legacyapriltag
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.
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.so
library for backwards compatibility but added the following libraries (with associated cmake exports)libapriltag-detector.so
just the apriltag detector with the exports limited to the detector APIlibapriltag-utils.so
the utility functions used in the exampleslibapriltag-tags.so
all the tag librarieslibapriltag-<tag_family>.so
each 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_image
which provides minimal functionality to write thatimage
to apnm
file and then deallocate it (apriltag_image_write_pnm
&apriltag_image_destroy
)Control of exports is achieved via macro definition and cmake's
generate_export_headers
although its usage is slightly complicated by the fact that multiple targets use the sameapriltag_export.h
. TheWINDOWS_EXPORT_ALL
target 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.txt
should 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/apriltag
should provide better control for users integrating this code into other projects.Happy to hear your thoughts and discuss any changes.
John