pimutils / vdirsyncer

📇 Synchronize calendars and contacts.
https://vdirsyncer.pimutils.org/
Other
1.53k stars 160 forks source link

setuptools-scm gets patched away by distro packagers #487

Closed untitaker closed 8 years ago

untitaker commented 8 years ago

In Fedora, setuptools-scm is patched away, and it seems Debian is going to do the same thing (actual patch). As explained in the second link, I think this is creating incorrect Python package metadata on the user system, at the same time I do understand that people don't necessarily want to package setuptools as a build dependency.

@mathstuf @fpytloun Is there anything unclear about what setuptools-scm is supposed to do? How can we improve the packaging guidelines?

@ronnypfannschmidt Is setuptools-scm required when using a tarball from PyPI?

mathstuf commented 8 years ago

The comments in the history for the patch indicates that setuptools-scm is failing without network access. Fedora's build setup (Koji) does not allow network access.

mathstuf commented 8 years ago

Heh, looking at the spec file, that patch isn't applied?

untitaker commented 8 years ago

The network requests you're seeing are most likely from setuptools trying to install setuptools-scm. If you make python-setuptools-scm a build dependency, it shouldn't get installed by pip. I don't think setuptools-scm does any network requests.

RonnyPfannschmidt commented 8 years ago

setuptools will easy_install missing setup_requirements - packaging setuptools-scm first is the correct way

RonnyPfannschmidt commented 8 years ago

the fact that the Packagers dont understand the errors is worrying

untitaker commented 8 years ago

Why is setuptools-scm required for installing from PyPI? Couldn't it hardcode the determined version into setup.py when creating the sdist?

RonnyPfannschmidt commented 8 years ago

it takes a while to implement that one, since it needs removing the setup-requires as well

i took a look at doing a correct implementation a while back and decided i wont do it unless i find a few days motivated for just doing that

since its hooking into setuptools the world is full of strange edge-cases

untitaker commented 8 years ago

I've pushed a commit to master that should make it more obvious which dependencies go where.

untitaker commented 8 years ago

I wonder if I could copy the logic of parse_pkginfo into setup.py, such that the dependency is only required when PKG-INFO doesn't exist.

RonnyPfannschmidt commented 8 years ago

should setuptools_scm provide a shim file that can be inserted into a repo, giving it the needed tooling?

untitaker commented 8 years ago

I'm not sure to what extent this would count as "vendoring" and therefore make the entire packaging situation even worse.

RonnyPfannschmidt commented 8 years ago

the file would have a nice documentative header telling its purely there to prevent those bad packagers from doing worse

untitaker commented 8 years ago

Meh. I don't think the situation is to blame on distro packagers, Python's packaging is confusing to me as well. Given that setuptools-scm is not really "standard", I can totally understand the additional confusion around it.

RonnyPfannschmidt commented 8 years ago

i'll teach setuptools to opt out of easy_install, and print the missing deps instead ^^

fpytloun commented 8 years ago

If I am not wrong, all that setuptools-scm does in this case is that it generates vdirsyncer/version.py that contains variable version=x.y.z. Which is what I do here instead https://github.com/fpytloun/debian-vdirsyncer/blob/master/debian/rules#L11 which seems to be ok to me as d/changelog is in this case main source of truth for package version.

But main reason for getting rid of setuptools-scm was it's need to access network which is forbidden during package build (as far as I remember it was also failing when building from Pypy sdist but can't verify now) but this was probably solved by this recent patch to dh-python:

https://anonscm.debian.org/cgit/dh-python/dh-python.git/commit/?id=3bf414409b5af46ede0f6d3a6bbb89487eeb5aeb https://anonscm.debian.org/cgit/dh-python/dh-python.git/commit/?id=29146857bbedd4a5e4c609f79b2965121eebee71

untitaker commented 8 years ago

If I am not wrong, all that setuptools-scm does in this case is that it generates vdirsyncer/version.py that contains variable version=x.y.z

There is also metadata in a directory called vdirsyncer.dist-info that contains information about the vdirsyncer module. This is important! If khal wants to check which version of vdirsyncer is installed (to change its behavior based on that), it would query that information. With your previous patch it would probably be set to a bogus value. In a correct installation, this prints the correct version of vdirsyncer:

