-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
realsense2-all #12310
Conversation
ca2522f
to
ac6e2b1
Compare
src/realsense2-all.cmake
Outdated
VERBATIM ) | ||
|
||
else() | ||
message( FATAL_ERROR "Unknown bundle scenario!" ) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIN32 is better IMO
There was a problem hiding this comment.
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?
Had to rebase because of conflict |
Question: which is better? |
Not sure... your call |
@@ -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 ) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" ) |
There was a problem hiding this comment.
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??
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
andssl crypto rt
with DDS.These are not included in realsense2-all! They need to be specified by the client.
realsense2-all
is now dependent onrealsense2
BUILD_RS2_ALL
flag in CMake, defaulting to ONfoonathan_memory
was misbehaving and not getting picked up as a library0.7-3
from -2Tracked on [LRS-931]