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

Add a png.lib file on windows #4

Closed
wants to merge 1 commit into from

Conversation

jankatins
Copy link

matplotlib expects the lib under that name, so add it as well. The file is also
in the package from the defaults channel, so this could also be considered a
regression against that version.

See matplotlib/matplotlib#6460 for the discussion on
that topic.

matplotlib expects the lib under that name, so add it as well. The file is also
in the package from the defaults channel, so this could also be considered a
regression against that version.

See matplotlib/matplotlib#6460 for the discussion on
that topic.
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jankatins
Copy link
Author

jankatins commented May 22, 2016

I'm currently building it locally to see if the file is included...

[Update: my local version had the file...]

@jankatins
Copy link
Author

Seems that everything built fine :-) and there were 4 files copied on appveyor, so this seems to be ok, too

@ocefpaf
Copy link
Member

ocefpaf commented May 22, 2016

@JanSchulz I am OK with this if there is no other way. However, even copying the versioned lib to an unversioned incarnation is weird. We should keep the lib as pristine as possible and fix the packages that use it and not the lib. After all there is a reason to why the lib is called libpng16 😉

There are other issues here though that is quite puzzling. Why the name libpng.lib requires [png] in matplotlib and renaming it to png.lib fixes the issue? Another issue is the fact that the lib is conflicting with itself and cannot be installed. (I am looking in the latter, but I have no idea how to fix the former.)

Note that there are many packages where we do rename the libs on Windows b/c those are imported from conda-recipes. That does not mean we should keep doing this!

@jankatins
Copy link
Author

jankatins commented May 22, 2016

Why the name libpng.lib requires [png] in matplotlib and renaming it to png.lib fixes the issue?

I think this is just a problem with fallbacks mpl implements for windows when pkgconfig is not there: Usually pkgconfig takes png and returns the current version of the lib (e.g. in this case libpng16), but without knowledge about the currently installed version, mpl can't infer the 16 part of the name. So I think someone started simply renaming the lib to png.lib.

Here are the names for the fallback defined and I guess that windows runs into this lines here which use the fallback defaults.

I think in this case matplotlib could be changed to at least add the lib part but not the 16 part. But this a) would break every manual setup which currently works :-( and b) I've no idea how many other python libs have this problem as well (= they expect png.lib and not libpng.lib or libpng16.lib).

This also seems to be not png specifc, as e.g. the zlib package also does export the lib under three different names...

A better way to fix it would probably be to make the pkgconfig scripts work so that the default fallbacks are not used?

This looks like it is a bigger problem under windows :-(

[Update: as it is "just" the lib versions which will be statically compiled in and not the dll version (which is only available as libpng16.dll), I don't think the problem is big...?]

@jankatins
Copy link
Author

jankatins commented May 22, 2016

Re the libpng package itself and linking to the right version: as long as the package is called libpng, this doesn't matter anyway, as when the lib would switch to libpng17.dll (=an ABI or even API incompatible change), the automatic upgrades of the package libpng would break any downstream package anyway (either because the package links against libpng16.dll which wouldn't be there anymore or because libpng.dll is incompatible[No libpng.dll, only the versioned one]) .

For more on this, see Debian Policy manual 8.1

The run-time shared library must be placed in a package whose name changes whenever the SONAME of the shared library changes. [...] Every time the shared library ABI changes in a way that may break binaries linked against older versions of the shared library, the SONAME of the library and the corresponding name for the binary package containing the runtime shared library should change. Normally, this means the SONAME should change any time an interface is removed from the shared library or the signature of an interface (the number of parameters or the types of parameters that it takes, for example) is changed. This practice is vital to allowing clean upgrades from older versions of the package and clean transitions between the old ABI and new ABI without having to upgrade every affected package simultaneously.

The SONAME and binary package name need not, and indeed normally should not, change if new interfaces are added but none are removed or changed, since this will not break binaries linked against the old shared library. Correct versioning of dependencies on the newer shared library by binaries that use the new interfaces is handled via the symbols or shlibs system.

So IMO the libpng package needs to be renamed to libpng16 (and similar every other native lib package...) and mpl somehow patched (or better changed upstream) to link against this version of the library (which would probably automatically happen if pkgconfig would work under windows?).

Right now, the whole (conda-forge) ecosystem will really break big time when one of the core libraries would change the ABI in an incompatible way and this is uploaded without anyone noticing: every installation would upgrade to it and suddenly all packages which link against that lib would stop working... In this case, the copy step during builds will actually prevent this disaster, as they make the build break (as there is also no libpng16.lib anymore) :-)

[update: for mpl, which takes the lib version and not the dll, it seems less problematic: if it compiles, all is good and if there is an incompatibility, it breaks on build time... Not sure which packages use a dll and not statically compile the lib versions in.]

@jankatins
Copy link
Author

jankatins commented May 31, 2016

Ok, I digged a bit deeper and it seems that there are two kinds of lib files: the "png.lib" which mpl expects is the static version (aka libpng16_static.lib) which includes all object code, but in this case it also takes the "library declaration file" (aka libpng16.lib) which is a kind of "description" of libpng16.dll -> conda-forge mpl is dynamically linked to libpng16.dll. So for me this looks a bit like dynamic linking works by accident :-/

I'm not sure what the fix is: on linux this all works because the default path calls pkgconfig and get the right flags for the name. On windows this seems to be not possible :-(

The problem I see is that libpng includes both the "header" files (libpng16.lib or even a png.lib) and the dynamic library libpng16.dll file. Building against the "header" part of libpng ends up as a fixed dependency on libpng16.dll, so theoretically just including png.lib gives no harm, as long as the binary dependency is on the package which contains libpng16.dll.

[mpltest_35] λ dumpbin.exe /DEPENDENTS _png.cp35-win_amd64.pyd
Dump of file _png.cp35-win_amd64.pyd

File Type: DLL

  Image has the following dependencies:

    libpng16.dll
    MSVCP140.dll
    python35.dll
    KERNEL32.dll
    VCRUNTIME140.dll
    api-ms-win-crt-stdio-l1-1-0.dll
    api-ms-win-crt-math-l1-1-0.dll
    api-ms-win-crt-runtime-l1-1-0.dll
    api-ms-win-crt-heap-l1-1-0.dll

Including "unversioned" "header" files (libpng.lib or png.lib) has the potential problem that it could happen that a new libpng version would increment the internal version (libpng16 -> libpng17) and this wouldn't be noticed on compile time: the dependent package (e.g. mpl) would end up with a dependency on libpng17.dll and would then fail big time if an older version of the dll would be installed. But on the other hand we already have that now if the lib includes new symbols (=#3).

So, my recommendation would be:

  • At least include a check on libpng16.dll -> So that a libpng16.dll does not slip in unnoticed
  • Copy libpng16.lib to png.lib -> to make it easier for downstream libs to link against libpng
  • Maybe rename libpng -> libpng16 -> Make a potential libpng17.dll coinstallable with libpng16.dll

(I still think this also points to some problems with conda, which has no distinction between source and binary packages, can't build multiple packages from a source package and has no automatic dependencies for binary packages. All of these would help to make this much easier to handle :-/)

CC: @tacaswell

@ocefpaf
Copy link
Member

ocefpaf commented Jun 29, 2016

@JanSchulz is this still valid? Or will this be fixed in matplotlib? (Sorry I missed this for a while and I just found it again).

@jankatins
Copy link
Author

Probably not. I currently copy the static file to the right place in the matplotlib build matplotlib/matplotlib@a5d8e7e and the conda-forge recipe patches setup.py

@jankatins jankatins closed this Jun 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants