[OpenCV] patches for v3.4.0, use vcpkg external libraries, many static triplet fixes, opt-out from ninja parallel configure#2764
[OpenCV] patches for v3.4.0, use vcpkg external libraries, many static triplet fixes, opt-out from ninja parallel configure#2764cenit wants to merge 27 commits intomicrosoft:masterfrom cenit:master
Conversation
…s downloaded on the fly from parallel configs
|
Thanks. But this works for me, appreciate it! --update: |
|
could you please tell me more? I have a sample build project which worked ok.... |
|
I'm also getting multiple |
|
Maybe @jasjuang can help as well, he updated from 3.3 to 3.4 which broke the patches. |
|
This PR fixes that problem. I am collecting problems if you have any. I will work on opencv[ffmpeg] also soon, because I need it. |
|
I also discussed about that patch (accepted too quickly imho) here #2762 |
What do you mean with that - does it not work for you? I had problems with it (see #2613) but since @ras0219-msft can't reproduce them, I am just trying again now to see whether it's a local problem or one with the vcpkg port. What do you think about #2794? Thank you very much for your work @cenit. |
|
It just means that I have not tested OpenCV[ffmpeg] for now, so I don't want to say that it's working without having tried it! |
|
I can try to exploit feature packages and extract OpenCV-contrib and protobuf, #2794 is a good idea. Unfortunately my time is really constrained now, but since I need this package working (my time spent fixing it would be much more reasonable than any time spent manually installing on many system the library) I will find some time to fix it. If you want access to my repo to contribute to it (automatically only maintainers, so MSFT's Robert and Alex, have it), please let me know so that I can add you manually |
|
One thing to point out first, in the usage It seems that you remove the patches I did earlier, which might be the cause of the path issues that @lijgame is seeing. I will test out this PR later. |
If OpenCV needs more files, we should do the download ourselves and place it into the right place. In order to ensure things like proxy support / authentication / determinism, we need to have all the downloads occur as directly in the portfile as possible.
I'll look into this!
We should have fixed this in our toolchain file [1]. Could you go into a bit more detail here about what specifically isn't working? |
|
mycpp.cpp can be as simple as After running cmake configuration for the downstream, the below error shows up Additionally when I tested out |
|
@jasjuang which patch are you talking about? I ported patches, not removed any... |
While I fully understand your point, the fact is that dynamic downloading of files is embedded in OpenCV CMake toolchain through the OpenCVDownload.cmake functions (available in the
There are some unresolved lzma symbols when using static builds of OpenCV. As you can see from the patch attached to the first message, the default |
|
If you made changes to |
|
There are no traces of those kind of paths prefixes in the machines on which I am testing (and already using) this PR, so please double check it. I could miss problems because all of the machines ultimately are made from the same setup, but still that problem should not be present here (maybe you referenced the wrong vcpkg toolchain, if you have many vcpkg installations?). |
|
@ras0219-msft @alexkaratarakis I am having some problems: when a library has the same name in both debug and release, FindXXX will have a hard time distinguishing them. In particular, the great |
|
by the way, I fear that from the license point of view, a static linked software containing both GPL code (ffmpeg) and BSD code (OpenCV) is not redistributable. Am I right? |
…figuration steps to be in line with build steps
|
also OpenEXR is now used from vcpkg (the library did not build, I had to add some patches to it too). |
| + IexMathFloatExc.cpp | ||
| IexMathFpu.cpp | ||
| ) | ||
| -TARGET_LINK_LIBRARIES(IexMath Iex) |
There was a problem hiding this comment.
Why is this needed? I am able to build locally without this patch for both x86-windows and x64-windows-static, even when including the changes below to the portfile.
There was a problem hiding this comment.
If you use the ilmlibrary, usually you have to link to Iex and IexMath and others (as also examples do). If you link Iex library symbols into IexMath you have symbols multiply defined. Also, linking a library to another one is not a best practice.
The library of course builds perfectly also without the patch, but you cannot use it. In the original conditions any symbol of Iex not included into IexMath requires linking also to Iex, but you cannot do it because of the problem just described.
There was a problem hiding this comment.
Ok after more investigation, I think I should drop this patch too
ports/ilmbase/portfile.cmake
Outdated
| "${CMAKE_CURRENT_LIST_DIR}/fix-parallel-build.patch" | ||
| "${CMAKE_CURRENT_LIST_DIR}/IexMath__CMakeLists.txt.patch" | ||
| "${CMAKE_CURRENT_LIST_DIR}/IlmThread__CMakeLists.txt.patch" | ||
| "${CMAKE_CURRENT_LIST_DIR}/Imath__CMakeLists.txt.patch" |
There was a problem hiding this comment.
I am able to build without these three patches -- what are they used for?
There was a problem hiding this comment.
To remove the interlinking between libraries, which make them not working if you try to use them (downstream)
ports/ilmbase/portfile.cmake
Outdated
| set(BUILD_SHARED_LIBS OFF) | ||
| else() | ||
| set(BUILD_SHARED_LIBS ON) | ||
| endif() |
There was a problem hiding this comment.
This is not needed -- we automatically set BUILD_SHARED_LIBS inside vcpkg_configure_cmake based on the VCPKG_LIBRARY_LINKAGE setting
There was a problem hiding this comment.
ok good, I will remove it :)
ports/opencv/FindFFMPEG.cmake
Outdated
| @@ -0,0 +1,105 @@ | |||
| # Distributed under the OSI-approved BSD 3-Clause License. | |||
There was a problem hiding this comment.
Checking in source code under a different license than Vcpkg is a problem. Instead, can we download these from their original locations using vcpkg_download_distfile()?
Or, we can author shorter alternatives that are more specialized for Vcpkg.
Finally, it makes more sense to me to move these to the respective publishing port (in this particular case FFMPEG) and putting it somewhere like /share/ffmpeg/FindFFMPEG.cmake. This won't be picked up by default from CMake, but then this port could explicitly pass
vcpkg_configure_cmake(
…
OPTIONS
"-DCMAKE_MODULE_PATH=${CURRENT_INSTALLED_DIR}/share/X;..."
)There was a problem hiding this comment.
I wrote all of them. I can move them wherever you need them and also relicense as you like
ports/opencv/FindJPEG.cmake
Outdated
| @@ -0,0 +1,61 @@ | |||
| # Distributed under the OSI-approved BSD 3-Clause License. | |||
There was a problem hiding this comment.
See comments about FindFFMPEG
There was a problem hiding this comment.
Shall you give me the proper license tag to put on the header? Also, this port is becoming huge. I think I will submit all these FindXXX.cmake modules to official cmake repository, but it will take time (FindFLTK is there since so many months, accepted but CMake 3.11 is not coming out!! these ones should be easier, as you can see, the includes are almost ready for official inclusion, but even if they are considered for CMake 3.12, it means 6 months at least), so having them here in the meantime can speed up things a lot
| @@ -0,0 +1,19 @@ | |||
| diff --git a/IlmImfUtilTest/CMakeLists.txt b/IlmImfUtilTest/CMakeLists.txt | |||
There was a problem hiding this comment.
Is this patch needed given that you've removed the add_subdirectory(IlmImfUtilTest) above?
There was a problem hiding this comment.
Same as before, will remove in the next commit
ports/openexr/portfile.cmake
Outdated
| set(BUILD_SHARED_LIBS OFF) | ||
| else() | ||
| set(BUILD_SHARED_LIBS ON) | ||
| endif() |
There was a problem hiding this comment.
This is not needed, see previous comments.
| @@ -74,6 +74,10 @@ function(vcpkg_configure_cmake) | |||
| set(NINJA_CAN_BE_USED OFF) | |||
There was a problem hiding this comment.
Please make these changes in a separate PR.
| endif() | ||
|
|
||
| if (_csc_DISABLE_PARALLEL_CONFIGURE) | ||
| message(STATUS "Disabling parallel configure - package has opted-out") |
There was a problem hiding this comment.
I don't believe being noisy here is beneficial
There was a problem hiding this comment.
Right. It was necessary for me during the debug, to be sure that the correct path was chosen. Will remove!
| message(STATUS "Configuring ${TARGET_TRIPLET}-dbg done") | ||
| endif() | ||
|
|
||
| if(NOT DEFINED VCPKG_BUILD_TYPE OR VCPKG_BUILD_TYPE STREQUAL "release") |
There was a problem hiding this comment.
Please make this swap (Debug first) in a separate PR!
|
I'm going to prepare a rebased branch with some of these changes I've requested |
|
on my train to a conference, I implemented the external TIFF library detection fixing the LZMA dependency (it required the deployment of another couple of FindXXX.cmake modules, both for TIFF and for LibLZMA...), but now it works. I will also rewrite git history with a force push before merging, so we can have only the necessary number of commits and any trace of wrong licenses. |
|
Ok, I've rebased, squashed, cherry-picked, and minimized my way to a (small) victory: I've prepared two branches: one without the ffmpeg change and one with. I think we should deliberate a bit more on what the right answer is w.r.t. the ffmpeg change (licenses as well as the upstream choices make me hesitant), however the branch without external ffmpeg LGTM. I've also split out the various Barring bugs, I plan to merge opencv-rework soon. We can then keep working on the external ffmpeg and switching the dependencies to use debug suffixes. |
|
First of all, thank you for revising the patchset and for all the small enhancements along the road.
For the license problem, I would not give it too much weight.
On all the other systems,
Ok let me know how to proceed in the best coordinated way. I don't want to do anything that you have already done and I am sure also the vice-versa is valid. In the meantime I will also open some PR in the [1] https://github.com/opencv/opencv_3rdparty/tree/ffmpeg/master/ffmpeg |
|
I've merged the OpenCV-rework branch to master!
I am simply concerned about blocking users from using the dynamic ffmpeg (which is currently the out-of-the-box experience with OpenCV on Windows/MSVC, even when building statically). However, this should still be possible with our triplets system, so I think that while changing the default is always something worth contemplating, it doesn't block us here.
That's a good argument. I'm mostly convinced, but I'll wait a bit longer for other users to potentially chime in (@jasjuang @lp55 @Mixaill @UnaNancyOwen @amincheloh @zabulus @tobiaskohlbau -- you've all contributed in the past so please chime in if you feel one way or another!).
I see a few things:
Sorry for doing the separate rebase, but I didn't want to force-push-rebase over your master branch 😢. I still really appreciate what you've provided here -- it really makes up the majority of the branch I did merge! Unless you have some changes you'd like to apply on top of dev/roschuma/opencv-use-external-ffmpeg, I think it'd be best to close this PR and open new ones for any future work (also, please use separate branches when possible so I can contribute directly to them if I want simple formatting changes 😄)! [1] https://github.com/search?utf8=%E2%9C%93&q=filename%3Afindopenexr.cmake&type=Code |
|
Thanks for the work guys. I tested the builds and it works with my downstream project with both release and debug. |
I am testing it and it seems good. I understand your point on waiting for others before merging also the external
As I said, I will push these script upstream to CMake. I hope they can be accepted, even if the timing there is very slow. We can postpone the work on the debug suffixes when those scripts will be accepted, if ever, so that your concerns should not be a problem anymore. I just didn't check the
Thank you. In any case, the really important thing is that OpenCV now is in a much better state here on vcpkg. I was also contacted by people that used my fork to develop the OpenCV[cuda] feature up to a working state, I think that it will be my next step (together with the python libraries, I see that python is cited here sometimes and so maybe we can extend the OpenCV library to build also the python interfaces if requested) |
|
With the latest master branch of vcpkg, |
|
With regards to ffmpeg, I don't have a particular opinion. If OpenCV on all other platforms is using system ffmpeg too and not the OpenCV-bundled one, then that sounds like a good reason to use vcpkg-ffmpeg on Windows too. |
Exactly. But I can understand that we should just let the dust settle a bit. The rework is just big enough to deserve some days to see if there's a surge of issues related to it (I am almost sure not, on the other hands many problems are now solved) |
Just came across this again by coincidence. Strictly speaking OpenCV build does not trigger a FindOpenGL. It only triggers it when Also another thing, I'm not sure which module it is but some apps that I build with OpenCV, now show some output on the command-line about OpenCL initialisation - without me ever printing something. This must be one of OpenCV's modules - and I am definitely not using OpenCL. So I am not sure but I think this output message is a bit intrusive and it's on with vcpkg's default configuration of OpenCV. I don't really want any spurious output in my apps on the command line, particularly if I'm not even using OpenCL. I'd set building OpenCV with OpenCL off by default - I am surprised that it isn't. |
|
in the latest patchset, I explicitly treated OpenGL as a feature (a default one, by the way), but still you can force to disable it during install. |
Trying to use OpenCV from vcpkg, I encountered few problems, which have been fixed with this PR (I also cleaned up the commmit history before editing this message, removing unnecessary tests):
As a byproduct, now OpenCV build is much, much faster (counted in minutes instead of hours on my machine) and also the downloads on the fly are reduced to a very minimum (hope they go to zero)
There was also the request to move "pcl" to features package, but I need more info on this @patrikhuber