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

[opencv4] Fix build failure when dnn feature is not enabled #38594

Merged
merged 1 commit into from
May 8, 2024

Conversation

WentsingNee
Copy link
Contributor

@WentsingNee WentsingNee commented May 6, 2024

Given the following manifest (when dnn feature is not included):

"dependencies": [
  {
    "name": "opencv4",
    "version>=": "4.8.0#18",
    "default-features": false,
    "features": [
      "png"
    ]
  }
]

it failed and here is what vcpkg/buildtrees/opencv4/config-x64-linux-out.log said:

...
CMake Error at /home/peter/open-source/git/vcpkg/scripts/buildsystems/vcpkg.cmake:859 (_find_package):
  Could not find a package configuration file provided by "flatbuffers" with
  any of the following names:

    flatbuffersConfig.cmake
    flatbuffers-config.cmake

  Add the installation prefix of "flatbuffers" to CMAKE_PREFIX_PATH or set
  "flatbuffers_DIR" to a directory containing one of the above files.  If
  "flatbuffers" provides a separate development package or SDK, be sure it
  has been installed.
Call Stack (most recent call first):
  CMakeLists.txt:759 (find_package)
...

This problem is introduced by commit 6d2c9714 contributed by @jimwang118

In the patch vcpkg/ports/opencv4/0023-fix-no-flatbuffers.patch, Jim disabled the procedure of Flatbuffers detection introduced by the previous patch 0017-fix-flatbuffers.patch, which has correctly fixed the flatbuffers-related issues.

...
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -756,7 +756,7 @@ include(cmake/OpenCVFindLibsVideo.cmake)
 include(cmake/OpenCVFindLibsPerf.cmake)
 include(cmake/OpenCVFindLAPACK.cmake)
 include(cmake/OpenCVFindProtobuf.cmake)
-include(cmake/OpenCVDetectFlatbuffers.cmake)
+find_package(flatbuffers CONFIG REQUIRED)
...

In vcpkg/ports/opencv4/vcpkg.json we can see, flatbuffers is only the dependency of feature dnn:

  "dependencies": [
...
    "dnn": {
      "description": "Enable dnn module",
      "dependencies": [
        "flatbuffers",
        {
          "name": "flatbuffers",
          "host": true,
          "default-features": false
        },
        "protobuf"
      ]
    },
...

and when dnn is not enabled, it broke down at the line find_package(flatbuffers CONFIG REQUIRED)

And this change is unnecessary either:

-  list(APPEND libs ocv.3rdparty.flatbuffers)
+  list(APPEND libs flatbuffers::flatbuffers)

Thus, we'd better to revert those changes.

@WentsingNee WentsingNee marked this pull request as ready for review May 6, 2024 16:38
@jimwang118 jimwang118 added the category:port-bug The issue is with a library, which is something the port should already support label May 7, 2024
@WentsingNee
Copy link
Contributor Author

I tested it locally without installing the dnn feature, and the error you mentioned did not occur. Installation command: vcpkg install opencv4[core,png] result: image

port version 15 is no problem. Bug is introduced since port version 16 you committed
image

image

@jimwang118
Copy link
Contributor

I suggest adding constraints to the patch that determine when using dnn to find flatbuffers through the patch. Removing the patch may trigger the original CI error.
...
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -756,7 +756,7 @@ include(cmake/OpenCVFindLibsVideo.cmake)
include(cmake/OpenCVFindLibsPerf.cmake)
include(cmake/OpenCVFindLAPACK.cmake)
include(cmake/OpenCVFindProtobuf.cmake)
-include(cmake/OpenCVDetectFlatbuffers.cmake)
if(BUILD_opencv_dnn)
+find_package(flatbuffers CONFIG REQUIRED)
endif()
...

@WentsingNee
Copy link
Contributor Author

But I don't think that it's a good way to use find_package(flatbuffers) instead of include(cmake/OpenCVDetectFlatbuffers.cmake) because their script is better to handle with some corner cases.

Eg.

if(BUILD_opencv_dnn)

We shouldn't assume that dnn feature depends on flatbuffers or there is ONLY dnn feature depends on flatbuffers (i.e. if in the future, dnn is not depend on flatbuffers anymore or there is an another feature depends on flatbuffers), it will trigger errors again.

@WentsingNee
Copy link
Contributor Author

May I have ask why CI failed if we remove the patch? Does it related to something like caches?

@WentsingNee
Copy link
Contributor Author

image

I' ve seen that there are some commits removed the patches. It doesn't seem that vcpkg project disallow to remove port's patches

@WentsingNee
Copy link
Contributor Author

And your patches is the last one applied, remove it will not cause other patches applied failed

@jimwang118
Copy link
Contributor

And your patches is the last one applied, remove it will not cause other patches applied failed

Ok, I will test it locally and if there are no problems, I will mark it as reviewed.

@jimwang118
Copy link
Contributor

Compile test pass with following triplets:

x64-windows
x64-windows-static

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label May 7, 2024
@data-queue data-queue merged commit 44fb94f into microsoft:master May 8, 2024
17 checks passed
yurybura pushed a commit to yurybura/vcpkg that referenced this pull request May 8, 2024
…t#38594)

Given the following manifest (when `dnn` feature is not included):

```json
"dependencies": [
  {
    "name": "opencv4",
    "version>=": "4.8.0#18",
    "default-features": false,
    "features": [
      "png"
    ]
  }
]
```

it failed and here is what
`vcpkg/buildtrees/opencv4/config-x64-linux-out.log` said:

```
...
CMake Error at /home/peter/open-source/git/vcpkg/scripts/buildsystems/vcpkg.cmake:859 (_find_package):
  Could not find a package configuration file provided by "flatbuffers" with
  any of the following names:

    flatbuffersConfig.cmake
    flatbuffers-config.cmake

  Add the installation prefix of "flatbuffers" to CMAKE_PREFIX_PATH or set
  "flatbuffers_DIR" to a directory containing one of the above files.  If
  "flatbuffers" provides a separate development package or SDK, be sure it
  has been installed.
Call Stack (most recent call first):
  CMakeLists.txt:759 (find_package)
...
```

This problem is introduced by [commit
6d2c971](microsoft@6d2c971)
contributed by @jimwang118

In the patch `vcpkg/ports/opencv4/0023-fix-no-flatbuffers.patch`, Jim
disabled the procedure of `Flatbuffers` detection introduced by the
previous patch `0017-fix-flatbuffers.patch`, which has correctly fixed
the `flatbuffers`-related issues.

```patch
...
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -756,7 +756,7 @@ include(cmake/OpenCVFindLibsVideo.cmake)
 include(cmake/OpenCVFindLibsPerf.cmake)
 include(cmake/OpenCVFindLAPACK.cmake)
 include(cmake/OpenCVFindProtobuf.cmake)
-include(cmake/OpenCVDetectFlatbuffers.cmake)
+find_package(flatbuffers CONFIG REQUIRED)
...
```

In `vcpkg/ports/opencv4/vcpkg.json` we can see, `flatbuffers` is only
the dependency of feature `dnn`:

```json
  "dependencies": [
...
    "dnn": {
      "description": "Enable dnn module",
      "dependencies": [
        "flatbuffers",
        {
          "name": "flatbuffers",
          "host": true,
          "default-features": false
        },
        "protobuf"
      ]
    },
...
```

and when `dnn` is not enabled, it broke down at the line
`find_package(flatbuffers CONFIG REQUIRED)`

And this change is unnecessary either:

```patch
-  list(APPEND libs ocv.3rdparty.flatbuffers)
+  list(APPEND libs flatbuffers::flatbuffers)
```

Thus, we'd better to revert those changes.

Co-authored-by: WentsingNee <8090395+wentsingnee@user.noreply.gitee.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants