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 generating cmd*.rb , conf/*.yaml files #270

Closed

Conversation

harshmahesheka
Copy link
Contributor

@harshmahesheka harshmahesheka commented Jul 2, 2022

🎉 New feature

Closes gazebosim/gz-tools#72

Summary

Added Macros for generating cmd*.rb and conf/*.yaml files which decreases boilerplate code from repositories.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Test It

I have replaced the boilerplate code with macros in my fork here. You can build this and confirm that the correct cmd and conf are being generated and installed.

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Harsh Mahesheka <harsh.mahesheka.eee20@iitbhu.ac.in>
Signed-off-by: Harsh Mahesheka <harsh.mahesheka.eee20@iitbhu.ac.in>
@chapulina chapulina requested a review from arjo129 July 6, 2022 17:44
@chapulina chapulina added the enhancement New feature or request label Jul 23, 2022
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.

Tested and works. We can update downstream cmake files.

Signed-off-by: Harsh Mahesheka <harsh.mahesheka.eee20@iitbhu.ac.in>
@harshmahesheka
Copy link
Contributor Author

Tested and works. We can update downstream cmake files.

Created PR's for removing boilerplates from gz-transport and gz-msgs

@j-rivero
Copy link
Contributor

Not that I'm completely against this PR but would like to clarify first what happened, follow up gazebosim/gz-tools#72 (comment) please.

@azeey azeey self-requested a review December 5, 2022 19:57
#################################################
# ign_generate_conf()
# Generate conf/*.yaml files for projects
macro(ign_generate_conf)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the name of the macro to be a little more descriptive? Maybe just including ign-tools in the name is sufficient, eg. ign_tools_generate_conf or ign_generate_conf_for_ign_tools. @j-rivero might have better suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can by cycling on this but I still have the feeling that adding functions that are intrinsic related to gz-tools should be done in that repository. Since @harshmahesheka mentioned that the instruction in the Gazebo meeting was to use this repo, I'm going to try to accommodate that the decision here:

  • Following Split gzutils into functional pieces #344, I'll move these set of function to a new file.
  • Consider not to include the new file automatically into the find_package(gz-cmake) call and make it somehow optional.
  • The migration from Ign to Gz needs to be done
  • As @azeey suggested let's find a good name for them, something like gztools_generate_subcommands_files and consider to merge both features in once since seems to be that both ruby and yaml files are highly linked.

#################################################
# ign_generate_cmd()
# Generate cmd*.rb files for projects
macro(ign_generate_cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the name of the macro to be a little more descriptive in the same way as my comment above?


#################################################
# ign_generate_conf()
# Generate conf/*.yaml files for projects
Copy link
Contributor

Choose a reason for hiding this comment

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

Mention that these files are used by ign-tools.

#################################################
# ign_generate_conf()
# Generate conf/*.yaml files for projects
macro(ign_generate_conf)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we go ahead and use gz_ instead of ign_ since these are new macros?

@j-rivero
Copy link
Contributor

I'm going to close this one since we did not get any feedback from the author in some months. Please feel free to reopen if you are interested into working in the points listed in #270 (comment) and I will happily help as much as I can.

@j-rivero j-rivero closed this Apr 20, 2023
@harshmahesheka
Copy link
Contributor Author

Sorry, I missed this. Currently, I am busy with my exams, I will work on these and get back to you sometime later.

@j-rivero
Copy link
Contributor

Sorry, I missed this. Currently, I am busy with my exams, I will work on these and get back to you sometime later.

No worries, thanks @harshmahesheka .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel enhancement New feature or request 🏯 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.

Add cmake helper functions for configuring / generating cmd / conf files
5 participants