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

Boost Logger Implementation #3571

Merged
merged 21 commits into from
Apr 30, 2021
Merged

Conversation

ShobhitAd
Copy link
Contributor

@ShobhitAd ShobhitAd commented Nov 12, 2020

This PR is ready for review.

Risk

This PR makes no API changes.

Summary

Implements a third party logger for sdl core

CLA

Copy link
Collaborator

@iCollin iCollin left a comment

Choose a reason for hiding this comment

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

Is it possible to make building Core with the boost logger not require sudo?

When building with Boost, I see this warning in the cmake output, could we suppress this?

+CMake Warning (dev) at src/components/utils/CMakeLists.txt:137 (add_dependencies):
+  Policy CMP0046 is not set: Error on non-existent dependency in
+  add_dependencies.  Run "cmake --help-policy CMP0046" for policy details.
+  Use the cmake_policy command to set the policy and suppress this warning.
+
+  The dependency target "install-3rd_party_logger" of target "Utils" does not
+  exist.
+This warning is for project developers.  Use -Wno-dev to suppress it.

CMakeLists.txt Outdated Show resolved Hide resolved
src/3rd_party/CMakeLists.txt Outdated Show resolved Hide resolved
src/appMain/CMakeLists.txt Outdated Show resolved Hide resolved
src/appMain/boostlogconfig.ini Show resolved Hide resolved
src/components/test_main.cc Show resolved Hide resolved
src/components/utils/test/CMakeLists.txt Outdated Show resolved Hide resolved
src/components/utils/src/logger/boostlogger.cc Outdated Show resolved Hide resolved
src/components/utils/src/logger/boostlogger.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@iCollin iCollin left a comment

Choose a reason for hiding this comment

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

Is sudo necessary for the boost install?

src/components/utils/src/logger/boostlogger.cc Outdated Show resolved Hide resolved
@ShobhitAd
Copy link
Contributor Author

ShobhitAd commented Dec 10, 2020

Is sudo necessary for the boost install?

I don't think sudo is necessary to compile and install the boost components. However, since the 3RD_PARTY_INSTALL_PREFIX is set to /usr/local(https://github.com/smartdevicelink/sdl_core/blob/master/src/3rd_party/set_3rd_party_paths.cmake#L50) we do require special permissions to write to that directory.

The install target could be changed but it would end up affecting more than just the boost libraries(libbson and log4cxx). That said, it looks like sudo is only required for the first time setup(when the library is not found). All subsequent builds will not require sudo.

@ShobhitAd ShobhitAd changed the title [WIP]Feature/Boost Logger Implementation Feature/Boost Logger Implementation Dec 11, 2020
if (!utils::appenders_loader.Loaded()) {
SDL_LOG_ERROR("Appenders plugin not loaded, file logging disabled");
}
#endif // LOG4CXX_LOGGER
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems this section causes the build to fail if ENABLE_LOG=OFF, maybe move it into the ENABLE_LOG ifdef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 770ef06

@ShobhitAd ShobhitAd merged commit 4dc2bd9 into develop Apr 30, 2021
@ShobhitAd ShobhitAd deleted the feature/boost_logger_implementation branch April 30, 2021 17:52
@Jack-Byrne Jack-Byrne mentioned this pull request Sep 29, 2021
@jacobkeeler jacobkeeler changed the title Feature/Boost Logger Implementation Boost Logger Implementation Oct 25, 2021
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.

3 participants