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

Fix building feature opencv halide #7581

Merged
merged 9 commits into from
Aug 17, 2019

Conversation

NiHoel
Copy link
Contributor

@NiHoel NiHoel commented Aug 7, 2019

No description provided.

@@ -82,6 +82,10 @@ Feature: eigen
Build-Depends: eigen3
Description: Eigen support for opencv

Feature: halide
Build-Depends: halide, opencv[dnn]
Copy link
Contributor

Choose a reason for hiding this comment

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

This will also pull in all the default opencv features, is that the intent?
If all you need is really just opencv[dnn] you should write opencv[core], opencv[dnn].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the default features are not required.

But changing line 86 to Build-Depends: halide, opencv[core], opencv[dnn] and running vcpkg install opencv[halide]:x64-windows printed:

The following packages will be built and installed:

  • eigen3[core]:x64-windows
  • halide[core]:x64-windows
  • libjpeg-turbo[core]:x64-windows
  • liblzma[core]:x64-windows
  • libpng[core]:x64-windows
    opencv[core,dnn,eigen,flann,halide,jpeg,opengl,png,tiff]:x64-windows
  • opengl[core]:x64-windows
  • protobuf[core]:x64-windows
  • tiff[core]:x64-windows
  • zlib[core]:x64-windows

The result is the same without changing line 86.
What is the effect of the suggested change?

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to write vcpkg install opencv[core,halide] on the command line to see the difference in behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even with Build-Depends: halide, opencv[dnn] and writing vcpkg install opencv[core,halide] it will only build opencv[core,dnn,halide] - no differences compared to Build-Depends: halide, opencv[core], opencv[dnn].

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a bug, caused by the fact that we don't track the set of requested features that are the result of the user writing them into the command-line and the result of them existing in a Build-Depends list separately.

We should still write out opencv[core], opencv[dnn].

@cbezault cbezault self-assigned this Aug 7, 2019
@cbezault
Copy link
Contributor

@NiHoel, all the work on this PR needs to be migrated to the opencv4 port. I'll do it if I have a second but it would be much appreciated if you could do it.

Default-Features: dnn, jpeg, opengl, png, tiff, webp

Feature: nonfree
Build-Depends: opencv4[nonfree]
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these features should Build-Depends on opencv4[core], opencv4[<feature-name>].

Copy link
Member

@vicroms vicroms Aug 14, 2019

Choose a reason for hiding this comment

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

I got a bit lost, why is that needed?

Requesting opencv4[core] from the meta-package will install all the default features anyway.

(All the following examples are after applying the requested change: Build-Depends: opencv4[core], opencv4[<feature-name>])

Meta-package with default features

./vcpkg install opencv
The following packages will be built and installed:
    opencv[core,dnn,jpeg,opengl,png,tiff,webp]:x64-windows
  * opencv4[core,dnn,jpeg,opengl,png,tiff,webp]:x64-windows

Meta-package without default features (core).

Requesting core from the meta-package disables the default-features on itself, however it cannot disable the default-features of the dependent opencv4.

PS D:\src\viromer\vcpkg> ./vcpkg install opencv[core]
The following packages will be built and installed:
    opencv[core]:x64-windows
  * opencv4[core,dnn,jpeg,opengl,png,tiff,webp]:x64-windows

Meta-package with extra feature

No difference between [feature] and [core,feature].

./vcpkg install opencv[halide]
The following packages will be built and installed:
    opencv[core,dnn,halide,jpeg,opengl,png,tiff,webp]:x64-windows
  * opencv4[core,dnn,halide,jpeg,opengl,png,tiff,webp]:x64-windows


./vcpkg install opencv[core,halide]
The following packages will be built and installed:
    opencv[core,halide]:x64-windows
  * opencv4[core,dnn,halide,jpeg,opengl,png,tiff,webp]:x64-windows

No meta-package

If you really want opencv4[core], then you have to skip the meta-package completely.

./vcpkg install opencv4
The following packages will be built and installed:
    opencv4[core,dnn,jpeg,opengl,png,tiff,webp]:x64-windows


./vcpkg install opencv4[core]
The following packages will be built and installed:
    opencv4[core]:x64-windows

Copy link
Contributor

@cbezault cbezault Aug 14, 2019

Choose a reason for hiding this comment

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

Yes it's true that requesting opencv4[core] from the build depends will give you the default features but that is a bug. The intention is that the package spec should mean the same thing everywhere. In this case there's a discrepancy in the behavior of the special feature core.

Note this is only true of dependencies on other ports. If you have a feature that depends on another feature within the port it mostly works. See #7472

@cbezault cbezault merged commit de3d6b5 into microsoft:master Aug 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants