Skip to content

Clean cmake opengl warning ubuntu22 #11442

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

Merged

Conversation

Tamir91
Copy link
Contributor

@Tamir91 Tamir91 commented Feb 15, 2023

Tracking on [LRS-673]

CMakeLists.txt Outdated
@@ -1,6 +1,12 @@
# minimum required cmake version: 3.1.0
cmake_minimum_required(VERSION 3.1.0)

# This policy provides compatibility with projects that expect the legacy GL library to be used.
if (POLICY CMP0072 AND UNIX AND NOT APPLE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why NOT APPLE?

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 understood from the ticket name that I need to fix the issue on U22.
Have we Apple machines to purpose seen new behavior?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you are right,
You can verify on GHA, we have a mac machine
https://github.com/IntelRealSense/librealsense/actions/runs/4183814020/jobs/7248655076
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ups, I saw it before)
May I run viewer on Mac and see live that all work correctly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you don't have a mac, you can only run MAC with a VM if you want

CMakeLists.txt Outdated
# This policy provides compatibility with projects that expect the legacy GL library to be used.
if (POLICY CMP0072 AND UNIX AND NOT APPLE)
set(CMAKE_POLICY_DEFAULT_CMP0072 NEW)
message(STATUS "Policy CMP0072 setted: NEW")
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to see this message..

Copy link
Contributor Author

@Tamir91 Tamir91 Feb 21, 2023

Choose a reason for hiding this comment

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

The comment was removed.

CMakeLists.txt Outdated
@@ -1,6 +1,12 @@
# minimum required cmake version: 3.1.0
cmake_minimum_required(VERSION 3.1.0)

# This policy provides compatibility with projects that expect the legacy GL library to be used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update this generic comment,
if you think NEW should be used..

@Nir-Az
Copy link
Collaborator

Nir-Az commented Feb 20, 2023

I just saw you place it on the main CMakelist, why is that?
You should place stuff like that as narrow and close to the relevant line as possible.
We don't builf GLFW project on all cases, so we don't need it for all cases.

The best place to place it is here:
https://github.com/IntelRealSense/librealsense/blob/master/third-party/glfw/CMakeLists.txt
This file is configures only if we include GLFW

@Tamir91
Copy link
Contributor Author

Tamir91 commented Feb 20, 2023

I just saw you place it on the main CMakelist, why is that? You should place stuff like that as narrow and close to the relevant line as possible. We don't builf GLFW project on all cases, so we don't need it for all cases.

The best place to place it is here: https://github.com/IntelRealSense/librealsense/blob/master/third-party/glfw/CMakeLists.txt This file is configures only if we include GLFW

You are right.
In my first commit and now I tried to add code to glfw/CMakeList.txt and it not solving the warning.
I have now checked if adding the code to third-party/CMakeList.txt solves the warning...
Please advise.

@Nir-Az
Copy link
Collaborator

Nir-Az commented Feb 20, 2023

I just saw you place it on the main CMakelist, why is that? You should place stuff like that as narrow and close to the relevant line as possible. We don't builf GLFW project on all cases, so we don't need it for all cases.
The best place to place it is here: https://github.com/IntelRealSense/librealsense/blob/master/third-party/glfw/CMakeLists.txt This file is configures only if we include GLFW

You are right. In my first commit and now I tried to add code to glfw/CMakeList.txt and it not solving the warning. I have now checked if adding the code to third-party/CMakeList.txt solves the warning... Please advise.

OK, thats interesting,
It means someone else is calling
find_package(OpenGL REQUIRED)

From the log we can see:

Call Stack (most recent call first):
  CMake/opengl_config.cmake:1 (find_package)
  tools/CMakeLists.txt:25 (include)

Please try adding this (instead of other lines)
to the top of CMake/opengl_config.cmake file

# Comment why we are doing this
if (POLICY CMP0072)
    cmake_policy(SET CMP0072 NEW)
endif()

BTW, why did you use another method than cmake_policy(SET..?

@Tamir91
Copy link
Contributor Author

Tamir91 commented Feb 21, 2023

@Tamir91 Tamir91 closed this Feb 21, 2023
@Tamir91 Tamir91 reopened this Feb 21, 2023
@Tamir91
Copy link
Contributor Author

Tamir91 commented Feb 21, 2023

I just saw you place it on the main CMakelist, why is that? You should place stuff like that as narrow and close to the relevant line as possible. We don't builf GLFW project on all cases, so we don't need it for all cases.
The best place to place it is here: https://github.com/IntelRealSense/librealsense/blob/master/third-party/glfw/CMakeLists.txt This file is configures only if we include GLFW

You are right. In my first commit and now I tried to add code to glfw/CMakeList.txt and it not solving the warning. I have now checked if adding the code to third-party/CMakeList.txt solves the warning... Please advise.

OK, thats interesting, It means someone else is calling find_package(OpenGL REQUIRED)

From the log we can see:

Call Stack (most recent call first):
  CMake/opengl_config.cmake:1 (find_package)
  tools/CMakeLists.txt:25 (include)

Please try adding this (instead of other lines) to the top of CMake/opengl_config.cmake file

# Comment why we are doing this
if (POLICY CMP0072)
    cmake_policy(SET CMP0072 NEW)
endif()

BTW, why did you use another method than cmake_policy(SET..?

  • I used cmake_policy(SET. function because only it affects the policy warning (I found it Stackoverflow).
  • I put the policy in CMake/opengl_config.cmake and found it works too.
    Are you think set policy there?

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

@Nir-Az Nir-Az merged commit 43cc7c2 into IntelRealSense:development Feb 21, 2023
@Tamir91 Tamir91 deleted the clean_cmake_opengl_warning_ubuntu22 branch February 26, 2023 09:29
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