Skip to content

Bring spdlog into OpenSim. #2591

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

Merged
merged 2 commits into from
Oct 15, 2019
Merged

Bring spdlog into OpenSim. #2591

merged 2 commits into from
Oct 15, 2019

Conversation

chrisdembia
Copy link
Member

@chrisdembia chrisdembia commented Oct 11, 2019

Brief summary of changes

This PR is an offshoot of #2551. I built on @aymanhab's original work to build and install spdlog.

The destination for this PR is feature_logging, not master.

Testing I've completed

I can build and run an OpenSim C++ example from the OpenSim installation on Mac. I did not test on Windows.

CHANGELOG.md (choose one)

  • no need to update because...this feature is not complete yet

This change is Reviewable

@chrisdembia chrisdembia requested a review from aymanhab October 11, 2019 03:13
Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @chrisdembia)


cmake/OpenSimConfig.cmake.in, line 80 at r1 (raw file):

    # We pre-define these variables because finding Simbody redefines variables
    # that we need for finding spdlog.
    set(_SIMBODY_PATH "@PACKAGE_OPENSIM_INSTALL_SIMBODYDIR@")

Can you clarify this comment?


OpenSim/Common/CMakeLists.txt, line 18 at r1 (raw file):

    # Clients of osimCommon need not link to BTK.
    LINKLIBS PUBLIC ${Simbody_LIBRARIES} spdlog::spdlog PRIVATE ${BTK_LIBRARIES}
    INCLUDES ${INCLUDES}

Can we move the explicit library name to a CMake variable for future maintenance?

Copy link
Member Author

@chrisdembia chrisdembia left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aymanhab)


cmake/OpenSimConfig.cmake.in, line 80 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Can you clarify this comment?

Done.


OpenSim/Common/CMakeLists.txt, line 18 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Can we move the explicit library name to a CMake variable for future maintenance?

The Simbody_LIBRARIES and BTK_LIBRARIES variables are defined by the projects' SimbodyConfig.cmake, etc. files, not by OpenSim's CMake files. spdlog does not define a similar variable, so I would prefer to not invent one ourselves.

@chrisdembia
Copy link
Member Author

Ready for review.

Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chrisdembia)


cmake/OpenSimConfig.cmake.in, line 80 at r1 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

Done.

I don't think anything changed here, unless I'm looking at a stale file.The confusing part for me was the statement that "finding Simbody redefines variables that we need for finding spdlog"

Copy link
Member Author

@chrisdembia chrisdembia left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @aymanhab)


cmake/OpenSimConfig.cmake.in, line 80 at r1 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

I don't think anything changed here, unless I'm looking at a stale file.The confusing part for me was the statement that "finding Simbody redefines variables that we need for finding spdlog"

Oops; I hadn't pushed. Can you check again?

Copy link
Member

@aymanhab aymanhab left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chrisdembia)


cmake/OpenSimConfig.cmake.in, line 80 at r1 (raw file):

Previously, chrisdembia (Christopher Dembia) wrote…

Oops; I hadn't pushed. Can you check again?

👍

@chrisdembia
Copy link
Member Author

@aymanhab do you know why testStaticOptimization is failing on Mac?

@aymanhab
Copy link
Member

aymanhab commented Oct 11, 2019 via email

@chrisdembia
Copy link
Member Author

Thanks for the review @aymanhab

@chrisdembia chrisdembia merged commit 3242769 into feature_logging Oct 15, 2019
@chrisdembia chrisdembia deleted the spdlog_cmake branch October 15, 2019 01:19
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.

2 participants