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

Build as sub components with defined exports #264

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

j-medland
Copy link
Contributor

@j-medland j-medland commented Oct 24, 2022

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 API
libapriltag-utils.so the utility functions used in the examples
libapriltag-tags.so all the tag libraries
libapriltag-<tag_family>.so each tag family as a standalone library for users who don't need every tag library

I have added the two functions to the apriltag-detector API to compliment the existing apriltag_to_image which provides minimal functionality to write that image to a pnm 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 same apriltag_export.h. The WINDOWS_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 an include/apriltag should provide better control for users integrating this code into other projects.

Happy to hear your thoughts and discuss any changes.

John

@j-medland
Copy link
Contributor Author

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.

@j-medland
Copy link
Contributor Author

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 j-medland reopened this Nov 4, 2022
@j-medland
Copy link
Contributor Author

j-medland commented Nov 4, 2022

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

@christian-rauch
Copy link
Collaborator

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/[email protected] action to ros-tooling/[email protected]? Maybe this fixes the issue.

@christian-rauch
Copy link
Collaborator

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.

@christian-rauch
Copy link
Collaborator

Can you rebase your PR to fix the CI issue?



# Python wrapper
## Python wrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a single # just like in the other cases.

if(BUILD_EXAMPLES)
# build examples
add_subdirectory(example)
endif()
Copy link
Collaborator

@christian-rauch christian-rauch Dec 5, 2022

Choose a reason for hiding this comment

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

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()
Copy link
Collaborator

Choose a reason for hiding this comment

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

new line missing

)
endfunction()

function(set_apriltag_export_all target_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a 1~2 line comment to each of the three functions to explain what they are doing?

Comment on lines +8 to +9
## apriltag-tags (Apriltag's tag libraries)
## apriltag-<tag_family_name> e.g apriltag-tagStandard41h12 (Individual libraries for each tag family)
Copy link
Collaborator

@christian-rauch christian-rauch Dec 5, 2022

Choose a reason for hiding this comment

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

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})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see SRC_HEADERS defined anywhere. I think this is empty and can be removed.

Copy link
Collaborator

@christian-rauch christian-rauch left a comment

Choose a reason for hiding this comment

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

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.

@j-medland
Copy link
Contributor Author

j-medland commented Dec 8, 2022

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.

Copy link
Collaborator

@christian-rauch christian-rauch left a comment

Choose a reason for hiding this comment

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

What is the status of this PR? Is there still interest in merging this?

if(BUILD_EXAMPLES)
# build examples
add_subdirectory(example)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

new line missing

endif(OpenCV_FOUND)

# install example programs
install(TARGETS apriltag_demo RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR})
Copy link
Collaborator

Choose a reason for hiding this comment

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

new line missing

configure_file(${PROJECT_NAME}.pc.in ${PROJECT_NAME}.pc @ONLY)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}.pc
DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

new line missing


int apriltag_image_write_pnm(image_u8_t * im, const char* path){
return image_u8_write_pnm(im,path);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

new line missing

@j-medland
Copy link
Contributor Author

I am still interested and hoping to give it another pass with recent updates and your suggestions soon.

Cheers!

@eli-schwartz
Copy link

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.

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.

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.

3 participants