Build against system deps, Python test fixes #250
Conversation
|
Thanks a lot for the work. I have 2 questions, though:
|
|
The module builds and works in Python 2 so I thought I would throw in
fixing the Python tests on Python 2 for free :)
Using the system provided libraries by default would be good, I guess I
incorrectly assumed you would prefer FetchContent be the default. Also,
an option to fully enable or disable FetchContent might be useful in
some cases, so I might keep the option but make it auto/no/yes.
BTW, I note that the copy of stb is modified and replacing it with the
unmodified one causes the tests to fail. I'm wondering if there is any
easy way around that, I'd like to make the system stb used too.
…--
bye,
pabs
https://bonedaddy.net/pabs3/
|
|
I have updated the patch to use system deps by default and tested all
three of the AUTO, ALWAYS, NEVER values for the option.
…--
bye,
pabs
https://bonedaddy.net/pabs3/
|
|
ad Python 2: I prefer not to add 'cruft' just to supports deprecated software. (If it was for me, we'd be at c++20 already ;)) ad FetchContent: Yes, the auto/no/yes approach is obviously the most flexible. Default should be auto. ad stb: There is a 2 line change that allows to use the upstream version but that comes at a considerable runtime cost for the blackbox tests (30% slower). There should also be a bit more elaborate change which should take care of that. I agree that treating stb exactly like the other 3 external dependencies would be preferable and cleaner. I'll look into the required changes and then either drop the local copy of stb myself or let you know about it ;). |
Some Python interpreter distributions are versioned, so the right version has to be used or the test will fail.
|
I could not find an easy way to prevent pybind11 building with Python 2
so I have just dropped that patch.
The auto/yes/no approach is implemented, although I went for AUTO,
ALWAYS, NEVER instead with AUTO being the default.
Thanks for the info and help on stb, much appreciated.
…--
bye,
pabs
https://bonedaddy.net/pabs3/
|
|
Something went wrong with the default value, it seems. Could you have a look at the build log failure? I'm off for the weekend now. Will come back to this on Monday. Thanks! |
Make the default be to use system versions when available, but allow always using them or never using them. Add searching for and using system versions of fmt/googletest/pybind11, which are currently pulled directly from git using FetchContent. This will allow distributions that do not allow network access at build time to depend on and build against these packages. Fixes: zxing-cpp#248
|
I made the mistake of assuming that cmake options are strings, but it
seems they are boolean only. It appears the way to do a string option
is set() instead of option(). I have updated the PR to do that. Another
option would be to have two boolean variables for system and fetch. Let
me know if you prefer that and I will update the patch to do that.
…--
bye,
pabs
https://bonedaddy.net/pabs3/
|
|
@axxel did you get a chance to look into using an unpatched stb version? |
…tive This removes the need to use a ZXing specific version of stb_image.h. The provided alternative for the Blackbox tests is now almost as fast as before. See also #250.
|
I totally forgot about it... If you feel like it, please feel free to look into the possibility to treat stb_image similar to the other external dependencies. I.e. fetch it directly from github in case it is not provided by the system libraries. Then we can completely remove the copy from the repo, I believe. |
|
Hmm, I thought the upstream version caused a big slowdown in the tests,
which was the reason for keeping the modified copy? You mentioned above
a more elaborate change which should fix that though?
The Debian package of libstb-dev contains all the stb headers used, so
once the slowdown issue is fixed, the same approach as now used for the
other dependencies should work for stb too.
A cmake patch for switching from the modified copy to the upstream and
system stb would be fairly easy, but it should probably only get merged
after fixing the slowdown.
Perhaps the right way forward would be to keep the patch modifying stb,
but delete the embedded stb copy, then auto-download stb from GitHub
and when the download happens, apply the patch modifying stb before
building against the modified stb. Then when using the system
libraries, have a separate option to build against the modified stb,
for the people who mostly want system libs but still want the speedup.
I've implemented this approach and am filing a new pull request.
…--
bye,
pabs
https://bonedaddy.net/pabs3/
|
Add mode to build against system versions of dependencies.
Use right Python interpreter, port Python tests to Python 2.