-
Couldn't load subscription status.
- Fork 4.9k
realsense2-all #12310
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
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-alltarget 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 rsutilsThat gets worse with DDS:
realsense2 realsense-file rsutils fastcdr fastrtps foonathan_memory realddsAnd the list gets longer if we add more libraries.
It gets worse in Linux, where we also have dependencies on
udev libusb-1.0 pthreadandssl crypto rtwith DDS.These are not included in realsense2-all! They need to be specified by the client.
realsense2-allis now dependent onrealsense2BUILD_RS2_ALLflag in CMake, defaulting to ONfoonathan_memorywas misbehaving and not getting picked up as a library0.7-3from -2Tracked on [LRS-931]