Skip to content

Move spdlog::logger to gz-utils#134

Merged
caguero merged 19 commits intomainfrom
mjcarroll/logger_in_utils
Aug 20, 2024
Merged

Move spdlog::logger to gz-utils#134
caguero merged 19 commits intomainfrom
mjcarroll/logger_in_utils

Conversation

@mjcarroll
Copy link
Copy Markdown

@mjcarroll mjcarroll commented Jul 31, 2024

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

Move spdlog::logger to gz-utils
965eff9 
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll mjcarroll requested a review from azeey as a code owner July 31, 2024 19:35
caguero and others added 2 commits August 2, 2024 16:31
@caguero
Code compiling and example.
ac15ba1 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
Merge branch 'main' into mjcarroll/logger_in_utils
6f20cd2
@azeey azeey added the beta Targeting beta release of upcoming collection label Aug 2, 2024
@caguero
Refactor
4eefd71 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@azeey
Copy link
Copy Markdown
Contributor

azeey commented Aug 16, 2024

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

caguero added 3 commits August 16, 2024 21:57
@caguero
Adding spdlog dependency to CI
6144605 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
Tweak
55eb7a5 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
Tweak
a927dad 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
azeey
azeey reviewed Aug 16, 2024
View reviewed changes
caguero and others added 5 commits August 16, 2024 22:42
@caguero
Tweak
4a893da 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
Update examples/log/CMakeLists.txt
4cf0bcb 
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Carlos Agüero <cen.aguero@gmail.com>
@caguero
Namespaces
be88966 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
Tweaks
0ac9ddd 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
Constructor
49cd5af 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
This was referenced Aug 19, 2024
Merged
Merged
@iche033
Copy link
Copy Markdown
Contributor

iche033 commented Aug 19, 2024

PRs to add spdlog dependency in:

@iche033
Copy link
Copy Markdown
Contributor

iche033 commented Aug 19, 2024

add libspdlog-dev dependency inpackage.xml?

@scpeters
Copy link
Copy Markdown
Member

scpeters commented Aug 19, 2024

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
Add spdlog to VCPKG_DEPENDENCIES_LEGACY
d4a8921 
Needed by gazebosim/gz-utils#134.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters mentioned this pull request Aug 19, 2024
Merged
@scpeters
Copy link
Copy Markdown
Member

scpeters commented Aug 19, 2024

PRs to add spdlog dependency in:

also:

scpeters added a commit to gazebo-tooling/release-tools that referenced this pull request Aug 20, 2024
@scpeters
Add spdlog to VCPKG_DEPENDENCIES_LEGACY (#1163)
d80e20f 
Needed by gazebosim/gz-utils#134.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@azeey azeey added the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Aug 20, 2024
caguero added 2 commits August 20, 2024 19:37
@caguero
SplitSink tweak
d96ec02 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
Tweak
d1402ec 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
Copy link
Copy Markdown
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
Copy Markdown
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.

caguero added 2 commits August 20, 2024 19:58
@caguero
Adding file
85c149e 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
Tweak
b1ea607 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@scpeters
Copy link
Copy Markdown
Member

scpeters 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.

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
Copy Markdown
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.

caguero added 2 commits August 20, 2024 21:21
@caguero
Win
66451d1 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero
package.xml
7638bbb 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
azeey
azeey reviewed Aug 20, 2024
View reviewed changes
log/include/gz/utils/log/SplitSink.hh Outdated
{
namespace log
{
inline namespace GZ_UTILS_LOG_VERSION_NAMESPACE {
Copy link
Copy Markdown
Contributor

@azeey azeey Aug 20, 2024

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

@caguero caguero Aug 20, 2024

Choose a reason for hiding this comment

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

Changed in f33b379.

@caguero
Tweak
f33b379 
Signed-off-by: Carlos Agüero <caguero@openrobotics.org>
@caguero caguero merged commit 095659d into main Aug 20, 2024
@caguero caguero deleted the mjcarroll/logger_in_utils branch August 20, 2024 21:36
@azeey azeey mentioned this pull request Jan 6, 2025
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@caguero caguero caguero left review comments

@azeey azeey azeey approved these changes

Assignees

@caguero caguero

Labels

beta Targeting beta release of upcoming collection Breaking change Breaks API, ABI or behavior. Must target unstable version.

Projects

Core development
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mjcarroll @azeey @iche033 @scpeters @caguero