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

[opencv] Fix import paths for debug Android builds #24394

Merged
merged 3 commits into from
May 5, 2022

Conversation

hh10k
Copy link
Contributor

@hh10k hh10k commented Apr 25, 2022

  • What does your PR fix?

Fixes OpenCV 4 import paths for Android debug builds.

For Android builds OpenCV places library outputs under a sdk/native/staticlibs/<arch> directory, and vcpkg correctly copies these to the install directory as either vcpkg_installed/<triplet>/sdk/... for release or vcpkg_installed/<triplet>/debug/sdk/... for debug, however the OpenCVModules-debug.cmake is not automatically updated by vcpkg_cmake_config_fixup to reference the debug directory.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

OpenCV will only use this sdk directory when the ANDROID cmake variable is set. I'm not aware of behaviour change based on the triplet.

I have read the guide and I think this PR follows it.

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

@ghost
Copy link

ghost commented Apr 25, 2022

CLA assistant check
All CLA requirements met.

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. labels Apr 25, 2022
@JackBoosY
Copy link
Contributor

Please click "ready for review" then I will review this PR.

@hh10k hh10k marked this pull request as ready for review April 25, 2022 11:29
@hh10k hh10k changed the title [opencv4] Fix import paths for debug Android builds [opencv] Fix import paths for debug Android builds Apr 29, 2022
@talregev
Copy link
Contributor

@hh10k Is it ready for review?

@hh10k
Copy link
Contributor Author

hh10k commented Apr 30, 2022

Yes, this is ready for review. It includes the fix for OpenCV 3 and 4, and all the changes from ./vcpkg x-add-version --all have been committed.

@talregev
Copy link
Contributor

Yes, this is ready for review. It includes the fix for OpenCV 3 and 4, and all the changes from ./vcpkg x-add-version --all have been committed.

@JackBoosY FYI

@cenit
Copy link
Contributor

cenit commented Apr 30, 2022

just noticed one thing:
the PR is not ready for single-config builds

please try to test your pr using the x64-windows-release community triplet and check if it works

i suppose you have to wrap your fix within the clause if (NOT VCPKG_BUILD_TYPE)

@hh10k
Copy link
Contributor Author

hh10k commented May 1, 2022

@cenit @JackBoosY I tested with the x64-osx-release triplet and there was a problem, so I added if (NOT VCPKG_BUILD_TYPE OR VCPKG_BUILD_TYPE STREQUAL "debug") to both portfiles. That is commonly used elsewhere in other ports.

I couldn't confirm whether a debug single configuration build would actually work though, because some dependency builds are broken.

@cenit
Copy link
Contributor

cenit commented May 1, 2022

single config debug triplets cannot work by design in vcpkg so that's not a problem

about the fix, a simpler clause like the one i told you was similarly effective, but yours is ok too

@adapptor-kurt
Copy link

single config debug triplets cannot work by design in vcpkg so that's not a problem

about the fix, a simpler clause like the one i told you was similarly effective, but yours is ok too

Ah, sorry for making it more complicated than necessary. I saw 164 instances of VCPKG_BUILD_TYPE STREQUAL "debug" in the codebase so assumed it was also possible.

@cenit
Copy link
Contributor

cenit commented May 2, 2022

until I asked and insisted with a test PR in #16110, the stance about the broken debug-only triplets was not clear. That's why in many places we were still hoping about that possibility

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels May 5, 2022
@JackBoosY
Copy link
Contributor

LGTM.

@BillyONeal
Copy link
Member

until I asked and insisted with a test PR in #16110, the stance about the broken debug-only triplets was not clear. That's why in many places we were still hoping about that possibility

Maybe we should add something like VCPKG_BUILD_INCLUDES_DEBUG or similar and replace all occurrences?

@BillyONeal BillyONeal merged commit fd6a336 into microsoft:master May 5, 2022
@BillyONeal
Copy link
Member

Thanks :)

@cenit
Copy link
Contributor

cenit commented May 5, 2022

until I asked and insisted with a test PR in #16110, the stance about the broken debug-only triplets was not clear. That's why in many places we were still hoping about that possibility

Maybe we should add something like VCPKG_BUILD_INCLUDES_DEBUG or similar and replace all occurrences?

just the first part is ok

if (NOT VCPKG_BUILD_TYPE)

without the OR VCPKG_BUILD_TYPE STREQUAL "debug" which is an impossible case

which also means that somehow the only valid value for the build type is "release", but since release could be overridden to have debug, it has lost its meaning. It should just be spelled as "vcpkg_single_config_build"...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. 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.

7 participants