Skip to content

wechat_qr: disable iconv dependancy for mingw #2916

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

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

berak
Copy link
Contributor

@berak berak commented Apr 7, 2021

resolves #2862

this also collapses all #if defined(XXX) blocks to disable iconv support into a single one

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • [x ] I agree to contribute to the project under Apache 2 License.
  • [x ] To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • [x ] The PR is proposed to proper branch
  • [x ] There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@berak
Copy link
Contributor Author

berak commented Apr 7, 2021

uff, how can OpenCV CN Ubuntu 18.04 x86-64 fail on goturn tracking tests using OpenCV(3.4.14-pre) ? pr was against master branch ...

Copy link
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Thank you!

@opencv-pushbot opencv-pushbot merged commit dc3cd0e into opencv:master Apr 7, 2021
@berak berak deleted the wechat_qr_fix_iconv_mingw branch April 7, 2021 18:38
@alalek alalek mentioned this pull request Apr 9, 2021
@ferdnyc
Copy link
Contributor

ferdnyc commented Apr 23, 2021

Hmm. I was just about to open a PR to enable iconv in wechat_qrcode, since in my testing it builds fine if linked with mingw-w64-{x86_64,i686}-libiconv. And CMake has, ever since 3.11, included a FindIconv.cmake in the standard distribution Modules collection.

Adding this block near the top of wechat_qrcode/CMakeLists.txt succeeded for me:

# iconv support isn't automatic on some systems
if(CMAKE_VERSION VERSION_GREATER 3.11)
  find_package(Iconv QUIET)
  if(Iconv_FOUND)
    ocv_target_link_libraries(${the_module} Iconv::Iconv)
  endif()
endif()

It's also safe on POSIX, I built that tree under Fedora and you just see this go by in the CMake output:

-- Performing Test Iconv_IS_BUILT_IN
-- Performing Test Iconv_IS_BUILT_IN - Success
-- wechat_qrcode: Download: detect.caffemodel
-- wechat_qrcode: Download: detect.prototxt
-- wechat_qrcode: Download: sr.caffemodel
-- wechat_qrcode: Download: sr.prototxt

...Can you comment on why you settled on disabling iconv instead, @berak ?

@berak
Copy link
Contributor Author

berak commented Apr 23, 2021

@ferdnyc ofc, linking iconv properly is much better than disabling it ;)

see previous discussion here

(tbh, i stopped fiddling with cmake, when dddgz proposed disabling it)

@ferdnyc
Copy link
Contributor

ferdnyc commented Apr 23, 2021

@berak Perfect, that's the answer I was hoping for! I'll go ahead and open a new PR to link libiconv in to wechat_qrcode, and let the downstream MSYS2 packaging maintainers know that the update I already submitted is the way forward, then. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

errors on building opencv with MinGW on windows without VisualStudio
4 participants