Move spdlog::logger to gz-utils

gazebosim / gz-utils

Classes and functions for robot applications

https://gazebosim.org/

Apache License 2.0

6 stars 10 forks source link

Move spdlog::logger to gz-utils #134

Closed mjcarroll closed 4 weeks ago

mjcarroll commented 1 month ago

Draft of moving https://github.com/gazebosim/gz-common/pull/615 here.

azeey commented 1 month ago

CI is not able to find spdlog. Do we need to add it to .githbub/ci/packages.apt?

iche033 commented 4 weeks ago

PRs to add spdlog dependency in:

gz-utils3-release: https://github.com/gazebo-release/gz-utils3-release/pull/4
homebrew-simulation: https://github.com/osrf/homebrew-simulation/pull/2729

iche033 commented 4 weeks ago

add libspdlog-dev dependency inpackage.xml?

scpeters commented 4 weeks ago

I just merged https://github.com/osrf/homebrew-simulation/pull/2729

@osrf-jenkins run tests please

scpeters commented 4 weeks ago

PRs to add spdlog dependency in:

gz-utils3-release: Add libspdlog-dev dependency gazebo-release/gz-utils3-release#4

homebrew-simulation: gz-utils3: Add spdlog dependency osrf/homebrew-simulation#2729

also:

for windows https://github.com/gazebo-tooling/release-tools/pull/1163

caguero commented 4 weeks ago

add libspdlog-dev dependency inpackage.xml?

Do we need to add it even if it's a component? I don't see any dependency related with CLI11.

caguero commented 4 weeks ago

CI is not able to find spdlog. Do we need to add it to .githbub/ci/packages.apt?

Added.

scpeters commented 4 weeks ago

add libspdlog-dev dependency inpackage.xml?

Do we need to add it even if it's a component? I don't see any dependency related with CLI11.

in my opinion we should add optional dependencies to package.xml

If we want to build a dependency from source in a colcon workspace, then the package.xml information should help with proper package build order (assuming the package name matches what we use in package.xml, see https://github.com/dartsim/dart/pull/1389)
I think we could eventually install dependencies using rosdep with the package.xml information instead of package.apt, so I think it's worth having there

azeey commented 4 weeks ago

Discussed this on a video call with Carlos, but the reason we don't have CLI11 in package.xml is that there was no binary package for it on Ubuntu Focal, which is also why we vendored it. I've created #135 to unvendor it and I think as part of that we should also add it to package.xml. We'll also need to add it to rosdep since there's no cli11 key there yet.

All of our platforms have system versions of spdlog available and there's already a rosdep key for it, so I think we should add it to package.xml.