python3 -c 'print(__import__("pkg_resources").get_distribution("vdirsyncer"))'

I don't understand why the patch for dh-python is necessary. It seems Debian does heavy modification of Python's packaging internals, and I find that appalling, but it seems that there's no other choice but to just go with it at the moment.

RonnyPfannschmidt commented 8 years ago

@fpytloun setuptools_scm does NOT acces the network, setuptools does if the packager didnt add a build-dependency on setuptools-scm ensuring its installed before running setup.py

@fpytloun in addition setuptools_scm does set the version metadata for python packages, as far as i can tell the patch only removes that

fpytloun commented 8 years ago

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512

Mentioned patch fixes behavior when building from non-pypi source tarball (eg. Github). I reworked packaging of vdirsyncer to use pypi sdist and added setuptools-scm into build-depends which seems to be working as expected.

  1. srpna 2016 14:06:09 SELČ, Markus Unterwaditzer notifications@github.com napsal:

    If I am not wrong, all that setuptools-scm does in this case is that it generates vdirsyncer/version.py that contains variable version=x.y.z

    There is also metadata in a directory called vdirsyncer.dist-info that contains information about the vdirsyncer module. This is important! If khal wants to check which version of vdirsyncer is installed (to change its behavior based on that), it would query that information. With your previous patch it would probably be set to a bogus value. In a correct installation, this prints the correct version of vdirsyncer:

    python -c 'print(import("pkg_resources").get_distribution("vdirsyncer"))'

    I don't understand why the patch for dh-python is necessary. It seems Debian does heavy modification of Python's packaging internals, and I find that appalling, but it seems that there's no other choice but to just go with it at the moment.

    -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/pimutils/vdirsyncer/issues/487#issuecomment-239670182 -----BEGIN PGP SIGNATURE----- Version: APG v1.1.1

iQI+BAEBCgAoBQJXsF/pIRxGaWxpcCBQeXRsb3VuIDxmaWxpcEBweXRsb3VuLmN6 PgAKCRBoCCObnHLmG8/rEACnRkY7v7Q9eJAGwJ40SoWy34Lp3xW/LfwEr+Rvq093 4BEZ8rpNDGtioOXBXzfC6JSIydFa/XQPOBHqQT6JyiKCV4Ylngn4kKtuZkIhZc1O X2Z9E7tqoVYV3ZEHuvSQGOOKMIa3wb8T2Nt7nh5yb8vapaqddFvXNhy77lauaJ8Q OdCkdGHeaIG1RTeyFheR7mlEtqyQW5t6Dn3tD23hg70vIe8WAQA53b0JWYaNuX48 PXoJtHFXiG9UZ4P5W3aGuB5tO8heH9JOQSvbaXl7+S8nmEK5yPlDw94FficN1jXb ikLINGk3pYqX9gitEE7LOCj+qufDlgLVE1i8OaJFli36TKVv4HzlScrr0VBpESdx nVSxq8AlHNv2rY9PzN0rBbniqO5d6bW+YyESa3qsNl60bxnZuhHOyWURb6O3uMG/ XM2e/+dngVPu/5t6JoR1J2rsfdaf37NocU2BFnl8Zf8Tij31XBrC9cpIvbCLe040 jZWrq3qlljGeN/H570C5mAIPY6SpVCQCHLpMeqje+Rm/iHoizrpu6ED6nSp2/blv lwXNrokhB8rTVmocIFLZf2BTlmlNAzUVAZLGmSZiAuncsqotTU/Kggt3EVCe9yOs 8DD4CXt43RnE5yXYOob96zPCuTKJhxjvnhiEjdT7L1Ooqzzx4w8Z+DtG3m2bD+kG /g== =28se -----END PGP SIGNATURE-----

untitaker commented 8 years ago

@fpytloun I see, thanks. I think I misunderstood what the patch is for.

@mathstuf @mbaldessari Could you change the Fedora package such that it depends on setuptools-scm? It appears there is a rpm package called python-setuptools_scm for that: https://apps.fedoraproject.org/packages/python-setuptools_scm

mbaldessari commented 8 years ago

@untitaker I have done that around March or so when I upgraded the package to 0.9.3.

untitaker commented 8 years ago

Okay, thank you. I think I was confused because the patch still existed in the git repo.