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

realsense2-all #12310

Merged
merged 5 commits into from
Nov 2, 2023
Merged

realsense2-all #12310

merged 5 commits into from
Nov 2, 2023

Conversation

maloel
Copy link
Contributor

@maloel maloel commented Oct 21, 2023

This adds a realsense2-all target which bundles all RealSense libraries into one with BUILD_SHARED_LIBS=OFF.

Today, a client needs to link with many libraries in order to have librealsense work: realsense2 realsense-file rsutils
That gets worse with DDS: realsense2 realsense-file rsutils fastcdr fastrtps foonathan_memory realdds
And the list gets longer if we add more libraries.

It gets worse in Linux, where we also have dependencies on udev libusb-1.0 pthread and ssl crypto rt with DDS.
These are not included in realsense2-all! They need to be specified by the client.

  • added .a/.lib to our output folder, for easy finding (and since they're actually targets)
  • a new target, realsense2-all is now dependent on realsense2
    • you have to specifically target it or compile everything
    • controlled by BUILD_RS2_ALL flag in CMake, defaulting to ON
  • with DDS, foonathan_memory was misbehaving and not getting picked up as a library
    • upgraded it to version 0.7-3 from -2
    • removed compilation of the configurations prior to inclusion in FastDDS (!!) [LRS-801]
    • turned off TOOLS for it (they are creating another target in our project and we've never once used them)
    • it's now a target and is picked up as a library
  • added GHA stage to the static Windows & Linux builds to compile & run an actual client that is not part of our regular makefile, to show that no additional libraries are needed

Tracked on [LRS-931]

@maloel maloel requested a review from Nir-Az October 21, 2023 12:43
@maloel maloel force-pushed the combine branch 3 times, most recently from ca2522f to ac6e2b1 Compare October 29, 2023 08:09
VERBATIM )

else()
message( FATAL_ERROR "Unknown bundle scenario!" )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure we only support clang/GNU/MSVC?
I see user use Ninja and other compilers as well.
Maybe we should drop the build of realsense-all on that case and not fail the cmake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to make it so it'll just give a warning and not add realsense2-all.
I can also change it to if( WIN32 ) ... else() ... endif(), if you think that's better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

WIN32 is better IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I go the WIN32 route, there's no recovery if it doesn't match... I think maybe keep as-is, just warn and not include the realsense2-all target?

@maloel
Copy link
Contributor Author

maloel commented Oct 31, 2023

Had to rebase because of conflict

@maloel maloel requested a review from Nir-Az October 31, 2023 15:09
@maloel
Copy link
Contributor Author

maloel commented Nov 1, 2023

Question: which is better?
realsense2-all
Or
realsense2-bundle
??

@Nir-Az
Copy link
Collaborator

Nir-Az commented Nov 1, 2023

Question: which is better? realsense2-all Or realsense2-bundle ??

Not sure... your call
I think all is fine

@@ -0,0 +1,53 @@
# License: Apache 2.0. See LICENSE file in root directory.
# Copyright(c) 2023 Intel Corporation. All Rights Reserved.
cmake_minimum_required( VERSION 3.15 )
Copy link
Collaborator

Choose a reason for hiding this comment

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

This bump our minimal CMake version.
For windows it's less painfull but I believe we can condition it with the version.
<3.15 dont build it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, it's just the test, forget it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right... wouldn't the static analysis catch this?? Weird.
I'll try it on U18.

Copy link
Contributor Author

@maloel maloel Nov 2, 2023

Choose a reason for hiding this comment

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

Oh!
Duh...
This is the GHA client!
It doesn't run on U18...
realsense2-all makefile works fine in U18. But this client is made for U20+. That's fine.

if( WIN32 )
# Take away the other configurations because they'd require we picked another link
# directory and target... keep it simple...
set( CMAKE_CONFIGURATION_TYPES "Release" )
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we wish to debug??

Copy link
Collaborator

Choose a reason for hiding this comment

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

NVM, test only is fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the unit-test, which only runs in GHA and compiles a specific build.
If you need to debug (unlikely) just remove the line. You'd have to make sure everything else lines up.

Copy link
Collaborator

@Nir-Az Nir-Az left a comment

Choose a reason for hiding this comment

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

LGTM

@maloel maloel merged commit 7979fd4 into IntelRealSense:development Nov 2, 2023
@maloel maloel deleted the combine branch November 2, 2023 10:57
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