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

Simplyfy c++ version check #11997

Merged
merged 4 commits into from May 13, 2024
Merged

Conversation

autoantwort
Copy link

This has the same effect then the code before.

@Nir-Az Nir-Az requested a review from maloel July 16, 2023 08:38
@maloel maloel changed the base branch from master to development July 16, 2023 09:02
@maloel
Copy link
Collaborator

maloel commented Jul 16, 2023

Hi @autoantwort

Thanks!

May I ask what prompted you to make this change, though? Did it not work?

The CMAKE_CXX_STANDARD no longer being there and therefore perhaps going awry in the other projects.
Also, what happens when another project specifies cxx_std_17, for example? Will the latest (17) be used, or both? Before, we left the client to its own devices but here we're setting 11...

@autoantwort
Copy link
Author

autoantwort commented Jul 16, 2023

The motivation was that I currently get:

CMake Error at CMake/lrs_macros.cmake:17 (message):
  Project 'realsense2' requires C++14 or higher
Call Stack (most recent call first):
  CMakeLists.txt:53 (config_cxx_flags)

but I use the most recent apple clang version ...
The underlying problem is probably

Performing C++ SOURCE FILE Test SUPPORTS_CXX14 failed with the following output:
Change Dir: /Users/leanderSchulten/git_projekte/vcpkg/buildtrees/realsense2/arm64-osx-rel/CMakeFiles/CMakeScratch/TryCompile-gF8fd4

Run Build Command(s):/Users/leanderSchulten/git_projekte/vcpkg/downloads/tools/ninja/1.10.2-osx/ninja cmTC_6bf46 && [1/2] Building CXX object CMakeFiles/cmTC_6bf46.dir/src.cxx.o
clang: warning: -latomic: 'linker' input unused [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-mfpu=neon' [-Wunused-command-line-argument]
clang: warning: argument unused during compilation: '-mfloat-abi=hard' [-Wunused-command-line-argument]
[2/2] Linking CXX executable cmTC_6bf46
FAILED: cmTC_6bf46 
: && /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -fPIC -pedantic -g -Wno-missing-field-initializers -Wno-switch -Wno-multichar -Wsequence-point -Wformat -Wformat-security -mfpu=neon -mfloat-abi=hard -ftree-vectorize -latomic -pthread  -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk -Wl,-search_paths_first -Wl,-headerpad_max_install_names  CMakeFiles/cmTC_6bf46.dir/src.cxx.o -o cmTC_6bf46   && :
ld: library not found for -latomic
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

But why do a try_compile if cmake can do it natively.

@autoantwort
Copy link
Author

Also, what happens when another project specifies cxx_std_17, for example? Will the latest (17) be used, or both?

target_compile_features specifies the minimum required c++ version needed to compile the project.

target_compile_features(main PRIVATE cxx_std_14)
target_compile_features(main PRIVATE cxx_std_11) # it is still 14
target_compile_features(main PRIVATE cxx_std_17) # now it is 17
set(CMAKE_CXX_STANDARD 17)
add_library(main main.cpp)
target_compile_features(main PRIVATE cxx_std_14) # now compiled with C++17 because of CMAKE_CXX_STANDARD
set(CMAKE_CXX_STANDARD 11)
add_library(main main.cpp)
target_compile_features(main PRIVATE cxx_std_14) # compiled with C++14

Before, we left the client to its own devices but here we're setting 11...

But the clients must at least choose C++11 right? Now C++11 is used when nothing is specified and if they specify something this is used (C++11 is used if they specify something smaller than c++11)

@maloel
Copy link
Collaborator

maloel commented Jul 17, 2023

I use the most recent apple clang version ...

I think, as of clang 16, c++17 is the minimum so specifying c++14 probably generates your error. This is a problem, I agree.
You don't have a problem with -std=c++11, e.g. when compiling rsutils?

It's funny, because of all the builds in our CI it was the Mac build that failed...

All the rest of your comments make sense, but we need to be careful here. Let's see what the CI says...

@autoantwort
Copy link
Author

You don't have a problem with -std=c++11, e.g. when compiling rsutils?

No. Ci also seems to be happy.

Now only LRS_libci_trigger is failing, but I don't know why.

@Nir-Az Nir-Az closed this Sep 20, 2023
@Nir-Az Nir-Az reopened this Sep 20, 2023
@Nir-Az
Copy link
Collaborator

Nir-Az commented May 12, 2024

@maloel any inputs regarding this PR?

Copy link
Collaborator

@maloel maloel left a comment

Choose a reason for hiding this comment

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

No problems that I can see...
But it needs a rebase to solve the conflict -- I've done this and will try to push.

@maloel
Copy link
Collaborator

maloel commented May 12, 2024

Looks like newer code in rsutils requires C++14.
Testing offline...

Copy link
Collaborator

@maloel maloel left a comment

Choose a reason for hiding this comment

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

Thanks @autoantwort

@maloel maloel merged commit a5bec4d into IntelRealSense:development May 13, 2024
16 of 17 checks passed
@autoantwort autoantwort deleted the less-code branch May 13, 2024 07:32
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.

None yet

3 participants