-
Notifications
You must be signed in to change notification settings - Fork 328
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
Integrate spdlog #2551
base: main
Are you sure you want to change the base?
Integrate spdlog #2551
Conversation
…re installed correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @aymanhab and @chrisdembia)
CMakeLists.txt, line 593 at r1 (raw file):
# SPDLOG_HOME to the installation directory of spdlog.") #endif() include_directories("${OPENSIM_DEPENDENCIES_DIR}/spdlog/include")
We should use find_package()
and target_link_libraries(osimCommon spdlog)
. I could look at this with you if there are issues.
CMakeLists.txt, line 765 at r1 (raw file):
#--- set(SPDLOG_HOME "${OPENSIM_DEPENDENCIES_DIR}/spdlog") if(WIN32)
Shouldn't spdlog be installed for all platforms?
Also, we should obey the value of OPENSIM_COPY_DEPENDENCIES
.
Do we link to a shared library or static library? I think in this case, static is preferred. If not, we have to handle RPATH for Mac, etc.
OpenSim/Common/Object.cpp, line 520 at r1 (raw file):
if (_debugLevel>=2) { cout << "Object.registerType: " << type << " .\n"; spdlog::debug("Object.registerType: {}!", type);
I think we should provide a way for users to set the debug level in Matlab and Python, and perhaps the command-line tools (in the future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this truly useful, we have to convert all our logging to spdlog 😱
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @aymanhab)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @chrisdembia)
CMakeLists.txt, line 593 at r1 (raw file):
Previously, chrisdembia (Christopher Dembia) wrote…
We should use
find_package()
andtarget_link_libraries(osimCommon spdlog)
. I could look at this with you if there are issues.
Apparaently every package that claims to use CMake ends up with a different layout/arrangement. There's no FindSPdlog.cmake that gets generated when you build/install. Also there's no library these are just headers AFAICT. I leveraged the dependencies directory because that seems like the approach used by ci and that has the potential to scale without much custom Cmake magic, but I'm open to ideas to make Simbody, BTK, SPDLog be treated relatively consistently.
CMakeLists.txt, line 765 at r1 (raw file):
Previously, chrisdembia (Christopher Dembia) wrote…
Shouldn't spdlog be installed for all platforms?
Also, we should obey the value of
OPENSIM_COPY_DEPENDENCIES
.Do we link to a shared library or static library? I think in this case, static is preferred. If not, we have to handle RPATH for Mac, etc.
Agreed, just didn't know where it should go on other platforms.
OpenSim/Common/Object.cpp, line 520 at r1 (raw file):
Previously, chrisdembia (Christopher Dembia) wrote…
I think we should provide a way for users to set the debug level in Matlab and Python, and perhaps the command-line tools (in the future).
We could still keep Object::debugLevel or change it to loggingLevel and internally pass to spdlog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this PR just makes sure we "can" use it in one place, next one would be to tag our current mishmash of cout statements with what counts as "Debug", "Trace", "Error", "Fatal", then convert to a Macro that uses spdlog underneath.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @chrisdembia)
CMakeLists.txt, line 765 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Agreed, just didn't know where it should go on other platforms.
Building time increased by a few minutes we should evaluate whether build libraries or stick with headers.
OpenSim/Common/Object.cpp, line 520 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
We could still keep Object::debugLevel or change it to loggingLevel and internally pass to spdlog.
We have a LogManager object that can serve as intermediary.
@noahggordon are you interested in converting lots of logging statements across the code base to use a new logging library? |
…org/opensim-core into integrate_spdlog
This reverts commit 0332552
@aymanhab I've updated this PR. I've set up the infrastructure so that we could replace the old mechanism the GUI used for populating the Messages window. Could you review? Before merging, we would need to move all uses of |
Thanks much for working on this @chrisdembia Will likely wait on merging until after 4.1 since it's still a work in progress and it adds a new library to the build/install mechanism, but I'll review anyway. |
That’s fine with me. I would like the code to be reviewed soon but it’s fine if it’s not merged for a few months. |
Based on conversation with Ayman , I will create a feature branch, split into multiple PRs, fix the MATLAB logging issue, and add a command line option. |
Skimming through the changes, looks very good. The issue I have is the mix of changes to build system, new abstractions and changes to our old code to use the new abstractions, if we create a "feature" branch for logging, first PR with only CMake changes to it, that would be great, 2nd PR into the branch would introduce new abstractions, bindings, third PR would use variety of these new abstractions/options that would help move this along fast. Very excited about this. Thanks @chrisdembia 🥇 |
Bring spdlog into OpenSim.
…:opensim-org/opensim-core into integrate_spdlog � Conflicts: � cmake/OpenSimConfig.cmake.in � dependencies/CMakeLists.txt
# Conflicts: # Applications/opensim-cmd/opensim-cmd.cpp # Applications/opensim-cmd/opensim-cmd_info.h # Applications/opensim-cmd/opensim-cmd_print-xml.h # Applications/opensim-cmd/opensim-cmd_run-tool.h # Applications/opensim-cmd/opensim-cmd_update-file.h # Bindings/Java/JavaLogSink.java # Bindings/Java/tests/TestBasics.java # Bindings/OpenSimHeaders_common.h # Bindings/common.i # OpenSim/Common/LogSink.h # OpenSim/Common/Object.cpp # OpenSim/Common/Object.h # OpenSim/Common/osimCommon.h # OpenSim/Simulation/Model/Model.cpp
Making a note here (from #2685) that logging was lost from IK. Does not necessarily need to be addressed in this PR. |
Fixes issue #0
Brief summary of changes
Integrate spdlog into the build/installation process.
Testing I've completed
Local and ci builds are running, added one use case in Object.cpp in prep for a full roll-out.
Looking for feedback on...
Build mechanism uses dependencies and SUPERBUILD exclusively rather than complicated CMake structures/commands. Would be good to discuss this vs how Simbody, BTK are integrated and how to scale.
CHANGELOG.md (choose one)
The Doxygen for this PR can be viewed at http://myosin.sourceforge.net/?C=N;O=D; click the folder whose name is this PR's number.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)