ELPP and utilities changes #11160

maloel · 2022-11-29T13:08:36Z

This is an attempt to remove the shared-init.cmake requirement from downstream projects.
This is accomplished by:

Replacing includes of shared-init.cmake with a call to a function:

using_easylogging( ${PROJECT_NAME} [SHARED|STATIC] )
The optional SHARED or STATIC can be used to indicate that the ELPP initialization needs attention only when building in shared or static mode (this is needed because in static, clients use librealsense's)
This will add elpp-init.cpp to the build of the project named

Other things in here:

utilities include files are now arranged according to the tree (requires CMake 3.8)
This means we no longer support U16 - see PR removing Xenial from our release process
The "librealsense" constant used to define our ELPP logger is now an actual define, LIBREALSENSE_ELPP_ID
Added a configure_elpp_logger utility function to easily configure ELPP (will be used in realdds, too)
Fixed some warnings

maloel · 2022-11-30T11:32:56Z

I don't like what I did :)
So I take it back.
In particular: I don't like forcing something (elpp-init.cpp) on the user and making them deal with it (NO_ELPP_INIT). We cannot assume the user of librealsense will even need ELPP, and making them do initialization may even get in their way.

Instead, I added a function that can be called (instead of including shared-init.cmake), which is equivalent to the previous state of things, just a little more understandable and does not require any special knowledge of our file structure.

Here's the original explanation before this update:

Always add a file, elpp-init.cpp, to any project that is dependent on utilities

Provide a mechanism (NO_ELPP_INIT) to the file from doing anything (i.e., it will compile but do nothing)

realsense2 dependents are automatically utilities dependents

Because realsense2 has its own INITIALIZE_EASYLOGGINGPP, it defines NO_ELPP_INIT

Any dependent of realsense2 will automatically use its ELPP in static mode, therefore any dependent gets NO_ELPP_INIT automatically

Any dependent that defines their own INITIALIZE_EASYLOGGINGPP will have to also specify NO_ELPP_INIT manually (e.g., rs-server) or it will get linkage errors

The result is that certain projects still need special attention.

Nir-Az · 2022-12-04T12:08:55Z

CMakeLists.txt

@@ -65,7 +65,7 @@ include(src/CMakeLists.txt)
 include(third-party/CMakeLists.txt)
 include(common/utilities/CMakeLists.txt)

-target_link_libraries( ${LRS_TARGET} PRIVATE utilities )
+target_link_libraries( ${LRS_TARGET} PUBLIC utilities )


Why does the user app must have utilities?
It is statically compiled into the SDK no?
I am not sure it's a good idea, where does it effect the user?

I can remove this but then you have to add utilities as a dependency in all the other projects -- possibly including users. If they don't use utilities, it won't make a difference.

But if we make it a statically dependence of the librealsense2 library all tools/examples/users will get it.
Only tools using the Utilities explicitly will need it and that's OK.

But how does it hurt? Projects that depend on us will either use our CMake in which case it will be transparent, or they compile it separately which will be transparent.

Let's do this and then I'll take some time to try different ways (I wanted to, anyway) to use libreasense and see what's better...

I don't understand why you insist for this change..
reverting this change should be easy.

I don't know what can happen for debians / Windows installer and it's not necessary.
We want to minimize the user dependencies.
Integrating a DDS third party took me a month because of this kind of things.

When we install rs-data-collect,
It will look for utilities library
Why do we want this?

That's not installing it. That's the dependency that CMake adds: not to the executable, but to the build. This is fine. This is a convenience. It lets you include utilities header without having to worry about it.
It doesn't add any dependency to the final executable.
Again: this has effect only in CMake projects. Not on the final output.

Because it is static? if it wasn't static it was needed doing runtime right?

Right, if it wasn't static it'd be needed at runtime and not just compile-time.

But:

static libraries do not link other static libraries into the code

Visual Studio has an option:

This is not currently on, nor is there any easy way to turn it on via CMake and cross-platform (from limited reading about it).

That means our current code (since introducing utilities/realdds) will have issues if librealsense2 (as a library) is used independently and not via CMake: the user would have to find the utilities lib (or realdds) and link with it, too.

With CMake, making the dependency PUBLIC solves this issue: it'll be automatic for the user.

Without CMake, they'll need to manually add the dependency even if they do not use utilities directly: we will likely have to distribute utilities and realdds libraries alongside our realsense2.lib for this use-case. Or we can try to find a solution for Linking library dependencies into realsense2.lib.

I want to add code to our CI that simulates an example that's not part of our main CMake. It would have caught a change like this.

Let's talk about it tomorrow.

This sounds like something we need to fix ASAP.
Like you said, let's speak tomorrow..

Nir-Az · 2022-12-04T12:10:11Z

src/hw-monitor.h

@@ -208,7 +208,7 @@ namespace librealsense
            {
                if (ptr) _heap.deallocate(ptr);
            });
-            if (!token.get()) throw;
+            if (!token.get()) throw std::runtime_error( "allocation failed" );


I did the same change on my PR :)

third-party/utilities/CMakeLists.txt

unit-tests/log/test-vs-LOG-static.cpp

Nir-Az

LGTM

…endents

maloel requested a review from Nir-Az November 29, 2022 13:08

maloel mentioned this pull request Dec 4, 2022

merge development (utilities) to dds #11145

Merged

Nir-Az reviewed Dec 4, 2022

View reviewed changes

third-party/utilities/CMakeLists.txt Show resolved Hide resolved

Nir-Az reviewed Dec 4, 2022

View reviewed changes

unit-tests/log/test-vs-LOG-static.cpp Show resolved Hide resolved

Nir-Az approved these changes Dec 6, 2022

View reviewed changes

maloel added 6 commits December 6, 2022 09:19

added LIBREALSENSE_ELPP_ID, configure_elpp_logger

1a3cc91

utilities files now in tree; shared-init -> elpp-init auto use in dep…

13fc819

…endents

librs uses NO_ELPP_INIT; dependents, too, in static

082cb07

update all dependent projects

97294a6

fixed some warnings

2bd2543

revert NO_ELPP_INIT -> using_easylogging() instead

103b7b4

maloel force-pushed the elpp branch from 7e0fd3d to 103b7b4 Compare December 6, 2022 07:22

maloel merged commit a98d805 into IntelRealSense:development Dec 6, 2022

maloel deleted the elpp branch December 6, 2022 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ELPP and utilities changes #11160

ELPP and utilities changes #11160

maloel commented Nov 29, 2022 •

edited

Loading

maloel commented Nov 30, 2022

Nir-Az Dec 4, 2022

maloel Dec 4, 2022

Nir-Az Dec 4, 2022

maloel Dec 4, 2022

Nir-Az Dec 4, 2022

Nir-Az Dec 4, 2022

maloel Dec 4, 2022

Nir-Az Dec 4, 2022 •

edited

Loading

maloel Dec 4, 2022

Nir-Az Dec 4, 2022

Nir-Az Dec 4, 2022

Nir-Az left a comment

ELPP and utilities changes #11160

ELPP and utilities changes #11160

Conversation

maloel commented Nov 29, 2022 • edited Loading

maloel commented Nov 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nir-Az Dec 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nir-Az left a comment

Choose a reason for hiding this comment

maloel commented Nov 29, 2022 •

edited

Loading

Nir-Az Dec 4, 2022 •

edited

Loading