microsoft / vcpkg

C++ Library Manager for Windows, Linux, and MacOS
MIT License
21.91k stars 6.09k forks source link

[OpenCV] fix compatibility with VTK9 #12785

Closed cenit closed 3 years ago

cenit commented 3 years ago

After VTK9 update in #12149, VTK feature in OpenCV was broken. This patch will fix it.

JackBoosY commented 3 years ago

LGTM.

cenit commented 3 years ago

back to draft: there are some problems in downstream usage. Might require deeper fixes unfortunately

JackBoosY commented 3 years ago

@cenit Please ping me if this PR is ready for review.

Neumann-A commented 3 years ago

@cenit: any application using the viz module which can be put into scripts\test_ports ?

cenit commented 3 years ago

I can look for one. The one I am working on right now is not open source :)

cenit commented 3 years ago

I had to patch vtk port, it was failing otherwise in downstream projects using the library...

cenit commented 3 years ago

A simple test project would have detected that vtk had green mark from CI but was not working in reality, since the linking stage was failing

cenit commented 3 years ago

@JackBoosY PR might be ready. I have enabled VTK also on static builds. Initial tests look fine, both on OpenCV3 and on OpenCV4.

Neumann-A commented 3 years ago

A simple test project would have detected that vtk had green mark from CI but was not working in reality, since the linking stage was failing

WTF? ParaView is not enough for you? It is also building a runable executable... Furthermore #11148 and #9960 were reviewed by one of the VTK maintainers. So if there are problems with VTK please do the following:

  1. make sure it is not vcpkg related
  2. make sure it is not a case of wrong usage.
  3. search for upstream issues in https://gitlab.kitware.com/groups/vtk/-/issues
    • if there is none with a similar problem please open up an upstream issue and mention it in the portfile.
    • if there is one then there is probably already a patch. If not mention that you also ran into the problem in the upstream issue and patch it in vcpkg accordingly. Also mention the issue in the portfile.
cenit commented 3 years ago

very easy check: try building this sample code without that fix Might be that Paraview is silently patching this failure? Or that maybe if you only built a dynamically linked .exe it was failing if tried to run?

#include <vtkSmartPointer.h>
#include <vtkTransform.h>
#include <vtkMath.h>

int main()
{
  vtkSmartPointer<vtkTransform> transform = vtkSmartPointer<vtkTransform>::New();
  return 0;
}

it was broken on linux (at least) without that change :)

ps: I also suggest you to moderate your tones. Also, are you sure you have any power to veto or delay a PR so strongly?

Neumann-A commented 3 years ago

Might be that Paraview is silently patching this failure? Or that maybe if you only built a dynamically linked .exe it was failing if tried

ParaView is not silently patching anything of VTK and is building on osx, linux and windows (x64&static; x86 is not tested due to a cascade failure) without issues (see vcpkg CI logs, basically everything rebuilding qt also rebuilds paraview). If it would have been patched by ParaView it would have made it directly into VTK since both ports are highly connected sharing devs.

ps: I also suggest you to moderate your tones.

The tone was more disbelief than anger if you read it with that tone.

Also, are you sure you have any power to veto or delay a PR so strongly?

I don't have any power I can just give strong suggestions ;) But you are neither disclosing a full repro including the CMakeLists.txt and feature set of the installed VTK nor have you opened up an upstream issue until now.
VTK is very active in development so delaying the PR until the root cause for the failure has been discussed or the failure has been identified as a bug and not a usage error seems reasonable to me. I am just considering probabilities here. From my point of view a usage error on your side has a higher probability than an error in VTK. As such I want the issue fully discussed before any action is taken and I don't mind being wrong in the end.

Neumann-A commented 3 years ago

CMakeLists.txt

cmake_minimum_required(VERSION 3.17)
project(vtk_test)
find_package(VTK CONFIG REQUIRED)
add_executable(vtk_test main.cpp)
target_link_libraries(vtk_test VTK::CommonCore VTK::CommonTransforms VTK::CommonMath)

main.cpp

#include <vtkSmartPointer.h>
#include <vtkTransform.h>
#include <vtkMath.h>

int main()
{
  vtkSmartPointer<vtkTransform> transform = vtkSmartPointer<vtkTransform>::New();
  return 0;
}

Terminal Output:

neumann@Iluinrandir:~/vtk_test$ make
Scanning dependencies of target vtk_test
[ 50%] Building CXX object CMakeFiles/vtk_test.dir/main.cpp.o
[100%] Linking CXX executable vtk_test
[100%] Built target vtk_test

I don't see any issues..... so somebody has to dig deeper. (Installed VTK: vtk[core,opengl,paraview,qt,vtkm]:x64-linux)

JackBoosY commented 3 years ago

If this PR-related issue is related to vtk, do we need to call vtk official review this PR?

cenit commented 3 years ago

Sorry I am on vacation from today for few days What if you replace the find package call with find_package(VTK 9 NAMES vtk COMPONENTS FiltersExtraction FiltersSources FiltersTexture IOExport IOGeometry IOPLY InteractionStyle RenderingCore RenderingLOD RenderingOpenGL2 NO_MODULE REQUIRED) and then the target creation with


add_executable(vtk_test vtk_test.cpp)
target_include_directories(vtk_test PRIVATE ${VTK_INCLUDE_DIRS})
target_link_libraries(vtk_test PRIVATE ${VTK_LIBRARIES})

?

This config was failing for me without that fix. This is how vtk is used in the opencv community, and it was working before vtk9

Neumann-A commented 3 years ago

Opened up https://gitlab.kitware.com/vtk/vtk/-/issues/17977

cenit commented 3 years ago

Absolutely no love upstream, despite what was vehemently said, but the problem is very real, again despite what was initially said against this PR. The PR seems not producing any regression, while it's also fixing OpenCV's VTK feature. Can we consider it for merge?

Neumann-A commented 3 years ago

Absolutely no love upstream

sometimes you need to poke people to get an answer ;)

But I am ok with it being merged if you add the comment about the upstream issue. If it is resolved the next person knows that the code may be removed/is unnecessary. The main objection here was missing documentation why the change is necessary and how to reproduce the issue.

cenit commented 3 years ago

Ok, the note was added. I also did something similar to paraview also for OpenCV in this PR, in order to keep openCV verified also for optional features.

JackBoosY commented 3 years ago

Will test features after pipeline test pass.

cenit commented 3 years ago

The main objection here was missing documentation why the change is necessary and how to reproduce the issue.

Ok, but I think the title said it all. VTK feature in OpenCV is broken. No need to provide another smoking gun evidence of the problem, which was anyway provided later on, on your request

Neumann-A commented 3 years ago

@cenit: Got a patch from upstream mind trying it out in CI?

diff --git a/CMake/vtkModule.cmake b/CMake/vtkModule.cmake
index 4093d2d826..3d844dd50f 100644
--- a/CMake/vtkModule.cmake
+++ b/CMake/vtkModule.cmake
@@ -4409,6 +4409,15 @@ endif ()\n\n")
   foreach (_vtk_export_module IN LISTS _vtk_export_MODULES)
     get_property(_vtk_export_target_name GLOBAL
       PROPERTY "_vtk_module_${_vtk_export_module}_target_name")
+    # Use the export name of the target if it has one set.
+    get_property(_vtk_export_target_has_export_name
+      TARGET    "${_vtk_export_target_name}"
+      PROPERTY  EXPORT_NAME SET)
+    if (_vtk_export_target_has_export_name)
+      get_property(_vtk_export_target_name
+        TARGET    "${_vtk_export_target_name}"
+        PROPERTY  EXPORT_NAME)
+    endif ()
     set(_vtk_export_base "_vtk_module_find_package_${_vtk_export_module}")
     get_property(_vtk_export_packages GLOBAL
       PROPERTY "${_vtk_export_base}")

(In my test it worked)

JackBoosY commented 3 years ago

json5-parser regressions will be fixed by PR #13289.

JackBoosY commented 3 years ago
CMake Error at /agent/_work/1/s/scripts/buildsystems/vcpkg.cmake:378 (_add_library):
  Target "opencv_text" links to target "LibXml2::LibXml2" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?
Call Stack (most recent call first):
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVUtils.cmake:1508 (add_library)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVModule.cmake:929 (ocv_add_library)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVModule.cmake:850 (_ocv_create_module)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVModule.cmake:1071 (ocv_create_module)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-1c7c3cc613.clean/modules/text/CMakeLists.txt:7 (ocv_define_module)

CMake Error at /agent/_work/1/s/scripts/buildsystems/vcpkg.cmake:378 (_add_library):
  Target "opencv_text" links to target "OpenSSL::Crypto" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?
Call Stack (most recent call first):
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVUtils.cmake:1508 (add_library)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVModule.cmake:929 (ocv_add_library)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVModule.cmake:850 (_ocv_create_module)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVModule.cmake:1071 (ocv_create_module)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-1c7c3cc613.clean/modules/text/CMakeLists.txt:7 (ocv_define_module)
...
cenit commented 3 years ago

@JackBoosY thanks for the help. ~I should have fixed regressions~ Nope sorry I misunderstood the problem. I will try to fix regressions ASAP

JackBoosY commented 3 years ago

Sadly, you didn't.

cenit commented 3 years ago

@JackBoosY I tried to analyse the situation. Technically this is a regression which is already in master and it does not depend on OpenCV. There are some dependencies which are not fully exporting their own transitive dependencies when used and so it makes downstream projects unable to use them. They just appeared because in this PR I expanded CI coverage for OpenCV, otherwise it gets slowly "damaged" over time by other port updates. Unfortunately I arrived late. These regressions will take time for me to identify and fix First of all, I need to identify which ports are depending on LibXML2 and OpenSSL and are exporting them without a find_dependency call first to define targets. Afterwards I will have to find a way to fix them. At first sight, they might be qt5-base (openssl) and vtk (libxml2), but I might be wrong.

Neumann-A commented 3 years ago

@cenit: qt5-base does not link any cmake targets. It is extracting its complete link interface from the *.prl files qmake generates. You might want to run vcpkg_configure_cmake with --trace in CI to get better logs

cenit commented 3 years ago

@JackBoosY I created a new env especially for this problem. For now I cannot reproduce the issue with these features: opencv4[contrib,core,dnn,jpeg,opengl,png,tiff,vtk,webp]:x64-linux So it looks like vtk has no faults

I will test opencv[nonfree,contrib,dnn,eigen,ffmpeg,jasper,jpeg,openexr,opengl,png,qt,sfm,tiff,vtk,webp] now

cenit commented 3 years ago

@JackBoosY I cannot reproduce any failure locally even with the OpenCV fully featured. Can you?

(but I see for example that ffmpeg feature is silently broken on linux...)

Neumann-A commented 3 years ago

@cenit: Probably needs a feature check of dependent ports and see if something is build with a different feature-set than you are testing with. Maybe post avcpkg list result for x64-linux and compare against CI

JackBoosY commented 3 years ago

Can't repro this regression on my ubuntu machine.

CMake Error at /agent/_work/1/s/scripts/buildsystems/vcpkg.cmake:395 (_add_library):
  Target "opencv_text" links to target "LibXml2::LibXml2" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?

When I built opencv4:x64-linux, libxml2 was not required to build.

cenit commented 3 years ago

It is required now in CI due to vcpkg-ci-opencv (which is a fully featured opencv). Still, I cannot reproduce it even with a fully featured one. We should investigate it more (I admit I had no time for this)

JackBoosY commented 3 years ago

@cenit Testing all features now...

JackBoosY commented 3 years ago

No...I'm always stuck at the step of building llvm:

collect2: fatal error: ld terminated with signal 9 [Killed]
compilation terminated.
ninja: build stopped: subcommand failed.
cenit commented 3 years ago

on linux? is it the halide feature? All the other features are ok?

JackBoosY commented 3 years ago
CMake Error at /agent/_work/1/s/scripts/buildsystems/vcpkg.cmake:410 (_add_library):
  Target "opencv_text" links to target "LibXml2::LibXml2" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?
Call Stack (most recent call first):
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVUtils.cmake:1508 (add_library)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVModule.cmake:929 (ocv_add_library)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVModule.cmake:850 (_ocv_create_module)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-b809df3922.clean/cmake/OpenCVModule.cmake:1071 (ocv_create_module)
  /mnt/vcpkg-ci/buildtrees/opencv4/src/4.3.0-1c7c3cc613.clean/modules/text/CMakeLists.txt:7 (ocv_define_module)

I can't find modules/text on my machine, which feature triggered it? And that feature should be depend on libxml2.

cenit commented 3 years ago

