sagemath/sage

update pkgconfig to version 1.5.1

dimpase opened this issue · 67 comments

the current version is buggy, as we found out on #27870

also, pkgconfig switched to https://python-poetry.org/ from setuptools, so we either get poetry installed too (which is a big task), or go Debian way and provide setup.py. That's what they do in their (not really "orig") tarball on

http://deb.debian.org/debian/pool/main/p/python-pkgconfig/

Cf. https://salsa.debian.org/python-team/modules/python-pkgconfig/tree/master

The pypi tarball has setup.py, so we can just use it.
tarball: https://files.pythonhosted.org/packages/6e/a9/ff67ef67217dfdf2aca847685fe789f82b931a6957a3deac861297585db6/pkgconfig-1.5.1.tar.gz

CC: @kiwifb @embray @tobihan @infinity0 @isuruf @mkoeppe

Component: packages: standard

Author: Dima Pasechnik

Branch: fb8f4e5

Reviewer: Isuru Fernando, Erik Bray, Matthias Koeppe

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

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 the current version is buggy, as we found out on #27870
+
+also, pkgconfig switched to https://python-poetry.org/ from setuptools, so we need poetry installed too.
comment:2

I found adding setup.py into the "orig" tarball rather than into debian/patches rather unusual.

Description changed:

--- 
+++ 
@@ -1,3 +1,7 @@
 the current version is buggy, as we found out on #27870
 
-also, pkgconfig switched to https://python-poetry.org/ from setuptools, so we need poetry installed too.
+also, pkgconfig switched to https://python-poetry.org/ from setuptools, so we either get poetry installed too (which is a big task), or go Debian way and provide `setup.py`. That's what they do in their (not really "orig") tarball:
+
+http://deb.debian.org/debian/pool/main/p/python-pkgconfig/python-pkgconfig_1.5.1.orig.tar.gz
+
+as can be seen on https://salsa.debian.org/python-team/modules/python-pkgconfig/tree/master

Commit: e5c91db

Author: Dima Pasechnik

Description changed:

--- 
+++ 
@@ -1,7 +1,12 @@
 the current version is buggy, as we found out on #27870
 
-also, pkgconfig switched to https://python-poetry.org/ from setuptools, so we either get poetry installed too (which is a big task), or go Debian way and provide `setup.py`. That's what they do in their (not really "orig") tarball:
+also, pkgconfig switched to https://python-poetry.org/ from setuptools, so we either get poetry installed too (which is a big task), or go Debian way and provide `setup.py`. That's what they do in their (not really "orig") tarball on
 
-http://deb.debian.org/debian/pool/main/p/python-pkgconfig/python-pkgconfig_1.5.1.orig.tar.gz
+http://deb.debian.org/debian/pool/main/p/python-pkgconfig/
 
-as can be seen on https://salsa.debian.org/python-team/modules/python-pkgconfig/tree/master
+Cf. https://salsa.debian.org/python-team/modules/python-pkgconfig/tree/master
+
+The pypi tarball has setup.py, so we can just use it.
+tarball: https://files.pythonhosted.org/packages/6e/a9/ff67ef67217dfdf2aca847685fe789f82b931a6957a3deac861297585db6/pkgconfig-1.5.1.tar.gz
+
+
comment:4

That poetry stuff is really a replacement for pip. And from where I see it, it can only fully replace superficially simple setup.py files. And it probably can generate such simple setup.py file, although I cannot see that in the Readme. And to be able to do what it does, it must be able to process complicated setup.py just fine, since it will install stuff that was never prepared for poetry if I am not mistaken.

Anyway that's outside of the scope of the ticket. But one more of this things to potentially deal with :(

comment:5

I guess poetry generates setup.py files as a part of its pypi deployment feature.

comment:6

[ A stupidity. Sorry for the noise ... ]

comment:7

Replying to @EmmanuelCharpentier:

[ A stupidity. Sorry for the noise ... ]

[ After re-checking the setup... ] This doesn't seem to be effective. I still get the systemwide pkg-config:

charpent@zen-book-flip:/usr/local/sage-9$ git status
Sur la branche t/28883/packages/pkgconfig151
Votre branche est à jour avec 'trac/u/dimpase/packages/pkgconfig151'.

rien à valider, la copie de travail est propre
charpent@zen-book-flip:/usr/local/sage-9$ ./sage -sh

Starting subshell with Sage environment variables set.  Don't forget
to exit when you are done.  Beware:
 * Do not do anything with other copies of Sage on your system.
 * Do not use this for installing Sage packages using "sage -i" or for
   running "make" at Sage's root directory.  These should be done
   outside the Sage shell.

Bypassing shell configuration files...

Note: SAGE_ROOT=/usr/local/sage-9
(sage-sh) charpent@zen-book-flip:sage-9$ command -v pkg-config
/usr/bin/pkg-config
(sage-sh) charpent@zen-book-flip:sage-9$ pkg-config --version
0.29
(sage-sh) charpent@zen-book-flip:sage-9$ exit
exit
Exited Sage subshell.
comment:8

this is a Python interface to pkg-config.
So you need to import pkgconfig in Sage's Python to see it in action.

Reviewer: Isuru Fernando

Changed commit from e5c91db to none

comment:13

This breaks on OSX:

[sagelib-9.0.beta9] Traceback (most recent call last):
[sagelib-9.0.beta9]   File "setup.py", line 72, in <module>
[sagelib-9.0.beta9]     from module_list import ext_modules, library_order
[sagelib-9.0.beta9]   File "/Users/buildbot-sage/slave/sage_git/build/src/module_list.py", line 42, in <module>
[sagelib-9.0.beta9]     zlib_pc = pkgconfig.parse('zlib')
[sagelib-9.0.beta9]   File "/Users/buildbot-sage/slave/sage_git/build/local/lib/python3.7/site-packages/pkgconfig/pkgconfig.py", line 248, in parse
[sagelib-9.0.beta9]     _raise_if_not_exists(package)
[sagelib-9.0.beta9]   File "/Users/buildbot-sage/slave/sage_git/build/local/lib/python3.7/site-packages/pkgconfig/pkgconfig.py", line 103, in _raise_if_not_exists
[sagelib-9.0.beta9]     raise PackageNotFoundError(package)
[sagelib-9.0.beta9] pkgconfig.pkgconfig.PackageNotFoundError: zlib not found
[sagelib-9.0.beta9] ************************************************************************
[sagelib-9.0.beta9] Error building the Sage library
[sagelib-9.0.beta9] ************************************************************************
comment:14

probably buildbot had a workaround for the buggy package?

comment:15

isn't the !Sage/Python environment of that buildbot broken?
One should have

sage: import pkgconfig
sage: pkgconfig.parse('zlib')
defaultdict(<class 'list'>, {'libraries': ['z']})
comment:16

It works for me on OS X 10.15.2.

comment:17

Turns out #27870 is the culprit...

comment:19

Oh just noticed that this ticket is merged into #27870

comment:20

To me, it looks like a Python with broken zlib module. Nothing to do with pkgconfig.

Yes, it could also be that local/lib/pkgconfig/ contents are somehow broken and thus
break Python's pkgconfig.

Was it a build from scratch? If not, stale things in local/lib/pkgconfig/ may break stuff.

comment:21

On Catalina, there is no zlib in /usr/lib/pkgconfig and we don't build our own zlib with #27870.

buildbot-sage@osx build % ./sage -sh -c 'pkgconf --cflags zlib'

Package zlib was not found in the pkg-config search path.
Perhaps you should add the directory containing `zlib.pc'
to the PKG_CONFIG_PATH environment variable
Package 'zlib', required by 'world', not found

All builds from scratch.

comment:22

The version bump here should be ok, its #27870 that is borked

comment:23

No, #27870 is fine.

what's "borked" is that the new pkgconfig v1.5.1 throws an error if there is no .pc file. E.g. on this branch one sees

sage: import pkgconfig
sage: pkgconfig.parse("uhohoh")
---------------------------------------------------------------------------
PackageNotFoundError                      Traceback (most recent call last)
<ipython-input-2-d927c49e8798> in <module>()
----> 1 pkgconfig.parse("uhohoh")

/home/dimpase/sage/local/lib/python3.7/site-packages/pkgconfig/pkgconfig.py in parse(packages, static)
    246     """
    247     for package in packages.split():
--> 248         _raise_if_not_exists(package)
    249 
    250     out = _query(packages, *_build_options('--cflags --libs', static=static))

/home/dimpase/sage/local/lib/python3.7/site-packages/pkgconfig/pkgconfig.py in _raise_if_not_exists(package)
    101 def _raise_if_not_exists(package):
    102     if not exists(package):
--> 103         raise PackageNotFoundError(package)
    104 
    105 

PackageNotFoundError: uhohoh not found
sage: 

but the previous version merrily returns some stuff:

sage: pkgconfig.parse("uhohoh")
defaultdict(<type 'list'>, {'define_macros': []})

All the systems this was tested apparently had zlib.pc somewhere, and so it worked.
Your box does not have it, and so an error is thrown.

OK, would it be easier if this is fixed (trivially) on a followup ticket rather than here?

comment:24

Feel free to fix it here since there are already a number of tickets that depend on this one that can't be merged until the issue is resolved

comment:25

imho the zlib spkg-configure.m4 should just ensure that a zlib.pc file can be found (i.e. use PKG_CHECK_MODULES)

comment:26

The whole thing about assuming that zlib.pc exists was iffy since #26286 - that's basically where the bug has slipped in.

It "worked" as the absense of zlib.pc was ignored by old Python's pkgconfig.

There is no actual need to insist on zlib.pc present, as Apple's implementation of zlib is good enough to be used.

comment:27

Indeed its never actually necessary to have .pc files you can always kludge it by hand and fudge cflags/libs on all supported platforms. Its just not a sane/maintainable/scalable system of working with shared libraries.

If you want to save OSX users from having to build zlib then we should just provide an appropriate zlib.pc on that particular snowflake of a platform.

comment:28

I'll provide workaround here, and properly deal with generation on zlib.pc on #28906.

For the problem at hand it'd suffice just to avoid an expection, and fake the output of
pkgconfig.parse() if need be.

comment:29

This should fix the OSX issue, IMHO (can't test on OSX, though)

Changed branch from e5c91db to none

comment:30

Oh blast, need to rename this branch...

New commits:

e5c91dbupdate pkgconfig to v1.5.1. See trac #28883
9d33b71allow absent zlib.pc

Commit: 9d33b71

comment:32

potentially, there could also be a problem with libpng.pc, for which ./configure does not check for/does not insist on its presense. But it comes before zlib.pc is tested, so that's apparently not blocking. I'll change #28906 to deal with these too.

comment:33

That should be pkgconfig.PackageNotFoundError, otherwise you'll just get a NameError raised during exception handling. Please fix that and rebase to squash the fix.

comment:34

I'm a little confused here. On OS X, when I run the command in comment:21 (./sage -sh -c 'pkgconf --cflags zlib'), I get the same output as given there, but the build succeeds for me. Why does it fail on some OS X systems and succeed on others? (It worked for me with the current branch but also the earlier one.)

comment:35

Replying to @jhpalmieri:

I'm a little confused here. On OS X, when I run the command in comment:21 (./sage -sh -c 'pkgconf --cflags zlib'), I get the same output as given there, but the build succeeds for me. Why does it fail on some OS X systems and succeed on others? (It worked for me with the current branch but also the earlier one.)

How about the commands in comment:15 ?

sage: import pkgconfig
sage: pkgconfig.parse('zlib')
comment:36

With the current branch:

sage:  import pkgconfig
sage: pkgconfig.parse('zlib')
defaultdict(<class 'list'>, {'libraries': ['z']})
comment:37

From reading https://github.com/matze/pkgconfig/blob/master/pkgconfig/pkgconfig.py
it could happen that you have environment variable PKG_CONFIG defined, and its value will be used instead of pkg-config in your PATH.

https://github.com/matze/pkgconfig/blob/a7108a65625a529dc1cf1b4779d23e5701f202dc/pkgconfig/pkgconfig.py#L108

comment:38

When I run Sage, it defines the environment variable

PKG_CONFIG_PATH=.../path/to/SAGE_ROOT/local/lib/pkgconfig

but I don't have anything similar set myself. export | grep PKG returns nothing.

comment:39

Is the output of

sage: import os
sage: os.system("which pkg-config")

the same as of which pkg-config at ./sage -sh prompt?

As well, please compare the value of env. var. PKG_CONFIG_PATH at the latter prompt with the output of

sage: os.environ.get('PKG_CONFIG_PATH', None)
comment:40

which pkg-config points to the homebrew installation, /usr/local/bin/pkg-config in both invocations.

sage: os.environ.get('PKG_CONFIG_PATH', None)
'/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/pkgconfig'
sage: sage.env.var('PKG_CONFIG_PATH')
sage:

and after running sage --sh:

$ echo $PKG_CONFIG_PATH
/Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/pkgconfig

('/Users/palmieri/Desktop/Sage_stuff/git/sage/` is SAGE_ROOT.)

comment:41

So maybe it's the presence of the homebrew pkg-config that makes this system behave differently from Volker's buildbot.

comment:42

You probably have zlib installed from homebrew.

What do you get with pkg-config zlib --variable pcfiledir ?

comment:43

Sage's Python has to dig up zlib.pc to read by pkgconfig.parse(), from somewhere.

By the way, I don't understand pkgconf in ./sage -sh -c 'pkgconf --cflags zlib ???

it should be pkg-config

I guess pkgconf is some "unofficial" name for pkg-config, used by Sage's pkgconf package, which installs pkg-config.

comment:44

pkgconf and pkg-config are two different programs both of which does the same thing. pkgconf has a pkg-config symlink as well. (Think of pkgconf as MPIR built with GMP compat and pkg-config as GMP.)

comment:45

So you were confused by Volker's pkgconf invocation --- sorry, I should have noticed it earlier. I bet ./sage -sh -c 'pkg-config --libs zlib has meaningful output with -lz in it.

comment:46

Replying to @isuruf:

You probably have zlib installed from homebrew.

What do you get with pkg-config zlib --variable pcfiledir ?

/usr/local/Homebrew/Library/Homebrew/os/mac/pkgconfig/10.15. Also

$ ./sage -sh -c 'pkg-config --libs zlib'
-lz
comment:47

What does pkg-config zlib --variable libdir give you?

comment:48

Replying to @jhpalmieri:

Replying to @isuruf:

You probably have zlib installed from homebrew.

What do you get with pkg-config zlib --variable pcfiledir ?

/usr/local/Homebrew/Library/Homebrew/os/mac/pkgconfig/10.15. Also

$ ./sage -sh -c 'pkg-config --libs zlib'
-lz

so it appears that you use zlib provided by Homebrew, which does supply zlib.pc, so everything works as it should. (I don't know whether they install a real zlib, or just provide zlib.pc pointing to the MacOS native one, probably the former)

comment:49

Replying to @isuruf:

What does pkg-config zlib --variable libdir give you?

/usr/lib

comment:50

For information on PEP517 and the whole poetry business of packaging https://blogs.gentoo.org/mgorny/2019/12/24/handling-pep-517-pyproject-toml-packages-in-gentoo/

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

809abd9added missing module name

Changed commit from 9d33b71 to 809abd9

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

f74f554update pkgconfig to v1.5.1. See trac #28883
fb8f4e5allow absent zlib.pc

Changed commit from 809abd9 to fb8f4e5

Changed reviewer from Isuru Fernando to Isuru Fernando, Erik Bray

comment:53

I've tried #28906 but it is ugly as hell, so not sure whether it should be getting in. So this is the minimal (rebased) fix.

Changed reviewer from Isuru Fernando, Erik Bray to Isuru Fernando, Erik Bray, Matthias Koeppe

comment:54

Looks good and works well on macOS Catalina - after fixing #28676.

comment:57

Follow-up on the poetry issue at #29810

Changed commit from fb8f4e5 to none

comment:58

"...and other forms of boredom advertised as poetry."