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

Hello world plugin added #699

Merged
merged 20 commits into from
Jul 1, 2021
Merged

Hello world plugin added #699

merged 20 commits into from
Jul 1, 2021

Conversation

addy1997
Copy link
Contributor

@addy1997 addy1997 commented Mar 21, 2021

I have added the hello_world plugin

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

@addy1997 addy1997 requested a review from chapulina as a code owner March 21, 2021 07:12
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Mar 21, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. I don't think this is being compiled right now, because there's no CMakeLists file.

This also seems to be very similar to SampleSystem. Maybe that example can be improved instead of adding another one? What are you envisioning Hello_world to help with that isn't accomplished by SampleSystem?

@addy1997
Copy link
Contributor Author

addy1997 commented Mar 22, 2021

Thank you for the PR. I don't think this is being compiled right now, because there's no CMakeLists file.

This also seems to be very similar to SampleSystem. Maybe that example can be improved instead of adding another one? What are you envisioning Hello_world to help with that isn't accomplished by SampleSystem?

@chapulina, I have added the CMakeLists.txt file.

@addy1997
Copy link
Contributor Author

Thank you for the PR. I don't think this is being compiled right now, because there's no CMakeLists file.

This also seems to be very similar to SampleSystem. Maybe that example can be improved instead of adding another one? What are you envisioning Hello_world to help with that isn't accomplished by SampleSystem?

Well, my only intention is to add a proper Hello world file just like the one in Gazebo classic.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Well, my only intention is to add a proper Hello world file just like the one in Gazebo classic.

Got it, you're talking about this one, right?

https://github.com/osrf/gazebo/tree/gazebo11/examples/plugins/hello_world

I have added the CMakeLists.txt file.

Our CI is failing because this plugin can't be compiled. Have you tried compiling it locally?

Can you also add a README to the plugin's folder like the newest example plugins have? See this for example. While working on the README, you should also follow the steps to check that the plugin works for you.

Current build failures
2021-03-22T20:24:36.7352330Z [ 50%] Building CXX object CMakeFiles/Hello_world.dir/Hello_world.cc.o
2021-03-22T20:24:36.7353488Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:6:19: error: expected '{' before ';' token
2021-03-22T20:24:36.7354229Z  namespace ignition;
2021-03-22T20:24:36.7354624Z                    ^
2021-03-22T20:24:36.7355519Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:7:17: error: expected '{' before ';' token
2021-03-22T20:24:36.7356257Z  namespace gazebo;
2021-03-22T20:24:36.7356625Z                  ^
2021-03-22T20:24:36.7357527Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:8:18: error: expected '{' before ';' token
2021-03-22T20:24:36.7358253Z  namespace systems;
2021-03-22T20:24:36.7358638Z                   ^
2021-03-22T20:24:36.7359622Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:15:1: error: qualified name does not name a class before '{' token
2021-03-22T20:24:36.7360344Z  {
2021-03-22T20:24:36.7360640Z  ^
2021-03-22T20:24:36.7361526Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:18:1: error: 'Hello_world' does not name a type
2021-03-22T20:24:36.7362570Z  Hello_world::Hello_world() : System(), dataPtr(std::make_unique<Hello_world>())
2021-03-22T20:24:36.7363106Z  ^~~~~~~~~~~
2021-03-22T20:24:36.7364162Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:26:6: error: 'MyPlugin' has not been declared
2021-03-22T20:24:36.7365051Z  void MyPlugin::Configure(const Entity &_entity,
2021-03-22T20:24:36.7365529Z       ^~~~~~~~
2021-03-22T20:24:36.7366443Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:26:32: error: 'Entity' does not name a type
2021-03-22T20:24:36.7367276Z  void MyPlugin::Configure(const Entity &_entity,
2021-03-22T20:24:36.7367764Z                                 ^~~~~~
2021-03-22T20:24:36.7368842Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:28:5: error: 'EntityComponentManager' has not been declared
2021-03-22T20:24:36.7369861Z      EntityComponentManager &_ecm,
2021-03-22T20:24:36.7370360Z      ^~~~~~~~~~~~~~~~~~~~~~
2021-03-22T20:24:36.7371326Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:29:5: error: 'EventManager' has not been declared
2021-03-22T20:24:36.7372152Z      EventManager &/*_eventMgr*/)
2021-03-22T20:24:36.7372580Z      ^~~~~~~~~~~~
2021-03-22T20:24:36.7373707Z /github/workspace/examples/plugin/Hello_world/Hello_world.cc:37:20: error: expected constructor, destructor, or type conversion before '(' token
2021-03-22T20:24:36.7374890Z  IGNITION_ADD_PLUGIN(Hello_world, System, Hello_world::ISystemConfigure)
2021-03-22T20:24:36.7375489Z                     ^
2021-03-22T20:24:36.7376482Z CMakeFiles/Hello_world.dir/build.make:62: recipe for target 'CMakeFiles/Hello_world.dir/Hello_world.cc.o' failed
2021-03-22T20:24:36.7377700Z make[3]: *** [CMakeFiles/Hello_world.dir/Hello_world.cc.o] Error 1
2021-03-22T20:24:36.7378796Z CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/Hello_world.dir/all' failed
2021-03-22T20:24:36.7379596Z make[2]: *** [CMakeFiles/Hello_world.dir/all] Error 2
2021-03-22T20:24:36.7381310Z Makefile:83: recipe for target 'all' failed
2021-03-22T20:24:36.7382003Z make[1]: *** [all] Error 2
2021-03-22T20:24:36.7383015Z �[1;36m[Dbg] [examples_build.cc:151] �[0m�[1;36mSource: �[0m�[1;36m/github/workspace/examples/plugin/command_actor�[0m�[1;36m�[0m
2021-03-22T20:24:36.7384175Z �[1;36m[Dbg] [examples_build.cc:157] �[0m�[1;36mBuild directory: �[0m�[1;36m/tmp/oTA1RF�[0m�[1;36m�[0m
2021-03-22T20:24:36.7385351Z �[1;36m[Dbg] [examples_build.cc:151] �[0m�[1;36mSource: �[0m�[1;36m/github/workspace/examples/plugin/custom_component�[0m�[1;36m�[0m
2021-03-22T20:24:36.7386513Z �[1;36m[Dbg] [examples_build.cc:157] �[0m�[1;36mBuild directory: �[0m�[1;36m/tmp/YQWpPd�[0m�[1;36m�[0m
2021-03-22T20:24:36.7387677Z �[1;36m[Dbg] [examples_build.cc:151] �[0m�[1;36mSource: �[0m�[1;36m/github/workspace/examples/plugin/gui_system_plugin�[0m�[1;36m�[0m
2021-03-22T20:24:36.7388822Z �[1;36m[Dbg] [examples_build.cc:157] �[0m�[1;36mBuild directory: �[0m�[1;36m/tmp/30O5P9�[0m�[1;36m�[0m
2021-03-22T20:24:36.7389968Z �[1;36m[Dbg] [examples_build.cc:151] �[0m�[1;36mSource: �[0m�[1;36m/github/workspace/examples/plugin/system_plugin�[0m�[1;36m�[0m
2021-03-22T20:24:36.7391123Z �[1;36m[Dbg] [examples_build.cc:157] �[0m�[1;36mBuild directory: �[0m�[1;36m/tmp/LUwUwT�[0m�[1;36m�[0m
2021-03-22T20:24:36.7392267Z �[1;36m[Dbg] [examples_build.cc:151] �[0m�[1;36mSource: �[0m�[1;36m/github/workspace/examples/plugin/Hello_world�[0m�[1;36m�[0m
2021-03-22T20:24:36.7395745Z �[1;36m[Dbg] [examples_build.cc:157] �[0m�[1;36mBuild directory: �[0m�[1;36m/tmp/WykYGa�[0m�[1;36m�[0m
2021-03-22T20:24:36.7396550Z /github/workspace/test/integration/examples_build.cc:164: Failure
2021-03-22T20:24:36.7397300Z Expected equality of these values:
2021-03-22T20:24:36.7397759Z   system(cmd)
2021-03-22T20:24:36.7398107Z     Which is: 512
2021-03-22T20:24:36.7398431Z   0
2021-03-22T20:24:36.7398745Z Hello_world
2021-03-22T20:24:36.7399392Z [  FAILED  ] Plugins/ExamplesBuild.Build/0, where GetParam() = "plugin" (56687 ms)

examples/plugin/Hello_world/Hello_world.cc Outdated Show resolved Hide resolved
examples/plugin/Hello_world/Hello_world.cc Outdated Show resolved Hide resolved
examples/plugin/Hello_world/Hello_world.cc Outdated Show resolved Hide resolved
examples/plugin/Hello_world/Hello_world.cc Outdated Show resolved Hide resolved
@addy1997
Copy link
Contributor Author

addy1997 commented Apr 5, 2021

Well, my only intention is to add a proper Hello world file just like the one in Gazebo classic.

Got it, you're talking about this one, right?

https://github.com/osrf/gazebo/tree/gazebo11/examples/plugins/hello_world

I have added the CMakeLists.txt file.

Our CI is failing because this plugin can't be compiled. Have you tried compiling it locally?

Can you also add a README to the plugin's folder like the newest example plugins have? See this for example. While working on the README, you should also follow the steps to check that the plugin works for you.
Current build failures

Yes, this plugin. No, I couldn't manage to compile it locally cause I got this error Unknown CMake command "ign_find_package". Also, I asked this on the Discord.

@addy1997 addy1997 requested a review from chapulina April 5, 2021 16:40
@addy1997 addy1997 closed this Apr 5, 2021
@addy1997 addy1997 deleted the Hello_world_plugin branch April 5, 2021 20:49
@addy1997 addy1997 restored the Hello_world_plugin branch April 5, 2021 21:12
@addy1997 addy1997 reopened this Apr 6, 2021
addy1997 added 2 commits April 6, 2021 21:09
Signed-off-by: addy1997 <[email protected]>
@addy1997
Copy link
Contributor Author

addy1997 commented Apr 6, 2021

@chapulina Ma'am, it was not the slightest of my intention to waste your time(gazebosim/ros_gz#148). I have updated the code and have a compiled plugin (terminal output below) for your reference.

##Terminal Output


adwaitnaik@ubuntu:~/plugin_ws/src/hello_world/build$ make 
Scanning dependencies of target HelloWorld
[ 50%] Building CXX object CMakeFiles/HelloWorld.dir/HelloWorld.cc.o
[100%] Linking CXX shared library libHelloWorld.so
[100%] Built target HelloWorld

I could compile it locally. Lastly, here's the library.

Thanks

PS: You can still reject it.

@chapulina
Copy link
Contributor

it was not the slightest of my intention to waste your time

Sorry if that's what it sounded like! I completely understand you have the best intentions, I was just making suggestions on how to optimize the review process 😉 This is in the review queue 👍

@addy1997
Copy link
Contributor Author

addy1997 commented Apr 22, 2021

@chapulina, any update on this 🙂?

@chapulina
Copy link
Contributor

Hi @addy1997 , the plugin still can't be compiled. I see you're using ROS packages like catkin, roscpp and rospy, which aren't dependencies of Ignition. It may compile on your system if you have those on your workspace, but to be included into our examples, it can't depend on ROS.

You can see how the test fails to compile on our CI. Click on the "Details" for the "Ubuntu CI" check and see on the logs how your plugin fails to compile:

2021-04-06T16:56:24.0205669Z CMake Error at CMakeLists.txt:10 (find_package):
2021-04-06T16:56:24.0206438Z   By not providing "Findcatkin.cmake" in CMAKE_MODULE_PATH this project has
2021-04-06T16:56:24.0207313Z   asked CMake to find a package configuration file provided by "catkin", but
2021-04-06T16:56:24.0207940Z   CMake did not find one.
2021-04-06T16:56:24.0208229Z 
2021-04-06T16:56:24.0208782Z   Could not find a package configuration file provided by "catkin" with any
2021-04-06T16:56:24.0209435Z   of the following names:
2021-04-06T16:56:24.0209734Z 
2021-04-06T16:56:24.0210132Z     catkinConfig.cmake
2021-04-06T16:56:24.0210815Z     catkin-config.cmake
2021-04-06T16:56:24.0211158Z 
2021-04-06T16:56:24.0211699Z   Add the installation prefix of "catkin" to CMAKE_PREFIX_PATH or set
2021-04-06T16:56:24.0212476Z   "catkin_DIR" to a directory containing one of the above files.  If "catkin"
2021-04-06T16:56:24.0213290Z   provides a separate development package or SDK, be sure it has been
2021-04-06T16:56:24.0213882Z   installed.
2021-04-06T16:56:24.0214144Z 
2021-04-06T16:56:24.0214357Z 
2021-04-06T16:56:24.0215007Z -- Configuring incomplete, errors occurred!
2021-04-06T16:56:24.0215720Z See also "/tmp/T57Zfo/CMakeFiles/CMakeOutput.log".
2021-04-06T16:56:24.0218106Z �[1;36m[Dbg] [examples_build.cc:151] �[0m�[1;36mSource: �[0m�[1;36m/github/workspace/examples/plugin/command_actor�[0m�[1;36m�[0m
2021-04-06T16:56:24.0219270Z �[1;36m[Dbg] [examples_build.cc:157] �[0m�[1;36mBuild directory: �[0m�[1;36m/tmp/d6ceuw�[0m�[1;36m�[0m
2021-04-06T16:56:24.0221105Z �[1;36m[Dbg] [examples_build.cc:151] �[0m�[1;36mSource: �[0m�[1;36m/github/workspace/examples/plugin/system_plugin�[0m�[1;36m�[0m
2021-04-06T16:56:24.0222246Z �[1;36m[Dbg] [examples_build.cc:157] �[0m�[1;36mBuild directory: �[0m�[1;36m/tmp/hY8fHb�[0m�[1;36m�[0m
2021-04-06T16:56:24.0223371Z �[1;36m[Dbg] [examples_build.cc:151] �[0m�[1;36mSource: �[0m�[1;36m/github/workspace/examples/plugin/Hello_world�[0m�[1;36m�[0m
2021-04-06T16:56:24.0224486Z �[1;36m[Dbg] [examples_build.cc:157] �[0m�[1;36mBuild directory: �[0m�[1;36m/tmp/T57Zfo�[0m�[1;36m�[0m
2021-04-06T16:56:24.0225276Z /github/workspace/test/integration/examples_build.cc:164: Failure
2021-04-06T16:56:24.0225971Z Expected equality of these values:
2021-04-06T16:56:24.0226439Z   system(cmd)
2021-04-06T16:56:24.0226809Z     Which is: 256
2021-04-06T16:56:24.0227126Z   0
2021-04-06T16:56:24.0227777Z Hello_world

Also, please clean up the code and remove all references to SampleSystem, as well as the usage of Hello_world instead of HelloWorld.

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

@chapulina, I have checked the log files it says this [0 errors, 2 warnings], rest of the checks seem to pass through. There doesn't seem to be any syntax-related errors in the log file.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Cool, the test passes now, which means the plugin compiles correctly!

Could you remove the .so file? We don't commit those to the repository, because they may not work for all users across all platforms.

examples/plugin/Hello_world/package.xml Outdated Show resolved Hide resolved
examples/plugin/Hello_world/HelloWorld.cc Outdated Show resolved Hide resolved
examples/plugin/Hello_world/HelloWorld.hh Outdated Show resolved Hide resolved
examples/plugin/Hello_world/README.md Outdated Show resolved Hide resolved
examples/plugin/Hello_world/HelloWorld.hh Outdated Show resolved Hide resolved
Signed-off-by: addy1997 <[email protected]>
@addy1997 addy1997 requested a review from chapulina April 28, 2021 19:58
@addy1997
Copy link
Contributor Author

addy1997 commented May 4, 2021

@chapulina, are there any more changes required in this plugin? If not, could you please approve the workflow request?

@chapulina
Copy link
Contributor

Hi @addy1997 , this is on my review queue 👍

@scpeters scpeters requested a review from j-rivero May 17, 2021 18:21
@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #699 (196106f) into ign-gazebo4 (f6a1a84) will decrease coverage by 0.01%.
The diff coverage is 33.33%.

❗ Current head 196106f differs from pull request most recent head 79bbfa9. Consider uploading reports for the commit 79bbfa9 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo4     #699      +/-   ##
===============================================
- Coverage        65.98%   65.97%   -0.02%     
===============================================
  Files              239      239              
  Lines            17657    17657              
===============================================
- Hits             11651    11649       -2     
- Misses            6006     6008       +2     
Impacted Files Coverage Δ
src/rendering/RenderUtil.cc 42.08% <0.00%> (-0.05%) ⬇️
src/systems/diff_drive/DiffDrive.cc 86.57% <100.00%> (-0.07%) ⬇️
src/Server.cc 82.80% <0.00%> (-0.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d2c076...79bbfa9. Read the comment docs.

@addy1997 addy1997 requested a review from j-rivero June 4, 2021 18:03
@addy1997
Copy link
Contributor Author

@chapulina, please could you merge this PR?

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thank you for iterating, @addy1997 ! I pushed some changes in 79bbfa9 to update style and add comments and instructions to help new users understand what's happening.

Let me know if you're ok with the changes!

@addy1997
Copy link
Contributor Author

addy1997 commented Jul 1, 2021

Thank you for iterating, @addy1997 ! I pushed some changes in 79bbfa9 to update style and add comments and instructions to help new users understand what's happening.

Let me know if you're ok with the changes!

Yes, I am good with the changes.

@chapulina chapulina merged commit 12f1237 into gazebosim:ign-gazebo4 Jul 1, 2021
chapulina added a commit that referenced this pull request Jul 23, 2021
Signed-off-by: addy1997 <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Jose Luis Rivero <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
chapulina added a commit that referenced this pull request Jul 23, 2021
Signed-off-by: addy1997 <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Jose Luis Rivero <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants