sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.41k stars 475 forks source link

generate libpng.pc, zlib.pc if needed #28906

Open dimpase opened 4 years ago

dimpase commented 4 years ago

libpng.pc, gdlib.pc, zlib.pcetc are needed in the calls of pkgconfig.parse in src/module_list.py

some perfectly fine implementations of zlib etc. don't install zlib.pc etc. (e.g. MacOS does not).

Since #26286 this problem was sleeping, until we updated pkgconfig on #28883, which made it throw an error on absense of zlib.pc.

This ticket provides generation of zlib.pc and libpng.pc, among with some fly-by refactoring.

CC: @kiwifb @embray @isuruf @mkoeppe

Component: build: configure

Author: Dima Pasechnik

Branch/Commit: u/dimpase/packages/missingpcs @ ba7d963

Issue created by migration from https://trac.sagemath.org/ticket/28906

dimpase commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,11 @@
-some perfectly fine implementations of zlib don't install zlib.pc (e.g. MacOS does not).
+`libpng.pc, gdlib.pc, zlib.pc`
+are needed in the calls of `pkgconfig.parse` in `src/module_list.py`
+
+
+some perfectly fine implementations of `zlib` etc. don't install `zlib.pc` etc. (e.g. MacOS does not).

 Since #26286 this problem was sleeping, until we updated `pkgconfig` on #28883, which made it throw an error on absense of `zlib.pc`.

+
+
dimpase commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-`libpng.pc, gdlib.pc, zlib.pc`
+`libpng.pc, gdlib.pc, zlib.pc`etc
 are needed in the calls of `pkgconfig.parse` in `src/module_list.py`

@@ -6,6 +6,7 @@

 Since #26286 this problem was sleeping, until we updated `pkgconfig` on #28883, which made it throw an error on absense of `zlib.pc`.

+This ticket provides generation of `zlib.pc` and `libpng.pc`, among with some fly-by refactoring.
dimpase commented 4 years ago
comment:3

The biggest question I here is how to find the full path to libz located by AC_CHECK_LIB or other similar macro. This path has to be written into zlib.pc.

I don't quite know: I can link an executable with libz and analyse it with readelf or otool or a similar thing, but it's not platform independent, and I could not find a ready-made macro (I could change AC_CHECK_LIB, sure...)

dimpase commented 4 years ago

Commit: 73d0619

dimpase commented 4 years ago

Author: Dima Pasechnik

dimpase commented 4 years ago

Branch: u/dimpase/packages/missingpcs

dimpase commented 4 years ago

New commits:

ad511a1simplify some spkg-configures (use new macro)
73d0619refactor zlib's spkg-configure, start on a macro for zlib.pc
isuruf commented 4 years ago
comment:5

You can use a minimal pkgconfig file like below which doesn't require the full path to the library.

Name: Foo
Description: description for foo
Version: 1.0.0
Libs: -lfoo
dimpase commented 4 years ago
comment:6

well, a minimal pkgconfig like this would be just a bug in waiting to bite.

I can think of one platform-independent way (well, it needs dladdr, which is non-POSIX, but BSD, something that is available on most Unices, certainly on Linux and MacOS)

Write a C function that calls dlopen() on the library in question, and then calls dladdr() on a symbol from the library; one of parts of the returned data is the full path to the shared object where the symbol is located. That's a hell of a macro call to AC_RUN_IFELSE though :-)

dimpase commented 4 years ago
comment:8

Could someone please try if the following works on MacOS, and if yes, what's the output

/* save into zt.c and run as follows:
  $ cc -o zt zt.c -ldl && ./zt

  On my Debian Linux box it outputs
  /usr/lib/x86_64-linux-gnu/libz.so
*/
#include <stdio.h>
#define __USE_GNU
#include <dlfcn.h>
#ifdef __APPLE__
#define EXT "dylib"
#else
#define EXT "so"
#endif
int main() {
void *z;
Dl_info  info;
int res;
void *ha;
z = dlopen("libz."EXT,  RTLD_LAZY); /* load zlib */
ha = dlsym(z, "inflateEnd");       /* get address of inflateEnd in libz */
res = dladdr(ha, &info);           /* get info for the function */
printf("%s\n", info.dli_fname);
return res;
}
kiwifb commented 4 years ago
comment:9

Replying to @dimpase:

Could someone please try if the following works on MacOS, and if yes, what's the output

fbissey@itsv-frb15 GitHub % uname -a
Darwin itsv-frb15.lan 19.2.0 Darwin Kernel Version 19.2.0: Sat Nov  9 03:47:04 PST 2019; root:xnu-6153.61.1~20/RELEASE_X86_64 x86_64
fbissey@itsv-frb15 GitHub % cc -o zt zt.c -ldl && ./zt
/usr/lib/libz.1.dylib
isuruf commented 4 years ago
comment:10

I suggest linking the executable zt to libz so that the correct shared library is loaded.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

6bb85c4implemented generating input data for zlib.pc
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 73d0619 to 6bb85c4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

80a23acimplemented generating zlib.pc
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 6bb85c4 to 80a23ac

dimpase commented 4 years ago
comment:13

This branch is so far only for zlib.pc

Any obvious things I missed?

Naturally, I'd appreciate it tested on MacOS.

For libpng.pc I'll probably refactor that, to make computing abstolute path to libpng/libz/libWhatever a macro.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

2d4a1cdrefactor finding absolute path to dylib into macro
ba7d963do not try to recongnise libpng without pkg-config
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 4 years ago

Changed commit from 80a23ac to ba7d963

dimpase commented 4 years ago
comment:15

this is a result - ugly as hell. I am not sure whether this should get in.

embray commented 4 years ago
comment:17

Replying to @dimpase:

well, a minimal pkgconfig like this would be just a bug in waiting to bite.

Would it though? Why exactly do we require pkgconfig here at all? It's a convenience sure, but on systems that don't install .pc files for some libraries we can still get the required information another way...

I mean, if autoconf could detect a system zlib in the first place, then I don't see why would need the absolute path to the SO file itself. All that should be needed to link against that library are whatever compiler/linker flags (e.g. -lz) that autoconf used to detect usability of the library in the first place.

dimpase commented 4 years ago
comment:18

Replying to @embray:

Replying to @dimpase:

well, a minimal pkgconfig like this would be just a bug in waiting to bite.

Would it though? Why exactly do we require pkgconfig here at all? It's a convenience sure, but on systems that don't install .pc files for some libraries we can still get the required information another way...

I am thinking of a library trying to detect zlib by calling pkg-config rather than using autoconf.

IMHO it's better not to install a pc file at all rather than install a rather bogus one. That's why I prefer how it's done on #28883.

I mean, if autoconf could detect a system zlib in the first place, then I don't see why would need the absolute path to the SO file itself. All that should be needed to link against that library are whatever compiler/linker flags (e.g. -lz) that autoconf used to detect usability of the library in the first place.

embray commented 4 years ago
comment:19

It's not necessarily "bogus". But I agree--I think what you did in #28883 is more in line with what I was trying to say here.

isuruf commented 4 years ago
comment:20

I don't see the difference in #28883 and a minimal .pc file as in #28906 comment:5 . They do the same thing.

dimpase commented 4 years ago
comment:21

Replying to @isuruf:

I don't see the difference in #28883 and a minimal .pc file as in #28906 comment:5 . They do the same thing.

yes, but they do so by different means, and a pc file is much more likely to trip up someone.

mkoeppe commented 4 years ago
comment:22

I don't know if this ticket is still needed; but if it is, then it should be revised along the lines of #29082 -- do not write into SAGE_LOCAL at configure time; instead generate a rule in build/make/Makefile that generates the pc file there.

dimpase commented 4 years ago
comment:23

I don't recall right now all the details, why I thought it's needed. Anyhow, perhaps as a concept of how to generate *.pc files, e.g. for openblas.