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

OpenCVModules.cmake has wrong libraries on static windows, static standalone IPP #19282

Open
3 tasks done
diablodale opened this issue Jan 7, 2021 · 8 comments
Open
3 tasks done

Comments

@diablodale
Copy link
Contributor

opencv built on windows, building static opencv libraries, with standalone IPP, with IPP static libraries creates wrong downstream CMake package files -- those files that are later included in applications that build with opencv as a library. These downstream package files fail to have the needed mt at the end of the library name.

These package files are created when OpenCV is built/installed. They declare a list of libraries to be linked and downstream apps that are built with cmake use that list when find_package(opencv)...target_link_libraries(${OpenCV_LIBS}) etc. is used. That list is incorrect. The errant list is ippi, ippcv, etc. The correct list is ippimt, ippcvmt, etc.

Easy fix with PR I have ready.

System information (version)
  • OpenCV => 4.5.1
  • Operating System / Platform => Microsoft Windows [Version 10.0.19042.685] 64-bit
  • Compiler => Visual Studio 2019 Community v16.8.3
Steps to reproduce
  1. Setup build machine as a describe above
  2. With_IPP=ON, IPPROOT=xxxx, BUILD_WITH_DYNAMIC_IPP=OFF, BUILD_IPP_IW=OFF, BUILD_SHARED_LIBS=OFF
  3. Build opencv
  4. install
  5. inspect install/Release/x64/vc16/staticlib/OpenCVModules.cmake
  6. (Optional) create downstream application with cmake that uses opencv and ipp functionality. Try to build that application.

You will see incorrect libraries listed in (5). And a build of a downstream application will fail with libraries not found with (6)

INTERFACE_LINK_LIBRARIES ...$<LINK_ONLY:ippcv>;\$<LINK_ONLY:ippi>;\$<LINK_ONLY:ippcc>;

The correct list is

INTERFACE_LINK_LIBRARIES ...$<LINK_ONLY:ippcvmt>;\$<LINK_ONLY:ippimt>;\$<LINK_ONLY:ippccmt>;
Issue submission checklist
  • I report the issue, it's not a question
  • I checked the problem with documentation, FAQ, open issues,
    answers.opencv.org, Stack Overflow, etc and have not found solution
  • I updated to latest OpenCV version and the issue is still there
@alalek
Copy link
Member

alalek commented Jan 7, 2021

CMake's target name is not the same as the library name. They can be different.

For static distribution it is "user" responsibility to track and properly define used external dependencies (their targets) before using find_package(OpenCV) with the static builds.
(OpenCV build scripts can't behave like the package manager).


Unfortunately there is no find_package(IPP), at least its reference code.
So there is no "the right way" of handling that.


INTERFACE_LINK_LIBRARIES ...$<LINK_ONLY:ippcv>;\$<LINK_ONLY:ippi>;\$<LINK_ONLY:ippcc>;

I would prefer to keep this current approach. At least as the default (to avoid breaking of existed external scripts).

@diablodale
Copy link
Contributor Author

Hi. This has nothing to do with find_package(IPP).

The file OpenCVModules.cmake is generated by the build of OpenCV itself. Then later, when an app uses find_package(opencv), several files including this OpenCVModules.cmake are loaded into the app's cmake setup. It is here that that the error occurrs. The libraries listed in that generated file are injected and not controlled by the app's cmake. It is controlled by find_package(opencv) and it generating the list of libraries for OpenCV as a library within another application.

There is no way for an app to stop this injection. It is happening with anyone building it like me. Which as I suspect, is a very small population of people since it has to be someone building opencv for themselves as: windows, static opencv libs, ipp standalone, ipp static libs. For the 100 people in the world doing it, I'm confident that any build automation they made, will continue to work with this bug fix...or their hack to workaround this bug can be removed so it works naturally. This is a minor build bug, not a running library crash.

It passes my local tests and passed all your test automation tests.

Just like you preferred in aligned memory...making it work with the natural native method is better. The only workaround is to start probing every library opencv as a library lists as needing to linked, and then make a mapping of fixing the bug and inject that again into the apps cmake. Very messy and not susatainable.

@alalek
Copy link
Member

alalek commented Jan 8, 2021

not controlled by the app's cmake.

Agreed.
Like OpenCV can't control dependencies of static dependencies directly (for example, static FFmpeg build). This is why I mentioned the "package manager" role.
CMake 3.0+ way is using of find_dependency(), like find_dependency(IPP) (but there are no instructions how users should get this IPP's CMake config). In ideal world it should look like find_dependency(IPP <VERSION> ...) with propagation of extra parameters like BUILD_WITH_DYNAMIC_IPP.


It passes my local tests

Just "correct" library name is not enough.
Build environment need to be externally hacked with the correct libraries paths too. I believe it is done by IPP's vars script.
Moreover it should point to the correct/compatible version of IPP binaries which is a tricky because they are distributed separately. Also there is no versions check in this approach.

and passed all your test automation tests

Because we don't test distribution (through "install") of static builds widely (this configuration is not well supported and rarely used).
There are some exceptions like OpenCV4Android SDK, but dependencies are fixed for that case.

@diablodale
Copy link
Contributor Author

diablodale commented Jan 8, 2021

I believe we can fix the broken build. As it is today, it is broken...apps will not compile that use the static scenario I describe in my OP. In my experience a "broken build" is one of the most important bugs to fix. We both agree there is a small set of people that are affected. Then I suggest we fix this because few are affected, therefore if there is some bug in the fix, then again few are affected. And it is the build...not a runtime code problem.

Yes, there are at least 3 issues with opencv's library injection:

  1. The problem I write in my OP. The name "ippi" is wrong for static IPP. Won't compile. It should be "ippimt".
  2. The path is missing. When I look at the full list of libraries that has been injected by all packages, most of them have full path names. Even FindXXX.cmake that I have written inject full path names. It is written in official cmake documentation to inject the full pathname so that you get the exact library that is needed. OpenCV's cmake code is not following the cmake standards on this.
  3. OpenCV injects multiple copies of the same library. Some have paths, some do not have paths. This causes version problems, path search problems, need to hack library paths, etc. And due to above (2) and (3), it causes both bugs to appear.

I have a fix in PR for (1). I have a workaround for (2) in my CMakeLists.txt. A fix for (3) needs more thought.

I recommend we make the build not break. I recommend we fix (1). Currently, it is broken for the scenario in my OP. Apps will not compile because opencv is injecting the wrong names of libraries. The only fix is my PR -- or to do very manual manipulation of app target library lists in CMake by parsing them, searching for all OpenCV libraries (which is not always known due to changes in module dependencies), then re-writing those libraries with whatever fix is needed (IPP needs "mt"). I started to write this workaround but it is so messy...I chose to fix the bug instead.

If OpenCV can't inject a correct list of libraries, then it should NOT inject libraries. Make the app do it all manually. Because the fixing of wrong libraries is harder than building a known set of libraries manually. Of course...I wouldn't suggest you actually stop injecting...that would break many many people.

Again this is not relevant: "...but there are no instructions how users should get this IPP's CMake config". You keep writing this and I keep telling you this is not the issues I am reporting. I think we have a language barrier occurring. How can I better explain this to you? 🤔

Apps don't load IPP. Apps don't find_package(IPP). Instead apps write find_package(opencv) and it is that opencv cmake files I named above that do the wrong injection. OpenCV is generating wrong lists of libraries.

Here is a sample CMakeFile.txt you can see. There is some extra things in there that I need for my specific setup. I am confident you can understand.

cmake_minimum_required(VERSION 3.18)
cmake_policy(SET CMP0091 NEW) # enable Msft VC runtime library selection

project(bug19282
    LANGUAGES CXX
)

set(OpenCV_STATIC ON)
find_package(
    OpenCV REQUIRED
    PATHS C:/repos-nobackup/opencv/.install/Debug
)

include_directories(${OpenCV_INCLUDE_DIRS})
add_executable(bug19282 myapp.cpp)
target_link_libraries(bug19282 ${OpenCV_LIBS})

Look above. IPP is not involved. This is all OpenCV. OpenCV is injecting the wrong library names. And therefore the app will not compile because those libraries don't exist on the hard drive (or are the wrong type static/dynamic) causing the linker to fail.

Don't compile. Just cmake configure the project, then look at build/build.ninja. You will see in that file ippi.lib, ippcc.lib, etc. All those are wrong names and the linker will fail. They need have have "mt" at the end of their names.

@alalek
Copy link
Member

alalek commented Jan 9, 2021

Please read the first sense of the first my comment.
OpenCV build scripts uses target names.
Scripts are moved away from library names with/without paths to using library names.
Just forget about the names of binaries - they are not reliable in the list of dependencies. Use targets.

For static builds is it responsibility of the user to properly configure external targets (because they are not re-distributed by OpenCV and scripts can't handle that).


You will see in that file ippi.lib, ippcc.lib

It is CMake fallback behavior for missing "targets".
Again, it is user's responsibility to specify proper targets (as imported) before calling find_package(OpenCV).


OpenCV is generating wrong lists of libraries.

There are no libraries. Just CMake targets.
OpenCV doesn't handle that completely because we don't want to maintain a package manager's logic in our scripts. Especially for the parts which we are not redistribute.

@diablodale
Copy link
Contributor Author

Thanks for more clarity. Give me a few days to digest this and think over all we have discussed. I will definitely close/reply before Wednesday.

@diablodale
Copy link
Contributor Author

I now follow the intention of OpenCV. It is technically a valid approach. However, it is harder for apps that static link since there is no "finder" written by OpenCV that matches OpenCV's specific symbolic target names. I think this is a disservice to a subset of your users. It makes them have to either: easy fix like my PR --or-- hard fix to write an entire "finder" that will match what OpenCV is outputting.

Given the relationship that OpenCV has with Intel -- I suggest that your core team escalate this gap to your Intel contacts. And have them write an official FindIPP.cmake and then submit that to Kitware for inclusion in cmake itself. Then when that work is done, OpenCV can migrate to use it.

Thoughts, opinions?

@janwilmans
Copy link

I used this to work around the issue, its ugly and hardcodes the paths, but it works for me:

set(open_cv_3rdparty 
    /usr/lib/opencv4/3rdparty/libippiw.a
    /usr/lib/opencv4/3rdparty/libippicv.a
    /usr/lib/opencv4/3rdparty/libade.a
    /usr/lib/opencv4/3rdparty/libittnotify.a
    /usr/lib/opencv4/3rdparty/libquirc.a
    /usr/lib/opencv4/3rdparty/liblibwebp.a
    /usr/lib/opencv4/3rdparty/liblibprotobuf.a
    /usr/lib/opencv4/3rdparty/liblibopenjp2.a
)

set(open_cv_all_libs
    /usr/lib/libopencv_imgproc.a
    /usr/lib/libopencv_core.a
    /usr/lib/libopencv_calib3d.a
    /usr/lib/libopencv_dnn.a
    /usr/lib/libopencv_features2d.a
    /usr/lib/libopencv_flann.a
    /usr/lib/libopencv_gapi.a
    /usr/lib/libopencv_highgui.a
    /usr/lib/libopencv_imgcodecs.a
    /usr/lib/libopencv_ml.a
    /usr/lib/libopencv_objdetect.a
    /usr/lib/libopencv_photo.a
    /usr/lib/libopencv_stitching.a
    /usr/lib/libopencv_video.a
    /usr/lib/libopencv_videoio.a
)

add_library(import_opencv_all INTERFACE)
target_link_libraries(import_opencv_all INTERFACE ${open_cv_all_libs} ${CMAKE_DL_LIBS} ${open_cv_3rdparty})
target_include_directories(import_opencv_all INTERFACE ${OpenCV_INCLUDE_DIRS})
target_compile_options(import_opencv_all INTERFACE -Wno-overloaded-virtual)

# after this you use it like:
target_link_libraries(${PROJECT_NAME}
    PRIVATE
        import_opencv_all
)

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

Successfully merging a pull request may close this issue.

3 participants