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.

Sign up for GitHub

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

Jump to bottom

Depend on ignition-utils1 cli component #55

Merged
merged 4 commits into from
Aug 9, 2021
Merged

Depend on ignition-utils1 cli component #55

merged 4 commits into from
Aug 9, 2021

Conversation

scpeters
Copy link
Member

🎉 New feature

Precursor to #54

Summary

This adds a dependency on the ignition-utils1::cli component that will be used to resolve #54. This also changes any includes of ignition/utilities/SuppressWarning.hh to ignition/utils/SuppressWarning.hh and changes the exported target dependency accordingly. This may impact downstream packages if they are using the ignition/utilities headers implicitly from this package. I'll monitor the nightly builds.

Test it

It should still compile like normal and pass all tests.

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

Note to maintainers: Remember to use Squash-Merge

@scpeters
Depend on ignition-utils1 with cli component
f9f27bc 
Signed-off-by: Steven Peters <[email protected]>
@scpeters
Use SuppressWarning.hh from ignition-utils
7f3df50 
Use the headers from ignition/utils instead of ignition/utilities.
This also exports the dependency on ignition-utils1::ignition-utils1
instead of ignition-cmake2::utilities.

Signed-off-by: Steven Peters <[email protected]>
@codecov
Copy link

codecov bot commented Jul 24, 2021 •
edited
Loading

Codecov Report

Merging #55 (eacfd36) into main (87b2b3b) will not change coverage.
The diff coverage is n/a.

❗ Current head eacfd36 differs from pull request most recent head 17c2ce2. Consider uploading reports for the commit 17c2ce2 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main      #55   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          15       15           
  Lines         584      584           
=======================================
  Hits          583      583           
  Misses          1        1
Impacted Files Coverage Δ
core/include/ignition/plugin/detail/Factory.hh 100.00% <ø> (ø)
...egister/include/ignition/plugin/detail/Register.hh 100.00% <ø> (ø)

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 87b2b3b...17c2ce2. Read the comment docs.

@scpeters scpeters mentioned this pull request Jul 24, 2021
Merged
@chapulina
Copy link
Contributor

This may impact downstream packages if they are using the ignition/utilities headers implicitly from this package. I'll monitor the nightly builds.

I don't think any downstream packages are using ign-plugin2, so this may not manifest just yet

adlarkin
adlarkin reviewed Jul 26, 2021
View reviewed changes
tutorials/02_installation.md Outdated
@@ -43,7 +43,7 @@ sudo apt install libignition-plugin-dev

1. Install Ignition dependencies
```
sudo apt-get install libignition-cmake2-dev
sudo apt-get install libignition-cmake2-dev libignition-utils1-cli-dev
Copy link

Choose a reason for hiding this comment

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

The packages.apt file for this repo also has libignition-tools-dev, should that be added here? https://github.com/ignitionrobotics/ign-plugin/blob/7f3df50589c996b88aba9a64b7e15f7f7097e40b/.github/ci/packages.apt#L2

Copy link
Member Author

Choose a reason for hiding this comment

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

17c2ce2

@scpeters scpeters mentioned this pull request Aug 7, 2021
Merged
@scpeters
Copy link
Member Author

scpeters commented Aug 7, 2021

updating ign-plugin windows builds to use colcon in gazebo-tooling/release-tools#495

ahcorde
ahcorde reviewed Aug 9, 2021
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

When this PR gazebo-tooling/release-tools#495 is in. I will run the CI again

@scpeters scpeters merged commit 91c3e64 into gazebosim:main Aug 9, 2021
@scpeters scpeters deleted the depend_utils_cli branch August 9, 2021 17:53
@scpeters scpeters mentioned this pull request Aug 9, 2021
Merged
ahcorde pushed a commit that referenced this pull request Nov 3, 2021
@scpeters @ahcorde
Depend on ignition-utils1 cli component (#55)
891ae70 
* Use SuppressWarning.hh from ignition-utils

Use the headers from ignition/utils instead of ignition/utilities.
This also exports the dependency on ignition-utils1::ignition-utils1
instead of ignition-cmake2::utilities.

* rm configure.bat
* Add libignition-tools-dev to tutorial

Signed-off-by: Steve Peters <[email protected]>
@chapulina chapulina mentioned this pull request Apr 14, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adlarkin adlarkin adlarkin left review comments

@ahcorde ahcorde ahcorde approved these changes

@chapulina chapulina Awaiting requested review from chapulina

@mxgrey mxgrey Awaiting requested review from mxgrey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@scpeters @chapulina @ahcorde @adlarkin