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

Move spdlog::logger to gz-utils #134

Merged
merged 19 commits into from
Aug 20, 2024
Merged

Move spdlog::logger to gz-utils #134

merged 19 commits into from
Aug 20, 2024

Conversation

mjcarroll
Copy link
Contributor

Draft of moving gazebosim/gz-common#615 here.

Signed-off-by: Michael Carroll <[email protected]>
@mjcarroll mjcarroll requested a review from azeey as a code owner July 31, 2024 19:35
@azeey azeey added the beta Targeting beta release of upcoming collection label Aug 2, 2024
Signed-off-by: Carlos Agüero <[email protected]>
@azeey
Copy link
Contributor

azeey commented Aug 16, 2024

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

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
CMakeLists.txt Outdated Show resolved Hide resolved
examples/log/CMakeLists.txt Outdated Show resolved Hide resolved
log/include/CMakeLists.txt Outdated Show resolved Hide resolved
log/include/gz/utils/log/Logger.hh Outdated Show resolved Hide resolved
log/src/SplitSink_TEST.cc Outdated Show resolved Hide resolved
log/include/gz/utils/log/SplitSink.hh Show resolved Hide resolved
log/include/gz/utils/log/Logger.hh Outdated Show resolved Hide resolved
log/include/gz/utils/log/SplitSink.hh Outdated Show resolved Hide resolved
log/include/gz/utils/log/SplitSink.hh Outdated Show resolved Hide resolved
log/include/gz/utils/log/SplitSink.hh Outdated Show resolved Hide resolved
caguero and others added 5 commits August 16, 2024 22:42
Signed-off-by: Carlos Agüero <[email protected]>
Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@iche033
Copy link
Contributor

iche033 commented Aug 19, 2024

PRs to add spdlog dependency in:

@iche033
Copy link
Contributor

iche033 commented Aug 19, 2024

add libspdlog-dev dependency inpackage.xml?

@scpeters
Copy link
Member

I just merged osrf/homebrew-simulation#2729

@osrf-jenkins run tests please

scpeters added a commit to gazebo-tooling/release-tools that referenced this pull request Aug 19, 2024
@scpeters
Copy link
Member

scpeters added a commit to gazebo-tooling/release-tools that referenced this pull request Aug 20, 2024
@azeey azeey added the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Aug 20, 2024
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@caguero
Copy link
Contributor

caguero commented Aug 20, 2024

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
Copy link
Contributor

caguero commented Aug 20, 2024

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

Added.

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@scpeters
Copy link
Member

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

  1. 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 Support colcon workspace with bullet and ode dartsim/dart#1389)
  2. 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
Copy link
Contributor

azeey commented Aug 20, 2024

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.

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
{
namespace log
{
inline namespace GZ_UTILS_LOG_VERSION_NAMESPACE {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think should just be GZ_UTILS_VERSION_NAMESPACE.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed in f33b379.

Signed-off-by: Carlos Agüero <[email protected]>
@caguero caguero merged commit 095659d into main Aug 20, 2024
9 checks passed
@caguero caguero deleted the mjcarroll/logger_in_utils branch August 20, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection Breaking change Breaks API, ABI or behavior. Must target unstable version.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants