Skip to content

Handle absolute OPENCV_INCLUDE_INSTALL_PATH correctly#14925

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
StefanBruens:handle_absolute_include_path
Jul 2, 2019
Merged

Handle absolute OPENCV_INCLUDE_INSTALL_PATH correctly#14925
opencv-pushbot merged 1 commit intoopencv:3.4from
StefanBruens:handle_absolute_include_path

Conversation

@StefanBruens
Copy link
Copy Markdown
Contributor

In case OPENCV_INCLUDE_INSTALL_PATH is absolute (i.e. starts with a "/"),
the path ends up with a double "/".

While this is mostly equivalent to a single slash, it may have a nasty
side effect when:

  • OpenCV_INSTALL_PATH is empty
  • OPENCV_INCLUDE_INSTALL_PATH is "/usr/include"
  • the calling build script uses "-isystem" to specify the path to the
    headers of dependencies (to avoid warnings)

Specifying "-isystem /usr/include" breaks the path ordering, and GCC can
no longer find its "stdlib.h", thus CMake filters such statements.
Unfortunately it fails to do so when using "//usr/include".

In case OPENCV_INCLUDE_INSTALL_PATH is absolute (i.e. starts with a "/"),
the path ends up with a double "/".

While this is mostly equivalent to a single slash, it may have a nasty
side effect when:
- OpenCV_INSTALL_PATH is empty
- OPENCV_INCLUDE_INSTALL_PATH is "/usr/include"
- the calling build script uses "-isystem" to specify the path to the
  headers of dependencies (to avoid warnings)

Specifying "-isystem /usr/include" breaks the path ordering, and GCC can
no longer find its "stdlib.h", thus CMake filters such statements.
Unfortunately it fails to do so when using "//usr/include".
@alalek
Copy link
Copy Markdown
Member

alalek commented Jun 28, 2019

OPENCV_INCLUDE_INSTALL_PATH
absolute

It should not (by CMake guidelines).
Instead of /usr/include you should use include (/usr is controlled by CMAKE_INSTALL_PREFIX).

Otherwise you break staging installations with DSTDIR

@StefanBruens
Copy link
Copy Markdown
Contributor Author

No, thats not correct, see
https://github.com/opencv/opencv/blob/master/CMakeLists.txt#L595

and
https://cmake.org/cmake/help/v3.4/module/GNUInstallDirs.html

CMAKE_INSTALL_FULL_

The absolute path generated from the corresponding CMAKE_INSTALL_<dir> value. **If the value is not already an absolute path,** an absolute path is constructed typically by prepending the value of the CMAKE_INSTALL_PREFIX variable. However, there are some special cases as documented below.

i.e. CMAKE_INSTALL_* is definitely allowed to be an absolute dir.

@alalek
Copy link
Copy Markdown
Member

alalek commented Jun 28, 2019

We are about different things.

All OPENCV_<something>_INSTALL_PATH should be relative (except may be "/etc" cases, but we don't have configs yet).
Absolute path is controlled by CMAKE_INSTALL_PREFIX (in your case it is /usr).


OpenCV_INSTALL_PATH is empty

How it can be empty?

get_filename_component(OpenCV_CONFIG_PATH "${CMAKE_CURRENT_LIST_DIR}" REALPATH)
get_filename_component(OpenCV_INSTALL_PATH "${OpenCV_CONFIG_PATH}/@OpenCV_INSTALL_PATH_RELATIVE_CONFIGCMAKE@" REALPATH)

/ in worst case (which cause triple slashes /// in the final expression).


Finally I believe it should be resolved in a different way.
By fixing the actual problem:

Specifying "-isystem /usr/include" breaks the path ordering

Probably OpenCVConfig.cmake should filter out entries which intersects with CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES list.

@StefanBruens
Copy link
Copy Markdown
Contributor Author

At least NixOS and openSUSE use absolute paths for CMAKE_INSTALL_<dir>, so these break.

To give you another data point:
https://cmake.org/cmake/help/v3.14/module/CMakePackageConfigHelpers.html

All 4 options shown above are not sufficient, since the first 3 hardcode the absolute directory locations, and the 4th case works only if the logic to determine the installedPrefix is correct, and if CONFIG_INSTALL_DIR contains a relative path, which in general cannot be guaranteed.

From the generated file:
get_filename_component(OpenCV_INSTALL_PATH "${OpenCV_CONFIG_PATH}/../../../../" REALPATH)

@alalek
Copy link
Copy Markdown
Member

alalek commented Jun 28, 2019

Please try this fix with your configurations: alalek@pr14925_fix

P.S. CMAKE_CXX_IMPLICIT_INCLUDE_DIRECTORIES filtering is not needed, because CMake do that (works well in CMake 2.8.12.2+ at least).

Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Lets put it in - it should not break anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants