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

Jammy compilation fix with log macro changes #12959

Merged
merged 5 commits into from
May 26, 2024

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented May 24, 2024

Recent PR #12924 introduced issues with compilations in U22 Jammy. This was not caught in GHA (only happens with DDS):

  • add a U22 GHA job

The error itself does not make sense. The offending code is LOG_DEBUG( settings ) where settings is a json object. And the error complains of incomplete type DataWriterQos... What's even more confusing is that replacing with ostringstream() << settings no longer causes the error.
So that's the direction of this PR:

  • redo the ELPP macros by wrapping the message building with ostringstream and only then sending to ELPP
    • this is a possible slight slowdown (if debug messages are on)
  • check if the logger has the proper log level enabled before evaluating the message content (before it always evaluated message content, even if the log was off)
    • this is a performance boost (if debug messages are off), depending on the message content

@maloel maloel requested a review from Nir-Az May 24, 2024 20:02
@maloel
Copy link
Collaborator Author

maloel commented May 25, 2024

Just for the record, this is this compiler error (I did a lot of simplifying to shorten and make it more readable):

In file included from /usr/include/c++/11/bits/move.h:57,
                 from /usr/include/c++/11/bits/stl_pair.h:59,
                 from /usr/include/c++/11/bits/stl_algobase.h:64,
                 from /usr/include/c++/11/bits/char_traits.h:39,
                 from /usr/include/c++/11/string:40,
                 from /jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/fastdds/include/fastdds/dds/domain/qos/DomainParticipantQos.hpp:23,
                 from /jenkins/workspace/LRS_SDK_CI_Debian/src/third-party/realdds/include/realdds/dds-participant.h:7,
                 from /jenkins/workspace/LRS_SDK_CI_Debian/src/third-party/realdds/src/dds-participant.cpp:4:
