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

Macros for creating new gazebo package #262

Closed
wants to merge 19 commits into from

Conversation

harshmahesheka
Copy link
Contributor

@harshmahesheka harshmahesheka commented May 24, 2022

🎉 New feature

Summary

This pull request is regarding my GSoC project which you can visit here .The idea is to create a series of macros that will help both new and old users in creating new packages quickly. The current plan includes the creation of-

  1. ign_add_resources (/path/to/models_or_world)-This will install all the models
    inside the mentioned folder and add required paths in hooks.

  2. ign_add_plugins(/path/to/plugin)-This will add all the plugins inside the
    mentioned folder as libraries, install them, and add required paths in hooks. We can
    have an option for adding common target_link_libraries, dependencies, etc., and an
    option if you want to add something to some specific plugin from the folder.

  3. ign_add_executables(/path/to/example)-This will be similar to the last one, just
    instead of libraries; this will add files as executables.

  4. ign_add_tests(/path/to/tests)-First, we can add macros for gtest for the
    non-amnet package(basically a counterpart to ament_cmake_gtest), and then similar
    to the last 2, this will build all the tests inside the directory with options to add
    common and specific dependencies.

  5. ign_add_msgs(/path/to/messages)-Similar to the above macros this will install and source all the messages inside the folder while providing users all customization options like add_libraray etc.

  6. ign_environment_hook(): This will generate and install hooks based on the macros
    mentioned above.

For more information, you can visit my proposal.

Currently, I have created add resources, executables, plugins, and ign_environment_hook. This pr will be a draft pr while I work on this project and since this is a quite user-based project any kind of review or suggestion on existing macros or idea of creating some new ones are most welcomed.
(Also currently, all my code uses old names like ign which I will change once ign-cmake undergoes these changes)

Test it

I have created a test_package,simply clone and build it to see macros in action for now.I will work on proper tests in meantime.

@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels May 24, 2022
Signed-off-by: Harsh Mahesheka <[email protected]>
Signed-off-by: Harsh Mahesheka <[email protected]>
Signed-off-by: Harsh Mahesheka <[email protected]>
Signed-off-by: Harsh Mahesheka <[email protected]>
Signed-off-by: Harsh Mahesheka <[email protected]>
Signed-off-by: Harsh Mahesheka <[email protected]>
@chapulina chapulina requested a review from arjo129 May 24, 2022 16:42
Copy link

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

I've done a really quick first pass. These are areas which you may want to change.

cmake/IgnUtils.cmake Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
macro(ign_add_plugins path_to_plugin )

file(GLOB source_list CONFIGURE_DEPENDS
"${path_to_plugin}/*.cc"
Copy link

Choose a reason for hiding this comment

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

Again this assumes certain folder structure. Its fine to keep it this way for early prototyping as we iterate on the tool but, it would be better if we had them directly adding files to this call. Further what if someone wants to add dependencies.

@arjo129
Copy link

arjo129 commented May 25, 2022

Also some thought about API design:
Currently we pass in .cc files that belong to a folder. What if the end user does not use .cc? What if the end user needs to do some fancy code generation?

A cleaner API would be to have

add_library(MyAwesomePlugin file1.cc file2.cc)
target_link_library(MyAwesomePlugin <own libraries here like opencv, pcl>)
ign_export_plugin(MyAwesomePlugin)

@harshmahesheka
Copy link
Contributor Author

There is a function to include dependencies for all files as well as individual files. Also, the idea of doing such a thing was to help users install all files in a go. I think what we can do is users can add individual files and if we write all.cc(or any other format) all files of that format will be installed. We can implement your API design also basically separating export feature so, that if users want to build single files he can do this and if he wants to build all files he can just enter the extension in my current macros design.

cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
Signed-off-by: Harsh Mahesheka <[email protected]>
@harshmahesheka
Copy link
Contributor Author

I have completed macros for resources, plugins(system and GUI), messages, and executables along with macros for exporting paths. For now, I have added basic comments which I am planning to elaborate on one's we finalize the code. I am also working on exporting paths for the plain cmake dependent package which I might push soon. Would love to hear everyone's reviews

@harshmahesheka
Copy link
Contributor Author

I have tried to add a mechanism for exporting paths in plain cmake build-type packages. To keep things easy to review, I have created a separate pull request in my fork here. Also, I have updated my testing_package to use all of the macros. This can act as an elementary testing mechanism for the macros.

Signed-off-by: Harsh Mahesheka <[email protected]>
Plain-cmake export mechanism added
@harshmahesheka harshmahesheka marked this pull request as ready for review August 25, 2022 21:28
Copy link

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

I've done a cursory pass. Quite a few things stand out here. In particular please document all your functions like the other macros do. See here for an example.

cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
file(APPEND ${CMAKE_CURRENT_SOURCE_DIR}/hooks/hook.dsv.in
"prepend-non-duplicate;${variable_export_path}\n")
endforeach()
file(WRITE ${CMAKE_CURRENT_SOURCE_DIR}/colcon.pkg "{\n \"hooks\": [\"share/${CMAKE_PROJECT_NAME}/hooks/hook.dsv\"]\n}")
Copy link

Choose a reason for hiding this comment

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

I'm again not sure we want this here as the first run this will not work. We could alternatively just have these as part of the template you generate in https://github.com/gazebosim/gz_pkg_create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can leave it here and create it there as well because someone might not use that tool but use these macros

Signed-off-by: Harsh Mahesheka <[email protected]>
@harshmahesheka
Copy link
Contributor Author

I have added detailed documentation for all the macros.

Copy link

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Done a really quick pass. Will look into more depth in the morning.

cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
cmake/IgnUtils.cmake Outdated Show resolved Hide resolved
Signed-off-by: Harsh Mahesheka <[email protected]>
@j-rivero
Copy link
Contributor

j-rivero commented Oct 27, 2022

Thanks @harshmahesheka @arjo129 for the work. I see value in adding helper functions for helping with the creation of projects based on gz-. Are the functions in this PR prepared to be used by the other gz- libraries in the family (i.e: by gz-math or gz-transport)?

If the new functions should be ready to be used internally, I would recommend to work on transitioning an existing gz- library to this PR since it can reveal some issues, i.e: some of them use GZ_DATA_INSTALL_DIR for the place to put world files and I see here a hardcoded share directory which probably will break multi-platform support.

@arjo129
Copy link

arjo129 commented Nov 1, 2022

I believe these are meant for external consumption to make it easier for beginners to get started with environment and model creation.

@j-rivero
Copy link
Contributor

For this PR guys, I'm sorry but I still have many doubts on how to introduce helper functions that are dedicated for beginners that are not going in the main API of the library to be used by gz- libs or others. Again, I see value in adding them but more work would be required to properly place theses macros in a place/package where we are sure that they will be understood correctly. The PR also needs a migration from Ign to Gz. Please create an issue if you want me to help to revive this idea and have the time to work on changes. Thanks again.

@j-rivero j-rivero closed this Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🏯 fortress Ignition Fortress Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants