Link wechat_qrcode with libiconv on MinGW #2931

ferdnyc · 2021-04-23T07:44:18Z

This PR is an alternate approach that fixes #2862, and partially reverts #2916 (with original submitter @berak's approval).

Instead of disabling wechat_qrcode's use of iconv in MinGW environments, the module simply needs to be linked with the external library libiconv, which is packaged and available as part of the MSYS2 collection.

(On POSIX systems iconv is built into libc, but on others it's provided by an external library.)

Fortunately, since CMake 3.11 the standard module FindIconv.cmake has been included in the distribution that can handle linking with libiconv if necessary on the target system.

The use of FindIconv.cmake is safe on POSIX systems; on my Fedora box it simply noted that iconv is built in to the C library and fell through harmlessly:

-- 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

Pull Request Readiness Checklist

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

I agree to contribute to the project under Apache 2 License.
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
The PR is proposed to proper branch
There is reference to original bug report and related work

N/A:

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

Iconv isn't automatic on all systems, non-POSIX have a separate libiconv that needs to be found in CMake and linked.

berak · 2021-04-23T08:48:45Z

unfortunately, it does not work "out-of-the-box" here ;(
(reversing the ifdef, and adding above to cmakelists.txt, rerun cmake) :

(from cmake-gui, configure)

i do have libiconv.a in C:\Program Files\mingw-w64\x86_64-8.1.0-posix-seh-rt_v6-rev0\mingw64\x86_64-w64-mingw32\lib
cmake is cmake-3.14.6-win32-x86

the module simply needs to be linked with the external library libiconv, which is packaged and available as part of the MSYS2 collection.

so, does it look for an "external package" ? any idea, how to "debug" it ?
and honestly, no idea about msys (not using that),
does your proposal only work with the msys distribution (instead of plain mingw-w64) ?

please also note, that the buildbots don't have any mingw installed, and thus won't check

ferdnyc · 2021-04-23T12:29:07Z

@berak

libiconv is mingw-w64-libiconv, packaged from https://www.gnu.org/software/libiconv/ and installed before building — binary packages are mingw-w64-{i686,x86_64,ucrt,clang}-libiconv which I guess would become a new dependency. It's not present in vanilla MinGW out of the box, no.

I suppose the question is, what to do if iconv is not available? We could just disable iconv in the module, as you'd previously been doing... since I guess it can be built without it (though what effect that has on functionality isn't clear to me)... or, we could disable the entire module in the build, as not being usable due to the missing dependency.

berak · 2021-04-23T13:24:09Z

and installed before building

what does one have to do, exactly ?

I guess would become a new dependency.

i don't like that idea (esp. given, that it works fine without)

It's not present in vanilla MinGW out of the box, no.

hmm, both 7.2.0 and 8.1.0 versions have it here,
the cmake script just did not find them (different location / package, i guess)
(kinda) working approach now:

run cmake configure (won't find the files)
specify them manually, run again.
cmake generate

but this is painful. we cannot expect noobs to dig 4 levels deep into their compiler installation to find those things
(i mean like : C:/Program Files/mingw-w64/x86_64-8.1.0-posix-seh-rt_v6-rev0/mingw64/x86_64-w64-mingw32/lib/libiconv.a - seriously ?)
(cmake also made a blunder with \\ seperators in the 1st attempt, another pitfall)

ferdnyc · 2021-04-23T22:56:20Z

@berak

Yeah, I agree the need to dig iconv out from that path is a bridge too far, when installed via the MSYS2 package libiconv.dll lives in C:\msys64\mingw64\bin\libiconv.dll and its import lib is C:\msys64\mingw64\lib\libiconv.dll.a, same as any other packaged DLL. (libiconv.a is also installed in ...\lib\.) I dunno what that file you're unearthing is, or where it comes from. 🤣

(cmake also made a blunder with \\ seperators in the 1st attempt, another pitfall)

That's interesting, actually — I wonder if that means it's finding that (weirdly buried) library using pkg-config? I never thought to check whether there was a .pc file for it, but that might explain the \\ paths.

...Huh, there is a C:\msys64\mingw64\lib\pkgconfig\iconv.pc installed with the MSYS2 package! But looking over FindIconv.cmake... it never tries pkg-config, So, that's not it.

I thought maybe there was some implicit automatic fallback to pkg-config, for CMake Find modules... but if there is, it's undocumented.

Well, all of this is sort of tangential. As you say, obviously we can't be expecting users to direct CMake to C:/Program Files/mingw-w64/x86_64-8.1.0-posix-seh-rt_v6-rev0/mingw64/x86_64-w64-mingw32/lib/libiconv.a just to build the wechat_qrcode module. And since you say it works fine without iconv (I've never tried, my sole goal was to get it to build so that it was no longer blocking me from submitting the 4.5.2 upgrade to MSYS) then I suppose the best thing to do is to just add a target_compile_definition to the CMakeLists.txt that sets ~~NO_ICONV_INSIDE~~ NO_ICONV, if iconv support can't be discovered during configuration.

I'll update the PR with that change.

ferdnyc · 2021-04-23T23:24:47Z

That should do it. Though, turns out I can't easily test that change... the MSYS2 cmake itself is linked with libiconv, and it breaks if I uninstall the DLL. 😆

berak · 2021-04-24T07:59:59Z

That should do it.

indeed, it does, all fine here :)
(it still does not find libiconv.a without manual override, but it sets NO_ICONV properly)

alalek

@ferdnyc Thank you for contribution 👍

@berak Thank you for reviewing!

sergiomb2 · 2022-10-19T13:45:15Z

Hello , to add this to opencv Fedora package, I need clarification on license of content on https://github.com/WeChatCV/opencv_3rdparty . These models are patent free ? and what license they have ?

Thank you

ferdnyc · 2022-10-27T18:12:11Z

@sergiomb2 Given that there's no license statement at that repo, I don't think that's a question anyone but @dddzg (the author of nearly all commits related to WeChat + OpenCV, and uploader of the model files) can answer.

The checked-in code is all licensed under Apache 2.0, so maybe the intent is that the models would be licensed the same way... but no way that's explicit enough to satisfy legal. Without a license file in that repo, it's pretty much in limbo. Even a clarification statement here would be unlikely to fly.

I suppose someone could submit a PR to WeChatCV/opencv_3rdparty, adding an Apache 2.0 license file to the repo, and see if it gets merged. That would presumably count as clarification/agreement.

Otherwise, perhaps the package can contain a download script the user can use to retrieve the models themselves? Has legal ever ruled on whether they'd be OK with that?

sergiomb2 · 2022-11-10T22:25:08Z

what user download in his computer, is not a concern .
The concern is distribute one content that might not be free (as freedom) on Fedora and ReahHat products .
I liked your ideia and will propose join the License File
Thank you

alalek · 2022-11-11T08:02:02Z

Licensing discussion should be in initial patch: #2821

Whole repo license for opencv_3rdparty is not possible due to different content (repo is a collection of 3rdparty dependencies or files or models).
License is applicable on "branches" only.

ferdnyc · 2022-11-15T13:02:05Z

@alalek Yeah, I don't see that there was much discussion there, either.

If the repo can't be licensed homogeneously, that's fine, different licenses can apply to different branches/directories/datasets. Heck, tools like reuse make it possible to have a different license for each individual file in a repo.

Reuse also requires that some license be explicitly documented for every file in a project's repo, which is one of its big advantages. Mystery-license repos like WeChatCV/opencv_3rdparty are minefields, and "everything doesn't have the same license" still isn't any kind of reason not to have clear licensing. I looked at a bunch of the branches there, and not a single one included a license file, OR any license statement in the README.

alalek · 2022-11-16T04:38:57Z

and not a single one included a license file, OR any license statement in the README.

It depends: https://github.com/opencv/opencv_3rdparty/tree/ffmpeg/4.x

Anyway, we should go to #2821 or create a new issue and ask @dddzg (contribution author) for clarification with model licenses.

ferdnyc added 2 commits April 23, 2021 03:32

wechat_qrcode: explicit iconv dependency

f279542

Iconv isn't automatic on all systems, non-POSIX have a separate libiconv that needs to be found in CMake and linked.

Don't disable iconv on MinGW

012b031

ferdnyc mentioned this pull request Apr 23, 2021

Update OpenCV to 4.5.2 release msys2/MINGW-packages#8457

Merged

wechat_qrcode: Disable iconv if not found

c33419d

mshabunin mentioned this pull request Apr 27, 2021

Release 4.5.2 conda-forge/opencv-feedstock#271

Merged

5 tasks

alalek approved these changes May 1, 2021

View reviewed changes

alalek merged commit 0d74c50 into opencv:master May 1, 2021

ferdnyc deleted the wechat-iconv-fix branch May 1, 2021 09:36

alalek mentioned this pull request Jun 4, 2021

(5.x) Merge 4.x #2966

Merged

lebarsfa mentioned this pull request Aug 6, 2022

Qt mingw32 compile error! opencv/opencv#20404

Closed

lwqwag mentioned this pull request Sep 20, 2022

Update CMakeLists.txt shimat/opencvsharp#1478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link wechat_qrcode with libiconv on MinGW #2931

Link wechat_qrcode with libiconv on MinGW #2931

ferdnyc commented Apr 23, 2021

berak commented Apr 23, 2021 •

edited

Loading

ferdnyc commented Apr 23, 2021 •

edited

Loading

berak commented Apr 23, 2021 •

edited

Loading

ferdnyc commented Apr 23, 2021 •

edited

Loading

ferdnyc commented Apr 23, 2021

berak commented Apr 24, 2021 •

edited

Loading

alalek left a comment

sergiomb2 commented Oct 19, 2022

ferdnyc commented Oct 27, 2022

sergiomb2 commented Nov 10, 2022

alalek commented Nov 11, 2022

ferdnyc commented Nov 15, 2022

alalek commented Nov 16, 2022

Link wechat_qrcode with libiconv on MinGW #2931

Link wechat_qrcode with libiconv on MinGW #2931

Conversation

ferdnyc commented Apr 23, 2021

Pull Request Readiness Checklist

N/A:

berak commented Apr 23, 2021 • edited Loading

ferdnyc commented Apr 23, 2021 • edited Loading

berak commented Apr 23, 2021 • edited Loading

ferdnyc commented Apr 23, 2021 • edited Loading

ferdnyc commented Apr 23, 2021

berak commented Apr 24, 2021 • edited Loading

alalek left a comment

Choose a reason for hiding this comment

sergiomb2 commented Oct 19, 2022

ferdnyc commented Oct 27, 2022

sergiomb2 commented Nov 10, 2022

alalek commented Nov 11, 2022

ferdnyc commented Nov 15, 2022

alalek commented Nov 16, 2022

berak commented Apr 23, 2021 •

edited

Loading

ferdnyc commented Apr 23, 2021 •

edited

Loading

berak commented Apr 23, 2021 •

edited

Loading

ferdnyc commented Apr 23, 2021 •

edited

Loading

berak commented Apr 24, 2021 •

edited

Loading