It should be one of the modules of opencv[contrib] but contrib does not depend on libxml2 (and adding it does not solve the problem, that's not the problem). It might be some of its dependencies that are transitively not exporting correctly the symbol downstream. But the main problem is that I cannot reproduce it locally. Can you?

JackBoosY commented 3 years ago

@cenit No, I can't. I passed the configure step.

cenit commented 3 years ago

And if you add all opencv features (apart from halide, which triggers the llvm problem you had before)?

JackBoosY commented 3 years ago

@cenit Only feature halide triggered llvm.

cenit commented 3 years ago

I mean, can you pass configure step with all features enabled (apart from halide)? I can, and so I cannot reproduce CI results locally

Neumann-A commented 3 years ago

please c&p the output of vcpkg.exe depend-info opencv4[*] here. I can take a look and compare against CI

JackBoosY commented 3 years ago
root@root:/home/work/12785/vcpkg# ./vcpkg  install opencv4[*]
Computing installation plan...
The following packages will be built and installed:
  * halide[core]:x64-linux
  * llvm[clang,core,disable-abi-breaking-checks,disable-assertions,disable-clang-static-analyzer,enable-rtti,lld,tools]:x64-linux
  * openblas[core]:x64-linux
    opencv4[ade,contrib,core,cuda,dnn,eigen,ffmpeg,gdcm,halide,ipp,jasper,jpeg,nonfree,openexr,opengl,openmp,ovis,png,qt,sfm,tbb,tiff,vtk,webp,world]:x64-linux
  * pegtl-2[core]:x64-linux
  * proj4[core,database]:x64-linux
  * qt5-base[core]:x64-linux
  * tbb[core]:x64-linux
  * utfcpp[core]:x64-linux
  * vtk[core]:x64-linux
Additional packages (*) will be modified to complete this operation.
Detecting compiler hash for triplet x64-linux...
^C
cenit commented 3 years ago

vcpkg.exe depend-info opencv4[*]

./vcpkg depend-info opencv[nonfree,ade,contrib,dnn,eigen,ffmpeg,gdcm,ipp,jasper,jpeg,openexr,opengl,openmp,png,qt,sfm,tiff,vtk]
zlib:
openssl-unix:
libjpeg-turbo:
liblzma:
openssl: openssl-unix
libpng: zlib
szip:
egl-registry:
brotli:
bzip2:
tiff: libjpeg-turbo, liblzma, zlib
opengl-registry: egl-registry
libffi:
xxhash:
libogg:
hdf5[szip, zlib]: szip, zlib
curl[openssl, ssl, non-http]: openssl, zlib
libiconv:
giflib:
gflags:
libwebp[simd, nearlossless]:
freetype[png, bzip2]: brotli, bzip2, libpng, zlib
ragel:
sqlite3[tool]:
dirent:
expat:
zstd:
lz4: xxhash
opengl:
netcdf-c: curl, hdf5
openjpeg:
pcre2:
pegtl-2:
proj4[database]: sqlite3
pugixml:
python3: libffi, openssl, zlib
utfcpp:
libtheora: libogg
fontconfig: dirent, expat, freetype, libiconv
freeglut:
eigen3:
glew:
double-conversion:
glog: gflags
harfbuzz: freetype, ragel
icu:
jsoncpp:
leptonica: giflib, libjpeg-turbo, libpng, libwebp, tiff, zlib
libharu[notiffsymbols]: libpng, tiff, zlib
libpq[zlib, openssl]: openssl, zlib
angle: egl-registry, opengl-registry
libxml2: libiconv, liblzma, zlib
vtk: double-conversion, eigen3, expat, freetype, glew, hdf5, jsoncpp, libharu, libjpeg-turbo, liblzma, libogg, libpng, libtheora, libxml2, lz4, netcdf-c, pegtl-2, proj4, pugixml, sqlite3, tiff, utfcpp, zlib
ceres: eigen3, glog
tesseract: leptonica
ade:
ffmpeg[swscale, swresample, postproc, avresample, avformat, avfilter, avdevice, avcodec]:
qt5-base: angle, double-conversion, egl-registry, fontconfig, freetype, harfbuzz, icu, libjpeg-turbo, libpng, libpq, openssl, pcre2, sqlite3, zlib, zstd
gdcm: expat, openjpeg, zlib
protobuf:
jasper: freeglut, libjpeg-turbo, opengl
openexr: python3, zlib
opencv4[tiff, sfm, qt, webp, png, openmp, ipp, gdcm, jpeg, jasper, ffmpeg, eigen, vtk, openexr, dnn, opengl, nonfree, contrib, ade]: ade, ceres, eigen3, ffmpeg, gdcm, gflags, glog, hdf5, jasper, libjpeg-turbo, libpng, libwebp, openexr, opengl, protobuf, qt5-base, tesseract, tiff, vtk, zlib
opencv[tiff, sfm, qt, webp, png, openmp, ipp, gdcm, jpeg, jasper, ffmpeg, eigen, vtk, openexr, dnn, opengl, nonfree, contrib, ade]: opencv4
cenit commented 3 years ago

I can build it locally on ubuntu 20.04 without any problem

Neumann-A commented 3 years ago

@cenit Have you tried to built libxml2 first and after that build everything opencv4 needs? I already checked the depend-info vs CI and it looks like some port has a hidden dependency on libxml2.

cenit commented 3 years ago

Good idea. I will do it tonight and check if I can recover the error.

cenit commented 3 years ago

error disappeared from Linux and appeared in macOS. Due to the similarities in the build toolchains, it looks really like it is due to hidden dependencies on OpenSSL and LibXML2 somewhere in some ports that just was casually removed from the Linux baseline and added to the osx recently

JackBoosY commented 3 years ago

@cenit I think so. That's affect the *inx OS.

cenit commented 3 years ago

@cenit Have you tried to built libxml2 first and after that build everything opencv4 needs? I already checked the depend-info vs CI and it looks like some port has a hidden dependency on libxml2.

@Neumann-A I did it. And with much sadness, after having created an empty environment, installed libxml2 manually at the beginning and then reinstalled everything, it built successfully.