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

Bumps in garden : ign-cmake3, ign-util2, ign-plugin2 #74

Merged
merged 5 commits into from
Apr 11, 2022

Conversation

methylDragon
Copy link
Contributor

Signed-off-by: Brandon Ong <brandon@openrobotics.org>
@methylDragon methylDragon changed the title Bumps in garden : ign-plugin2 Bumps in garden : ign-cmake3, ign-util2, ign-plugin2 Apr 6, 2022
@methylDragon
Copy link
Contributor Author

methylDragon commented Apr 6, 2022

Run with command:
./bump_dependency.bash garden "ign-cmake;ign-utils;ign-plugin" "3;2;2" 685 garden


https://github.com/ignitionrobotics/ign-plugin/pull/74/files#diff-b171701a8039aa411e8f6a1710925af464824660092863f0ff3f4e426f89402bL259 might be an edge case, perhaps an instance of greedy matching? It doesn't look like plugin1 should have been replaced with plugin2

Also, I'm not sure ign-common5 and ign-math7 was bumped... Will it cause downstream issues?

@chapulina
Copy link
Contributor

chapulina commented Apr 6, 2022

https://github.com/ignitionrobotics/ign-plugin/pull/74/files#diff-b171701a8039aa411e8f6a1710925af464824660092863f0ff3f4e426f89402bL259 might be an edge case, perhaps an instance of greedy matching?

Ouch yes, that doesn't need to change

Also, I'm not sure ign-common5 and ign-math7 was bumped... Will it cause downstream issues?

Oh that was actually the script being kinda smart! It's unusual that a library has a reference to libraries that aren't direct dependencies, which is the case of ign-math and ign-common here. But if it will refer to them, it should refer to the versions that are supported together. This shouldn't cause issues and it's a good change 👍🏽

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.

Just one extra comment besides the other issue that you already caught

CMakeLists.txt Outdated Show resolved Hide resolved
examples/CMakeLists.txt Outdated Show resolved Hide resolved
test/integration/plugin.cc Outdated Show resolved Hide resolved
methylDragon and others added 2 commits April 7, 2022 01:46
Co-authored-by: Louise Poubel <louise@openrobotics.org>
@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #74 (bff7428) into main (940e918) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #74   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files          17       17           
  Lines         616      616           
=======================================
  Hits          614      614           
  Misses          2        2           

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 940e918...bff7428. Read the comment docs.

@methylDragon
Copy link
Contributor Author

@osrf-jenkins run tests please!

CMakeLists.txt Outdated Show resolved Hide resolved
@chapulina
Copy link
Contributor

Windows has a new warning?!

[5.063s] CMake Warning:
[5.063s]   Manually-specified variables were not used by the project:
[5.063s] 
[5.063s]     CMAKE_TOOLCHAIN_FILE

@methylDragon
Copy link
Contributor Author

methylDragon commented Apr 8, 2022

Windows has a new warning?!

[5.063s] CMake Warning:
[5.063s]   Manually-specified variables were not used by the project:
[5.063s] 
[5.063s]     CMAKE_TOOLCHAIN_FILE

It passed on my push, even though nothing changed 😬

But it does still print that CMake projects should use: "-DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake", just that no warning seems to have been emitted?

@scpeters
Copy link
Member

scpeters commented Apr 8, 2022

Windows has a new warning?!

[5.063s] CMake Warning:
[5.063s]   Manually-specified variables were not used by the project:
[5.063s] 
[5.063s]     CMAKE_TOOLCHAIN_FILE

It passed on my push, even though nothing changed 😬

But it does still print that CMake projects should use: "-DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake", just that no warning seems to have been emitted?

the windows builds do not fail on cmake warnings because we haven't configured jenkins to check windows jobs with that parser. I think it's fine

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.

LGTM when CI comes back 🟢 ❗

@chapulina chapulina merged commit a52ad70 into main Apr 11, 2022
@chapulina chapulina deleted the ci_matching_branch/bump_garden_ign-plugin2 branch April 11, 2022 16:54
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