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

Improve error checking with the CMake build when finding libpng #215

Closed
AndySomogyi opened this Issue Jul 23, 2017 · 3 comments

Comments

2 participants
@AndySomogyi

AndySomogyi commented Jul 23, 2017

If the runtime dynamically linked libpng is different than the libpng header files that Magnum was compiled with, then it will result is a rather nasty exception:

 Assertion file failed in .../src/MagnumPlugins/PngImporter/PngImporter.cpp on line 71

This happens if you have more that one version of libpng installed (surprisingly common, as many programs install libpng).

I think we could improve the CMake by having it check to see if the runtime linked libpng is in the same directory tree as the header directory. I think some version info checking would make the runtime error detection a lot nicer.

The png_libpng_ver libpng global holds the current version:
https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Desktop-generic/LSB-Desktop-generic/libpng12-png-libpng-ver.html

So, something like

if (strcmp (PNG_LIBPNG_VER_STRING, png_libpng_ver) != 0) {
Error() << "libpng version mismatch, expecting version " << PNG_LIBPNG_VER_STRING 
   << " but the runtime library is version: " << png_libpng_ver;
}
@mosra

This comment has been minimized.

Owner

mosra commented Jul 24, 2017

Yup, a very good idea. Could you submit a PR to the plugins repository with this, ideally for both PngImporter and PngImageConverter plugins?

Thank you a lot! :)

@AndySomogyi

This comment has been minimized.

AndySomogyi commented Jul 25, 2017

No problem, but will have to wait until next week.

@mosra

This comment has been minimized.

Owner

mosra commented Aug 14, 2017

Done in mosra/magnum-plugins@ecc9c0b. In the end I decided to use an assert as well (it points out to a broken installation which can't really be solved from within the app at runtime), but this time it at least gives a helpful message. Thanks for the initial patch!

@mosra mosra closed this Aug 14, 2017

@mosra mosra self-assigned this Aug 14, 2017

@mosra mosra added the improvement label Aug 14, 2017

@mosra mosra added this to Done in Asset management Aug 14, 2017

@mosra mosra added this to the 2018.02 milestone Feb 15, 2018

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