/usr/include/c++/11/type_traits: In instantiation of 'struct std::is_default_constructible<DataWriterQos>':
/jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/json/include/nlohmann/detail/meta/type_traits.hpp:260:8:   required from 'struct json::detail::is_default_constructible<DataWriterQos>'
/jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/json/include/nlohmann/json.hpp:1603:70:   required by substitution of 'template<class ValueType, typename std::enable_if<(json::detail::is_default_constructible<T1>::value && json::detail::has_from_json<rsutils::json, ValueType, void>::value), int>::type <anonymous> > ValueType rsutils::json::get_impl<ValueType, <anonymous> >(json::detail::priority_tag<0>) const [with ValueType = DataWriterQos; typename std::enable_if<(json::detail::is_default_constructible<T1>::value && json::detail::has_from_json<rsutils::json, ValueType, void>::value), int>::type <anonymous> = <missing>]'
/jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/json/include/nlohmann/json.hpp:1746:81:   required by substitution of 'template<class ValueTypeCV, class ValueType> constexpr decltype (declval<const basic_json_t&>().get_impl<ValueType>(json::detail::priority_tag<4>{})) rsutils::json::get<ValueTypeCV, ValueType>() const [with ValueTypeCV = const DataWriterQos; ValueType = DataWriterQos]'
/jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/json/include/nlohmann/detail/meta/type_traits.hpp:110:73:   required by substitution of 'template<class T, class U> using get_template_function = decltype (declval<T>().get<U>()) [with T = const rsutils::json&; U = const DataWriterQos]'
/jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/json/include/nlohmann/detail/meta/detected.hpp:48:7:   [ skipping 2 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
/jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/json/include/nlohmann/detail/meta/detected.hpp:51:8:   required from 'struct json::detail::is_detected_lazy<json::detail::get_template_function, const rsutils::json&, const DataWriterQos>'
/jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/json/include/nlohmann/detail/meta/type_traits.hpp:250:8:   recursively required from 'struct json::detail::conjunction<json::detail::negation<std::is_same<const DataWriterQos, nullptr_t> >, json::detail::negation<std::is_same<const DataWriterQos, json::detail::json_ref<rsutils::json > > >, json::detail::negation<std::is_same<const DataWriterQos, char> >, json::detail::negation<json::detail::is_basic_json<const DataWriterQos> >, json::detail::negation<std::is_same<const DataWriterQos, std::initializer_list<char> > >, json::detail::negation<std::is_same<const DataWriterQos, string_view > >, json::detail::negation<std::is_same<const DataWriterQos, std::any> >, json::detail::is_detected_lazy<json::detail::get_template_function, const rsutils::json&, const DataWriterQos> >'
/jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/json/include/nlohmann/detail/meta/type_traits.hpp:250:8:   required from 'struct json::detail::conjunction<json::detail::negation<std::is_pointer<const DataWriterQos> >, json::detail::negation<std::is_same<const DataWriterQos, nullptr_t> >, json::detail::negation<std::is_same<const DataWriterQos, json::detail::json_ref<rsutils::json > > >, json::detail::negation<std::is_same<const DataWriterQos, char> >, json::detail::negation<json::detail::is_basic_json<const DataWriterQos> >, json::detail::negation<std::is_same<const DataWriterQos, std::initializer_list<char> > >, json::detail::negation<std::is_same<const DataWriterQos, string_view > >, json::detail::negation<std::is_same<const DataWriterQos, std::any> >, json::detail::is_detected_lazy<json::detail::get_template_function, const rsutils::json&, const DataWriterQos> >'
/jenkins/workspace/LRS_SDK_CI_Debian/src/obj-x86_64-linux-gnu/third-party/json/include/nlohmann/json.hpp:1895:73:   required by substitution of 'template<class ValueType, typename std::enable_if<json::detail::conjunction<json::detail::negation<std::is_pointer<_Tp> >, json::detail::negation<std::is_same<_Up, nullptr_t> >, json::detail::negation<std::is_same<ValueType, json::detail::json_ref<rsutils::json > > >, json::detail::negation<std::is_same<ValueType, char> >, json::detail::negation<json::detail::is_basic_json<BasicJsonType> >, json::detail::negation<std::is_same<ValueType, std::initializer_list<char> > >, json::detail::negation<std::is_same<ValueType, string_view > >, json::detail::negation<std::is_same<ValueType, std::any> >, json::detail::is_detected_lazy<json::detail::get_template_function, const rsutils::json&, ValueType> >::value, int>::type <anonymous> > rsutils::json::operator ValueType<ValueType, <anonymous> >() const [with ValueType = const DataWriterQos; typename std::enable_if<json::detail::conjunction<json::detail::negation<std::is_pointer<_Tp> >, json::detail::negation<std::is_same<_Up, nullptr_t> >, json::detail::negation<std::is_same<ValueType, json::detail::json_ref<rsutils::json > > >, json::detail::negation<std::is_same<ValueType, char> >, json::detail::negation<json::detail::is_basic_json<BasicJsonType> >, json::detail::negation<std::is_same<ValueType, std::initializer_list<char> > >, json::detail::negation<std::is_same<ValueType, string_view > >, json::detail::negation<std::is_same<ValueType, std::any> >, json::detail::is_detected_lazy<json::detail::get_template_function, const rsutils::json&, ValueType> >::value, int>::type <anonymous> = <missing>]'
/jenkins/workspace/LRS_SDK_CI_Debian/src/third-party/easyloggingpp/src/easylogging++.h:3146:3:   required from 'el::base::MessageBuilder& el::base::MessageBuilder::operator<<(const Class&) [with Class = rsutils::json]'
/jenkins/workspace/LRS_SDK_CI_Debian/src/third-party/easyloggingpp/src/easylogging++.h:3216:24:   required from 'el::base::Writer& el::base::Writer::operator<<(const T&) [with T = rsutils::json]'
/jenkins/workspace/LRS_SDK_CI_Debian/src/third-party/realdds/src/dds-participant.cpp:252:9:   required from here
/usr/include/c++/11/type_traits:964:52: error: static assertion failed: template argument must be a complete class or an unbounded array
  964 |       static_assert(std::__is_complete_or_unbounded(__type_identity<_Tp>{}),
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/11/type_traits:964:52: note: 'std::__is_complete_or_unbounded<std::__type_identity<DataWriterQos> >((std::__type_identity<DataWriterQos>{}, std::__type_identity<DataWriterQos>()))' evaluates to false


namespace rsutils {


extern std::string const g_librealsense_elpp_id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

WHat is this? comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

g_ ?

Not self explained..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the string representation of LIBREALSENSE_ELPP_ID. Internal, not for general use, and simply a performance optimization.

{ \
std::ostringstream os__; \
os__ << __VA_ARGS__; \
el::base::Writer( el::Level::LEVEL, __FILE__, __LINE__, ELPP_FUNC, el::base::DispatchAction::NormalLog ) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

IS this the same code as implemented in CLOG?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, except the construct() call is simpler and more efficient.

@maloel maloel merged commit d1da24c into IntelRealSense:development May 26, 2024
14 of 18 checks passed
@maloel maloel deleted the jammy-elpp branch May 26, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants