Package: guix;
Reported by: ng0 <ng0 <at> we.make.ritual.n0.is>
Date: Fri, 16 Sep 2016 20:02:01 UTC
Severity: normal
Tags: patch
Merged with 24557, 33047, 33569, 34266
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24450 in the body.
You can then email your comments to 24450 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
bug-guix <at> gnu.org:bug#24450; Package guix.
(Fri, 16 Sep 2016 20:02:01 GMT) Full text and rfc822 format available.ng0 <ng0 <at> we.make.ritual.n0.is>:bug-guix <at> gnu.org.
(Fri, 16 Sep 2016 20:02:01 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: ng0 <ng0 <at> we.make.ritual.n0.is> To: bug-guix <at> gnu.org Subject: pypi importer outputs strange character series in optional dependency case. Date: Fri, 16 Sep 2016 20:00:02 +0000
I think this should not happen with pypi import:
(inputs
`(("python-certifi==2016.2.28"
,python-certifi==2016.2.28)
("python-dateutil==2.5.3"
,python-dateutil==2.5.3)
("python-flask-babel==0.11.1"
,python-flask-babel==0.11.1)
("python-flask==0.11.1" ,python-flask==0.11.1)
("python-lxml==3.6.0" ,python-lxml==3.6.0)
("python-ndg-httpsclient==0.4.1"
,python-ndg-httpsclient==0.4.1)
("python-pyasn1-modules==0.0.8"
,python-pyasn1-modules==0.0.8)
("python-pyasn1==0.1.9" ,python-pyasn1==0.1.9)
("python-pygments==2.1.3"
,python-pygments==2.1.3)
("python-pyopenssl==0.15.1"
,python-pyopenssl==0.15.1)
("python-pyyaml==3.11" ,python-pyyaml==3.11)
("python-requests[socks]==2.10.0"
,#{python-requests\x5b;socks\x5d;==2.10.0}#)
("python-setuptools" ,python-setuptools)))
I can understand the version numbers, I can also understand the optional
socks building/module of the python-requests, but why does it read like
Gobbledygook? Can't we improve the output here?
For version numbers, this is not a format which happened recently which
is exclusive for python build system right? This is just bad formated
because of the pypi query.
I will first try and not pin the application to these version numbers,
maybe itjustworks™.
To reproduce: "guix import pypi searx"
--
ng0
T460s laptop <maxim.cournoyer <at> gmail.com>
to control <at> debbugs.gnu.org.
(Thu, 28 Mar 2019 04:33:02 GMT) Full text and rfc822 format available.Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
to control <at> debbugs.gnu.org.
(Thu, 28 Mar 2019 04:38:02 GMT) Full text and rfc822 format available.bug-guix <at> gnu.org:bug#24450; Package guix.
(Fri, 29 Mar 2019 04:20:01 GMT) Full text and rfc822 format available.Message #12 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: swedebugia <swedebugia <at> riseup.net> Cc: 34266 <at> debbugs.gnu.org, 24450 <at> debbugs.gnu.org Subject: Re: bug#34266: pypi importer cannot handle [ and ] correctly Date: Fri, 29 Mar 2019 00:19:12 -0400
swedebugia <swedebugia <at> riseup.net> writes:
> $ ./pre-inst-env guix import pypi beaker
>
> following redirection to `https://pypi.org/pypi/Beaker/json'...
>
> Starting download of /tmp/guix-file.p15GJZ
> From
> https://files.pythonhosted.org/packages/c2/21/b052b2fbfee3def06670923d5d34b0d353d4c278013e4a714c3fb663f150/Beaker-1.10.0.tar.gz...
> ...0.0.tar.gz 40KiB 521KiB/s 00:00
> [##################] 100.0%
> (package
> (name "python-beaker")
> (version "1.10.0")
> (source
> (origin
> (method url-fetch)
> (uri (pypi-uri "beaker" version))
> (sha256
> (base32
> "0l047yl3n9b3w7ba0wrqdb5fpww5y8pjy20kah2mlpr230lqjwk0"))))
> (build-system python-build-system)
> (propagated-inputs
> `(("python-[crypto]" ,#{python-\x5b;crypto\x5d;}#)
> ("python-[cryptography]"
> ,#{python-\x5b;cryptography\x5d;}#)
> ("python-[pycrypto]"
> ,#{python-\x5b;pycrypto\x5d;}#)
> ("python-[pycryptodome]"
> ,#{python-\x5b;pycryptodome\x5d;}#)
> ("python-[testsuite]"
> ,#{python-\x5b;testsuite\x5d;}#)
> ("python-coverage" ,python-coverage)
> ("python-cryptography" ,python-cryptography)
> ("python-cryptography" ,python-cryptography)
> ("python-funcsigs" ,python-funcsigs)
> ("python-memcached" ,python-memcached)
> ("python-mock" ,python-mock)
> ("python-nose" ,python-nose)
> ("python-pycrypto" ,python-pycrypto)
> ("python-pycryptodome" ,python-pycryptodome)
> ("python-pycryptodome" ,python-pycryptodome)
> ("python-pycryptopp" ,python-pycryptopp)
> ("python-pylibmc" ,python-pylibmc)
> ("python-pymongo" ,python-pymongo)
> ("python-redis" ,python-redis)
> ("python-sqlalchemy" ,python-sqlalchemy)
> ("python-webtest" ,python-webtest)))
> (home-page "https://beaker.readthedocs.io/")
> (synopsis
> "A Session and Caching library with WSGI Middleware")
> (description
> "A Session and Caching library with WSGI Middleware")
> (license license:bsd-3))
Testing with my soon-to-be sent for review changes:
--8<---------------cut here---------------start------------->8---
./pre-inst-env guix import pypi beaker
following redirection to `https://pypi.org/pypi/Beaker/json'...
Starting download of /tmp/guix-file.0MWu4B
From https://files.pythonhosted.org/packages/76/87/ecc1a222f0caaa7ba7b8928737e89b2e91b8c22450c12b8a51ee625a4d87/Beaker-1.10.1.tar.gz...
…0.1.tar.gz 40KiB 487KiB/s 00:00 [##################] 100.0%
(package
(name "python-beaker")
(version "1.10.1")
(source
(origin
(method url-fetch)
(uri (pypi-uri "beaker" version))
(sha256
(base32
"16zdjfl8v73yl1capph0n371vd26c7zpzb48n505ip32ffgmvc4f"))))
(build-system python-build-system)
(native-inputs
`(("python-coverage" ,python-coverage)
("python-cryptography" ,python-cryptography)
("python-memcached" ,python-memcached)
("python-mock" ,python-mock)
("python-nose" ,python-nose)
("python-pycryptodome" ,python-pycryptodome)
("python-pylibmc" ,python-pylibmc)
("python-pymongo" ,python-pymongo)
("python-redis" ,python-redis)
("python-sqlalchemy" ,python-sqlalchemy)
("python-webtest" ,python-webtest)))
(home-page "https://beaker.readthedocs.io/")
(synopsis
"A Session and Caching library with WSGI Middleware")
(description
"A Session and Caching library with WSGI Middleware")
(license license:bsd-3))
--8<---------------cut here---------------end--------------->8---
Looking better?
Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Fri, 29 Mar 2019 04:22:02 GMT) Full text and rfc822 format available.Message #15 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: swedebugia <swedebugia <at> riseup.net> Cc: 33569 <at> debbugs.gnu.org, 24450 <at> debbugs.gnu.org Subject: Re: bug#33569: Missing sanitizing of '[]' in pypi-importer Date: Fri, 29 Mar 2019 00:20:53 -0400
swedebugia <swedebugia <at> riseup.net> writes:
> E.g.
> sdb <at> komputilo ~/guix-tree$ ~/guix-tree/pre-inst-env guix import pypi
> snakemake
> ...
> (propagated-inputs
> `(("python-[reports]"
> ,#{python-\x5b;reports\x5d;}#)
> ("python-appdirs" ,python-appdirs)
> ...
This one now gives (local branch):
--8<---------------cut here---------------start------------->8---
./pre-inst-env guix import pypi snakemake
Starting download of /tmp/guix-file.4XvWMX
From https://files.pythonhosted.org/packages/4a/aa/aab1515d220be06fbdccf3c89335d9585b08ac6be74b8e3c9e8c3c32798e/snakemake-5.4.4.tar.gz...
….4.4.tar.gz 169KiB 723KiB/s 00:00 [##################] 100.0%
(package
(name "python-snakemake")
(version "5.4.4")
(source
(origin
(method url-fetch)
(uri (pypi-uri "snakemake" version))
(sha256
(base32
"0prpr5qajqwr8sh4gzggpj8l4np2rcm9nfdzvcp30d5yw7h26wqm"))))
(build-system python-build-system)
(propagated-inputs
`(("python-appdirs" ,python-appdirs)
("python-configargparse" ,python-configargparse)
("python-datrie" ,python-datrie)
("python-docutils" ,python-docutils)
("python-gitpython" ,python-gitpython)
("python-jsonschema" ,python-jsonschema)
("python-pyyaml" ,python-pyyaml)
("python-ratelimiter" ,python-ratelimiter)
("python-requests" ,python-requests)
("python-wrapt" ,python-wrapt)))
(home-page "http://snakemake.bitbucket.io")
(synopsis
"Snakemake is a workflow management system that aims to reduce the complexity of creating workflows by providing a fast and comfortable execution environment, together with a clean and modern specification language in python style. Snakemake workflows are essentially Python scripts extended by declarative code to define rules. Rules describe how to create output files from input files.")
(description
"Snakemake is a workflow management system that aims to reduce the complexity of creating workflows by providing a fast and comfortable execution environment, together with a clean and modern specification language in python style. Snakemake workflows are essentially Python scripts extended by declarative code to define rules. Rules describe how to create output files from input files.")
(license license:expat))
--8<---------------cut here---------------end--------------->8---
bug-guix <at> gnu.org:bug#24450; Package guix.
(Fri, 29 Mar 2019 04:24:02 GMT) Full text and rfc822 format available.Message #18 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Julien Lepiller <julien <at> lepiller.eu> Cc: 33047 <at> debbugs.gnu.org, 24450 <at> debbugs.gnu.org Subject: Re: bug#33047: pypi importer uses incorrect package names Date: Fri, 29 Mar 2019 00:23:03 -0400
Julien Lepiller <julien <at> lepiller.eu> writes:
> Hi,
>
> I found that sometimes the pypi importer had trouble importing
> packages correctly. For instance, running "guix import pypi txaio"
> gave me this list of dependencies:
>
> (propagated-inputs
> `(("python-[all]" ,#{python-\x5b;all\x5d;}#)
> ("python-[asyncio]"
> ,#{python-\x5b;asyncio\x5d;}#)
> ...))
>
> guix import pypi magic-wormhole had this:
>
> (propagated-inputs
> ("python-autobahn[twisted]"
> ,#{python-autobahn\x5b;twisted\x5d;}#)
> ...))
>
> Of course, they break the recursive importer, which makes it difficult
> to import these packages correctly.
Testing local branch:
./pre-inst-env guix import pypi txaio
Starting download of /tmp/guix-file.jTNBQz
From https://files.pythonhosted.org/packages/c1/99/81de004578e9afe017bb1d4c8968088a33621c05449fe330bdd7016d5377/txaio-18.8.1.tar.gz...
…8.1.tar.gz 50KiB 894KiB/s 00:00 [##################] 100.0%
Starting download of /tmp/guix-file.ZB3Q2n
From https://files.pythonhosted.org/packages/e9/6d/e1a6f7835cde86728e5bb1f577be9b2d7d273fdb33c286e70b087d418ded/txaio-18.8.1-py2.py3-none-any.whl...
….py3-none-any.whl 27KiB 746KiB/s 00:00 [##################] 100.0%
(package
(name "python-txaio")
(version "18.8.1")
(source
(origin
(method url-fetch)
(uri (pypi-uri "txaio" version))
(sha256
(base32
"1zmpdph6zddgrnkkcykh6qk5s46l7s5mzfqrh82m4b5iffn61qv7"))))
(build-system python-build-system)
(propagated-inputs `(("python-six" ,python-six)))
(native-inputs
`(("python-mock" ,python-mock)
("python-pep8" ,python-pep8)
("python-pyenchant" ,python-pyenchant)
("python-pytest" ,python-pytest)
("python-pytest-cov" ,python-pytest-cov)
("python-sphinx" ,python-sphinx)
("python-sphinx-rtd-theme"
,python-sphinx-rtd-theme)
("python-sphinxcontrib-spelling"
,python-sphinxcontrib-spelling)
("python-tox" ,python-tox)
("python-twine" ,python-twine)
("python-wheel" ,python-wheel)))
(home-page "https://github.com/crossbario/txaio")
(synopsis
"Compatibility API between asyncio/Twisted/Trollius")
(description
"Compatibility API between asyncio/Twisted/Trollius")
(license #f))
and
./pre-inst-env guix import pypi magic-wormhole
Starting download of /tmp/guix-file.80RRqk
From https://files.pythonhosted.org/packages/77/15/9438290bab8146efc0213f7c3d9645d9bc5a2e885e4049477e7432e40336/magic-wormhole-0.11.2.tar.gz...
…-0.11.2.tar.gz 193KiB 911KiB/s 00:00 [##################] 100.0%
Starting download of /tmp/guix-file.mRGAx3
From https://files.pythonhosted.org/packages/82/98/3e8d12fdb90457e8f3e1f5b877ee27f5db58dbaf4a4fbe95f7287a568401/magic_wormhole-0.11.2-py2.py3-none-any.whl...
…-py2.py3-none-any.whl 128KiB 1009KiB/s 00:00 [##################] 100.0%
(package
(name "python-magic-wormhole")
(version "0.11.2")
(source
(origin
(method url-fetch)
(uri (pypi-uri "magic-wormhole" version))
(sha256
(base32
"01fr4bi6kc6fz9n3c4qq892inrc3nf6p2djy65yvm7xkvdxncydf"))))
(build-system python-build-system)
(propagated-inputs
`(("python-attrs" ,python-attrs)
("python-autobahn" ,python-autobahn)
("python-automat" ,python-automat)
("python-click" ,python-click)
("python-hkdf" ,python-hkdf)
("python-humanize" ,python-humanize)
("python-pynacl" ,python-pynacl)
("python-pywin32" ,python-pywin32)
("python-six" ,python-six)
("python-spake2" ,python-spake2)
("python-tqdm" ,python-tqdm)
("python-twisted" ,python-twisted)
("python-txtorcon" ,python-txtorcon)))
(native-inputs
`(("python-magic-wormhole-mailbox-server"
,python-magic-wormhole-mailbox-server)
("python-magic-wormhole-transit-relay"
,python-magic-wormhole-transit-relay)
("python-mock" ,python-mock)
("python-pyflakes" ,python-pyflakes)
("python-tox" ,python-tox)))
(home-page
"https://github.com/warner/magic-wormhole")
(synopsis
"Securely transfer data between computers")
(description
"Securely transfer data between computers")
(license license:expat))
bug-guix <at> gnu.org:bug#24450; Package guix.
(Fri, 29 Mar 2019 04:25:02 GMT) Full text and rfc822 format available.Message #21 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: ng0 <ng0 <at> we.make.ritual.n0.is> Cc: 24450 <at> debbugs.gnu.org Subject: Re: bug#24450: pypi importer outputs strange character series in optional dependency case. Date: Fri, 29 Mar 2019 00:24:24 -0400
ng0 <ng0 <at> we.make.ritual.n0.is> writes:
> I think this should not happen with pypi import:
>
> (inputs
> `(("python-certifi==2016.2.28"
> ,python-certifi==2016.2.28)
> ("python-dateutil==2.5.3"
> ,python-dateutil==2.5.3)
> ("python-flask-babel==0.11.1"
> ,python-flask-babel==0.11.1)
> ("python-flask==0.11.1" ,python-flask==0.11.1)
> ("python-lxml==3.6.0" ,python-lxml==3.6.0)
> ("python-ndg-httpsclient==0.4.1"
> ,python-ndg-httpsclient==0.4.1)
> ("python-pyasn1-modules==0.0.8"
> ,python-pyasn1-modules==0.0.8)
> ("python-pyasn1==0.1.9" ,python-pyasn1==0.1.9)
> ("python-pygments==2.1.3"
> ,python-pygments==2.1.3)
> ("python-pyopenssl==0.15.1"
> ,python-pyopenssl==0.15.1)
> ("python-pyyaml==3.11" ,python-pyyaml==3.11)
> ("python-requests[socks]==2.10.0"
> ,#{python-requests\x5b;socks\x5d;==2.10.0}#)
> ("python-setuptools" ,python-setuptools)))
>
>
> I can understand the version numbers, I can also understand the optional
> socks building/module of the python-requests, but why does it read like
> Gobbledygook? Can't we improve the output here?
>
> For version numbers, this is not a format which happened recently which
> is exclusive for python build system right? This is just bad formated
> because of the pypi query.
> I will first try and not pin the application to these version numbers,
> maybe itjustworks™.
>
>
> To reproduce: "guix import pypi searx"
This would now give (change to be sent for review soon):
--8<---------------cut here---------------start------------->8---
./pre-inst-env guix import pypi searx
Starting download of /tmp/guix-file.1wD8K4
From https://files.pythonhosted.org/packages/75/3f/5941ad2d500ff7cf6f8da1022c78013dcd2207941d533586a8e7bfe699d3/searx-0.15.0.tar.gz...
…5.0.tar.gz 1.6MiB 729KiB/s 00:02 [##################] 100.0%
(package
(name "python-searx")
(version "0.15.0")
(source
(origin
(method url-fetch)
(uri (pypi-uri "searx" version))
(sha256
(base32
"1gmww73q7wydkvlyz73wnr3sybpjn40wha7avnz9ak9m365zcjxf"))))
(build-system python-build-system)
(propagated-inputs
`(("python-certifi" ,python-certifi)
("python-dateutil" ,python-dateutil)
("python-flask" ,python-flask)
("python-flask-babel" ,python-flask-babel)
("python-idna" ,python-idna)
("python-lxml" ,python-lxml)
("python-pygments" ,python-pygments)
("python-pyopenssl" ,python-pyopenssl)
("python-pyyaml" ,python-pyyaml)
("python-requests" ,python-requests)))
(native-inputs
`(("python-babel" ,python-babel)
("python-cov-core" ,python-cov-core)
("python-mock" ,python-mock)
("python-nose2" ,python-nose2)
("python-pep8" ,python-pep8)
("python-plone.testing" ,python-plone.testing)
("python-selenium" ,python-selenium)
("python-splinter" ,python-splinter)
("python-transifex-client"
,python-transifex-client)
("python-unittest2" ,python-unittest2)
("python-zope.testrunner"
,python-zope.testrunner)))
(home-page "https://github.com/asciimoo/searx")
(synopsis
"A privacy-respecting, hackable metasearch engine")
(description
"A privacy-respecting, hackable metasearch engine")
(license #f))
--8<---------------cut here---------------end--------------->8---
bug-guix <at> gnu.org:bug#24450; Package guix.
(Fri, 29 Mar 2019 04:35:02 GMT) Full text and rfc822 format available.Message #24 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: ng0 <ng0 <at> we.make.ritual.n0.is> Cc: 24450 <at> debbugs.gnu.org Subject: [PATCH] bug#24450: pypi importer outputs strange character series in optional dependency case. Date: Fri, 29 Mar 2019 00:34:43 -0400
[Message part 1 (text/plain, inline)]
Hello,
ng0 <ng0 <at> we.make.ritual.n0.is> writes:
> I think this should not happen with pypi import:
>
> (inputs
> `(("python-certifi==2016.2.28"
> ,python-certifi==2016.2.28)
> ("python-dateutil==2.5.3"
> ,python-dateutil==2.5.3)
> ("python-flask-babel==0.11.1"
> ,python-flask-babel==0.11.1)
> ("python-flask==0.11.1" ,python-flask==0.11.1)
> ("python-lxml==3.6.0" ,python-lxml==3.6.0)
> ("python-ndg-httpsclient==0.4.1"
> ,python-ndg-httpsclient==0.4.1)
> ("python-pyasn1-modules==0.0.8"
> ,python-pyasn1-modules==0.0.8)
> ("python-pyasn1==0.1.9" ,python-pyasn1==0.1.9)
> ("python-pygments==2.1.3"
> ,python-pygments==2.1.3)
> ("python-pyopenssl==0.15.1"
> ,python-pyopenssl==0.15.1)
> ("python-pyyaml==3.11" ,python-pyyaml==3.11)
> ("python-requests[socks]==2.10.0"
> ,#{python-requests\x5b;socks\x5d;==2.10.0}#)
> ("python-setuptools" ,python-setuptools)))
>
>
> I can understand the version numbers, I can also understand the optional
> socks building/module of the python-requests, but why does it read like
> Gobbledygook? Can't we improve the output here?
>
> For version numbers, this is not a format which happened recently which
> is exclusive for python build system right? This is just bad formated
> because of the pypi query.
> I will first try and not pin the application to these version numbers,
> maybe itjustworks™.
>
>
> To reproduce: "guix import pypi searx"
The following patches fix this, and more!
[0001-import-pypi-Do-not-consider-requirements.txt-files.patch (text/x-patch, attachment)]
[0002-import-pypi-Do-not-parse-optional-requirements-from-.patch (text/x-patch, attachment)]
[0003-import-pypi-Improve-parsing-of-requirement-specifica.patch (text/x-patch, attachment)]
[0004-import-pypi-Deduplicate-requirements.patch (text/x-patch, attachment)]
[0005-import-pypi-Support-more-types-of-archives.patch (text/x-patch, attachment)]
[0006-import-pypi-Parse-wheel-METADATA-instead-of-metadata.patch (text/x-patch, attachment)]
[0007-import-pypi-Include-optional-test-inputs-as-native-i.patch (text/x-patch, attachment)]
[Message part 9 (text/plain, inline)]
Thanks, Maxim
[signature.asc (application/pgp-signature, inline)]
bug-guix <at> gnu.org:bug#24450; Package guix.
(Sat, 30 Mar 2019 02:13:02 GMT) Full text and rfc822 format available.Message #27 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: T460s laptop <maxim.cournoyer <at> gmail.com> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: ng0 <ng0 <at> we.make.ritual.n0.is>, 24450 <at> debbugs.gnu.org Subject: [PATCHv2] bug#24450: pypi importer outputs strange character series in optional dependency case. Date: Fri, 29 Mar 2019 22:12:38 -0400
[Message part 1 (text/plain, inline)]
The previous 0007 patch had broken the recursive importer. This reworked version fixes this. The rest of the patches stack sent in the previous message is still good as is.
[0007-import-pypi-Include-optional-test-inputs-as-native-i.patch (text/x-patch, attachment)]
[signature.asc (application/pgp-signature, inline)]
T460s laptop <maxim.cournoyer <at> gmail.com>
to control <at> debbugs.gnu.org.
(Sat, 30 Mar 2019 02:16:02 GMT) Full text and rfc822 format available.bug-guix <at> gnu.org:bug#24450; Package guix.
(Sat, 30 Mar 2019 07:58:01 GMT) Full text and rfc822 format available.Message #32 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: swedebugia <swedebugia <at> riseup.net> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 34266 <at> debbugs.gnu.org, 24450 <at> debbugs.gnu.org Subject: Re: bug#34266: pypi importer cannot handle [ and ] correctly Date: Sat, 30 Mar 2019 08:59:12 +0100
[Message part 1 (text/plain, inline)]
On 2019-03-29 05:19, Maxim Cournoyer wrote:
> swedebugia <swedebugia <at> riseup.net> writes:
>
>> $ ./pre-inst-env guix import pypi beaker
>>
>> following redirection to `https://pypi.org/pypi/Beaker/json'...
>>
>> Starting download of /tmp/guix-file.p15GJZ
>> From
>> https://files.pythonhosted.org/packages/c2/21/b052b2fbfee3def06670923d5d34b0d353d4c278013e4a714c3fb663f150/Beaker-1.10.0.tar.gz...
>> ...0.0.tar.gz 40KiB 521KiB/s 00:00
>> [##################] 100.0%
>> (package
>> (name "python-beaker")
>> (version "1.10.0")
>> (source
>> (origin
>> (method url-fetch)
>> (uri (pypi-uri "beaker" version))
>> (sha256
>> (base32
>> "0l047yl3n9b3w7ba0wrqdb5fpww5y8pjy20kah2mlpr230lqjwk0"))))
>> (build-system python-build-system)
>> (propagated-inputs
>> `(("python-[crypto]" ,#{python-\x5b;crypto\x5d;}#)
>> ("python-[cryptography]"
>> ,#{python-\x5b;cryptography\x5d;}#)
>> ("python-[pycrypto]"
>> ,#{python-\x5b;pycrypto\x5d;}#)
>> ("python-[pycryptodome]"
>> ,#{python-\x5b;pycryptodome\x5d;}#)
>> ("python-[testsuite]"
>> ,#{python-\x5b;testsuite\x5d;}#)
>> ("python-coverage" ,python-coverage)
>> ("python-cryptography" ,python-cryptography)
>> ("python-cryptography" ,python-cryptography)
>> ("python-funcsigs" ,python-funcsigs)
>> ("python-memcached" ,python-memcached)
>> ("python-mock" ,python-mock)
>> ("python-nose" ,python-nose)
>> ("python-pycrypto" ,python-pycrypto)
>> ("python-pycryptodome" ,python-pycryptodome)
>> ("python-pycryptodome" ,python-pycryptodome)
>> ("python-pycryptopp" ,python-pycryptopp)
>> ("python-pylibmc" ,python-pylibmc)
>> ("python-pymongo" ,python-pymongo)
>> ("python-redis" ,python-redis)
>> ("python-sqlalchemy" ,python-sqlalchemy)
>> ("python-webtest" ,python-webtest)))
>> (home-page "https://beaker.readthedocs.io/")
>> (synopsis
>> "A Session and Caching library with WSGI Middleware")
>> (description
>> "A Session and Caching library with WSGI Middleware")
>> (license license:bsd-3))
>
> Testing with my soon-to-be sent for review changes:
>
> --8<---------------cut here---------------start------------->8---
> ./pre-inst-env guix import pypi beaker
> following redirection to `https://pypi.org/pypi/Beaker/json'...
>
> Starting download of /tmp/guix-file.0MWu4B
> From https://files.pythonhosted.org/packages/76/87/ecc1a222f0caaa7ba7b8928737e89b2e91b8c22450c12b8a51ee625a4d87/Beaker-1.10.1.tar.gz...
> …0.1.tar.gz 40KiB 487KiB/s 00:00 [##################] 100.0%
> (package
> (name "python-beaker")
> (version "1.10.1")
> (source
> (origin
> (method url-fetch)
> (uri (pypi-uri "beaker" version))
> (sha256
> (base32
> "16zdjfl8v73yl1capph0n371vd26c7zpzb48n505ip32ffgmvc4f"))))
> (build-system python-build-system)
> (native-inputs
> `(("python-coverage" ,python-coverage)
> ("python-cryptography" ,python-cryptography)
> ("python-memcached" ,python-memcached)
> ("python-mock" ,python-mock)
> ("python-nose" ,python-nose)
> ("python-pycryptodome" ,python-pycryptodome)
> ("python-pylibmc" ,python-pylibmc)
> ("python-pymongo" ,python-pymongo)
> ("python-redis" ,python-redis)
> ("python-sqlalchemy" ,python-sqlalchemy)
> ("python-webtest" ,python-webtest)))
> (home-page "https://beaker.readthedocs.io/")
> (synopsis
> "A Session and Caching library with WSGI Middleware")
> (description
> "A Session and Caching library with WSGI Middleware")
> (license license:bsd-3))
> --8<---------------cut here---------------end--------------->8---
>
> Looking better?
Perfect!
--
Cheers Swedebugia
[signature.asc (application/pgp-signature, attachment)]
bug-guix <at> gnu.org:bug#24450; Package guix.
(Sun, 31 Mar 2019 14:41:01 GMT) Full text and rfc822 format available.Message #35 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: ng0 <ng0 <at> we.make.ritual.n0.is> Cc: 24450 <at> debbugs.gnu.org Subject: [PATCH] bug#24450: pypi importer outputs strange character series in optional dependency case. Date: Sun, 31 Mar 2019 10:40:34 -0400
[Message part 1 (text/plain, inline)]
Hello! I've yet another commit to add on the top of the current stack of patches already sent. This one makes it so that the source archive is searched for the '[...].egg-info/requires.txt' file rather than check the more common, fixed location. This makes the importer more useful, as some packages such as robotframework-sshlibrary use a "src" directory in their hierarchy which would cause the previous scheme to not find requires.txt.
[0008-import-pypi-Scan-source-archive-to-find-requires.txt.patch (text/x-patch, attachment)]
[Message part 3 (text/plain, inline)]
Thanks, Maxim
[signature.asc (application/pgp-signature, inline)]
Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
to control <at> debbugs.gnu.org.
(Sun, 31 Mar 2019 14:42:02 GMT) Full text and rfc822 format available.bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 01 Apr 2019 15:29:02 GMT) Full text and rfc822 format available.Message #40 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: T460s laptop <maxim.cournoyer <at> gmail.com> Cc: ng0 <ng0 <at> we.make.ritual.n0.is>, 24450 <at> debbugs.gnu.org Subject: Re: bug#24450: [PATCHv2] bug#24450: pypi importer outputs strange character series in optional dependency case. Date: Mon, 01 Apr 2019 17:28:45 +0200
Hi Maxim,
T460s laptop <maxim.cournoyer <at> gmail.com> skribis:
> From 37e499d5d5d5f690aa0a065c730e13f6a31dd30d Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
> Date: Thu, 28 Mar 2019 23:12:26 -0400
> Subject: [PATCH] import: pypi: Include optional test inputs as native-inputs.
>
> * guix/import/pypi.scm (maybe-inputs): Add INPUT-TYPE argument, and use it.
> (test-section?): New predicate.
> (parse-requires.txt): Collect the optional test inputs, and return them as the
> second element of the returned list.
> (parse-wheel-metadata): Likewise.
> (guess-requirements): Adapt, and hide unzip output.
> (make-pypi-sexp): Likewise, and include the test inputs requirements as native
> inputs in the returned package expression.
>
> * tests/pypi.scm (test-requires.txt): Include a test section in the
> test-requires.txt data.
> (test-requires.txt-beaker): New variable.
> ("parse-requires.txt"): Adapt.
> ("parse-requires.txt - Beaker"): New test.
> ("parse-wheel-metadata, with extras"): Adapt.
> ("parse-wheel-metadata, with extras - Jedi"): Adapt.
> ("pypi->guix-package, no wheel"): Re-indent, and add the expected
> native-inputs.
> ("pypi->guix-package, wheels"): Likewise.
> ("pypi->guix-package, no usable requirement file."): New test.
It seems that the patch does several unrelated things, such as silencing
‘unzip’, improving docstrings, handling inputs other than
‘propagated-inputs’, and correctly parsing wheels.
Could you try to separate these as much as possible to simplify review?
Thank you!
Ludo’.
bug-guix <at> gnu.org:bug#24450; Package guix.
(Wed, 15 May 2019 11:07:01 GMT) Full text and rfc822 format available.Message #43 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: pypi importer outputs strange character series in optional dependency case. Date: Wed, 15 May 2019 13:06:37 +0200
Hi Maxim, I would very much like to see your improvements to the pypi importer to be merged. Have you been able to separate the independent changes as suggested by Ludo? -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 20 May 2019 04:07:02 GMT) Full text and rfc822 format available.Message #46 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 20 May 2019 00:05:59 -0400
[Message part 1 (text/plain, inline)]
Hi Ricardo! Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: > Hi Maxim, > > I would very much like to see your improvements to the pypi importer to > be merged. Have you been able to separate the independent changes as > suggested by Ludo? I'm thrilled that someone has an interest in this :-) I took my time, but finally got around to restructure the changes a bit. I hope it'll be easier to review this time around! Thank you! Maxim
[0001-import-pypi-Do-not-consider-requirements.txt-files.patch (text/x-patch, attachment)]
[0002-import-pypi-Do-not-parse-optional-requirements-from-.patch (text/x-patch, attachment)]
[0003-import-pypi-Improve-parsing-of-requirement-specifica.patch (text/x-patch, attachment)]
[0004-import-pypi-Deduplicate-requirements.patch (text/x-patch, attachment)]
[0005-import-pypi-Support-more-types-of-archives.patch (text/x-patch, attachment)]
[0006-import-pypi-Parse-wheel-METADATA-instead-of-metadata.patch (text/x-patch, attachment)]
[0007-import-pypi-Include-optional-test-inputs-as-native-i.patch (text/x-patch, attachment)]
[0008-import-pypi-Scan-source-archive-to-find-requires.txt.patch (text/x-patch, attachment)]
[0009-import-pypi-Preserve-package-name-case-when-forming-.patch (text/x-patch, attachment)]
[signature.asc (application/pgp-signature, inline)]
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 20 May 2019 15:07:02 GMT) Full text and rfc822 format available.Message #49 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de>, 24450 <at> debbugs.gnu.org Subject: Re: bug#24450: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 20 May 2019 17:05:58 +0200
Hello! Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis: > Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: > >> Hi Maxim, >> >> I would very much like to see your improvements to the pypi importer to >> be merged. Have you been able to separate the independent changes as >> suggested by Ludo? > > I'm thrilled that someone has an interest in this :-) > > I took my time, but finally got around to restructure the changes a > bit. I hope it'll be easier to review this time around! I’ll let Ricardo comment. For my part, I see that it has tests, and that’s enough to make me happy. :-) Thanks for improving the importer! Ludo’.
bug-guix <at> gnu.org:bug#24450; Package guix.
(Wed, 22 May 2019 01:14:02 GMT) Full text and rfc822 format available.Message #52 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de>, 24450 <at> debbugs.gnu.org Subject: Re: bug#24450: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Tue, 21 May 2019 21:13:24 -0400
Greetings! Ludovic Courtès <ludo <at> gnu.org> writes: > Hello! > > Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis: > >> Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: >> >>> Hi Maxim, >>> >>> I would very much like to see your improvements to the pypi importer to >>> be merged. Have you been able to separate the independent changes as >>> suggested by Ludo? >> >> I'm thrilled that someone has an interest in this :-) >> >> I took my time, but finally got around to restructure the changes a >> bit. I hope it'll be easier to review this time around! > > I’ll let Ricardo comment. For my part, I see that it has tests, and > that’s enough to make me happy. :-) OK! > Thanks for improving the importer! My pleasure! One less itch to scratch ;-) Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 27 May 2019 14:49:01 GMT) Full text and rfc822 format available.Message #55 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 27 May 2019 16:48:43 +0200
Hi Maxim, > Subject: [PATCH 1/9] import: pypi: Do not consider requirements.txt files. > > * guix/import/pypi.scm (guess-requirements): Update comment. > [guess-requirements-from-source]: Do not attempt to parse the file > requirements.txt. Streamline logic. Why remove the handling of the requirements.txt? Is it no longer popular enough to expect its availability in the source archives? Please also mention in the commit message that and how you adjusted the tests. You removed the comments from the example requires.txt — are comments no longer permitted in these files? If they are, please don’t include those changes. -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 27 May 2019 15:12:02 GMT) Full text and rfc822 format available.Message #58 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 27 May 2019 17:11:33 +0200
Hi Maxim,
on to patch number 2!
> From 5f79b0502f62bd1dacc8ea143c1dbd9ef7cfc29d Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
> Date: Thu, 28 Mar 2019 00:26:00 -0400
> Subject: [PATCH 2/9] import: pypi: Do not parse optional requirements from
> source.
>
> * guix/import/pypi.scm: Export PARSE-REQUIRES.TXT.
> (guess-requirements): Move the READ-REQUIREMENTS procedure to the top level,
> and rename it to PARSE-REQUIRES.TXT. Move the CLEAN-REQUIREMENT and COMMENT?
> functions inside the READ-REQUIREMENTS procedure.
> (parse-requires.txt): Add a SECTION-HEADER? predicate, and use it to prevent
> parsing optional requirements.
>
> * tests/pypi.scm (test-requires-with-sections): New variable.
> ("parse-requires.txt, with sections"): New test.
> ("pypi->guix-package"): Mute tar output to stdout.
The commit log does not match the changes. CLEAN-REQUIREMENT is now a
top-level procedure, not a local procedure inside of READ-REQUIREMENTS
as reported in the commit message. Which is correct?
> + (call-with-input-file requires.txt
> + (lambda (port)
> + (let loop ((result '()))
> + (let ((line (read-line port)))
> + ;; Stop when a section is encountered, as sections contains optional
Should be “contain”.
> + ;; (extra) requirements. Non-optional requirements must appear
> + ;; before any section is defined.
> + (if (or (eof-object? line) (section-header? line))
> + (reverse result)
> + (cond
> + ((or (string-null? line) (comment? line))
> + (loop result))
> + (else
> + (loop (cons (clean-requirement line)
> + result))))))))))
> +
I think it would be better to use “match” here instead of nested “let”,
“if” and “cond”. At least you can drop the “if” and just use cond.
The loop let and the inner let can be merged.
> +(define (parse-requires.txt requires.txt)
> + "Given REQUIRES.TXT, a Setuptools requires.txt file, return a list of
> +requirement names."
> + ;; This is a very incomplete parser, which job is to select the non-optional
“which” –> “whose”
> + ;; dependencies and strip them out of any version information.
> + ;; Alternatively, we could implement a PEG parser with the (ice-9 peg)
> + ;; library and the requirements grammar defined by PEP-0508
> + ;; (https://www.python.org/dev/peps/pep-0508/).
Let’s remove the sentence starting with “Alternatively…”. We could do
that but we didn’t :)
> + (define (section-header? line)
> + ;; Return #t if the given LINE is a section header, #f otherwise.
> + (let ((trimmed-line (string-trim line)))
> + (and (not (string-null? trimmed-line))
> + (eq? (string-ref trimmed-line 0) #\[))))
> +
How about using string-prefix? instead? This looks more complicated
than it deserves. You can get rid of string-null? and eq? and string-ref
and all that.
Same here:
> + (define (comment? line)
> + ;; Return #t if the given LINE is a comment, #f otherwise.
> + (eq? (string-ref (string-trim line) 0) #\#))
I’d just use string-prefix? here.
> +(define (clean-requirement s)
> + ;; Given a requirement LINE, as can be found in a setuptools requires.txt
> + ;; file, remove everything other than the actual name of the required
> + ;; package, and return it.
> + (string-take s (or (string-index s (lambda (chr)
> + (member chr '(#\space #\> #\= #\<))))
> + (string-length s))))
“string-take” with “string-length” is not very elegant. The char
predicate in string-index could better be a char set:
--8<---------------cut here---------------start------------->8---
(define (clean-requirement s)
(cond
((string-index s (char-set #\space #\> #\= #\<)) => (cut string-take s <>))
(else s)))
--8<---------------cut here---------------end--------------->8---
> ("pypi->guix-package"): Mute tar output to stdout.
Finally, I think it would be better to keep this separate because it’s
really orthogonal to the other changes in this patch.
What do you think?
--
Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 27 May 2019 15:55:02 GMT) Full text and rfc822 format available.Message #61 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 27 May 2019 17:54:39 +0200
Patch number 3!
> From 0c62b541a3e8925b5ca31fe55dbe7536cf95151f Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
> Date: Thu, 28 Mar 2019 00:26:01 -0400
> Subject: [PATCH 3/9] import: pypi: Improve parsing of requirement
> specifications.
>
> The previous solution was fragile and could leave unwanted characters in a
> requirement name, such as '[' or ']'.
Wouldn’t it be sufficient to add [ and ] to the list of forbidden
characters? The tests pass with this implementation of
clean-requirements:
(define (clean-requirements s)
(cond
((string-index s (char-set #\space #\> #\= #\< #\[ #\])) => (cut string-take s <>))
(else s)))
> +(define %requirement-name-regexp
> + ;; Regexp to match the requirement name in a requirement specification.
> +
> + ;; Some grammar, taken from PEP-0508 (see:
> + ;; https://www.python.org/dev/peps/pep-0508/).
> +
> + ;; The unified rule can be expressed as:
> + ;; specification = wsp* ( url_req | name_req ) wsp*
> +
> + ;; where url_req is:
> + ;; url_req = name wsp* extras? wsp* urlspec wsp+ quoted_marker?
> +
> + ;; and where name_req is:
> + ;; name_req = name wsp* extras? wsp* versionspec? wsp* quoted_marker?
> +
> + ;; Thus, we need only matching NAME, which is expressed as:
> + ;; identifer_end = letterOrDigit | (('-' | '_' | '.' )* letterOrDigit)
> + ;; identifier = letterOrDigit identifier_end*
> + ;; name = identifier
> + (let* ((letter-or-digit "[A-Za-z0-9]")
> + (identifier-end (string-append "(" letter-or-digit "|"
> + "[-_.]*" letter-or-digit ")"))
> + (identifier (string-append "^" letter-or-digit identifier-end "*"))
> + (name identifier))
> + (make-regexp name)))
This seems a little too complicated. Translating a grammar into a
regexp is probably not a good idea in general. Since we don’t care
about anything other than the name it seems easier to just chop off
the string tail as soon as we find an invalid character.
--
Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 27 May 2019 15:59:02 GMT) Full text and rfc822 format available.Message #64 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 27 May 2019 17:58:47 +0200
> From 76e4a3150f8126e0b952c6129b6e1371afba80c0 Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> > Date: Thu, 28 Mar 2019 00:26:01 -0400 > Subject: [PATCH 4/9] import: pypi: Deduplicate requirements. > > * guix/import/pypi.scm (parse-requires.txt): Remove potential duplicates. This looks fine to me, but it is subject to changes that I requested to the procedure in my comments to an earlier patch. -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Tue, 28 May 2019 10:24:02 GMT) Full text and rfc822 format available.Message #67 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Tue, 28 May 2019 12:23:53 +0200
On to the next: > From 73e27235cac1275ba7671fd2364325cf5788cb3c Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> > Date: Thu, 28 Mar 2019 00:26:02 -0400 > Subject: [PATCH 5/9] import: pypi: Support more types of archives. > > This change enables the PyPI importer to look for requirements in a source > archive of a different type than "tar.gz" or "tar.bz2". Okay. > * guix/import/pypi.scm: (guess-requirements)[tarball-directory]: Rename to... > [archive-root-directory]: this. Use COMPRESSED-FILED? to determine if an > archive is supported or not. Nitpick: please use “...this.” and leave two spaces between sentences. Typo: it should be COMPRESSED-FILE? > [guess-requirements-from-source]: Adapt to use the new method, and use unzip > to extract ZIP archives. s/method/procedure/ Please also mention that “compute-inputs” has been adjusted. > - (define (tarball-directory url) > - ;; Given the URL of the package's tarball, return the name of the directory > + (define (archive-root-directory url) > + ;; Given the URL of the package's archive, return the name of the directory > ;; that will be created upon decompressing it. If the filetype is not > ;; supported, return #f. > - ;; TODO: Support more archive formats. > - (let ((basename (substring url (+ 1 (string-rindex url #\/))))) > - (cond > - ((string-suffix? ".tar.gz" basename) > - (string-drop-right basename 7)) > - ((string-suffix? ".tar.bz2" basename) > - (string-drop-right basename 8)) > - (else > + (if (compressed-file? url) > + (let ((root-directory (file-sans-extension (basename url)))) > + (if (string=? "tar" (file-extension root-directory)) > + (file-sans-extension root-directory) > + root-directory)) > (begin > - (warning (G_ "Unsupported archive format: \ > -cannot determine package dependencies")) > - #f))))) > + (warning (G_ "Unsupported archive format (~a): \ > +cannot determine package dependencies") (file-extension url)) > + #f))) I think the double application of file-sans-extension and the intermediate variable name “root-directory” for something that is a file is a little confusing, but I don’t have a better proposal (other than to replace file-extension and file-sans-extension with a match expression). > (define (read-wheel-metadata wheel-archive) > ;; Given WHEEL-ARCHIVE, a ZIP Python wheel archive, return the package's > @@ -246,16 +243,20 @@ cannot determine package dependencies")) > (define (guess-requirements-from-source) > ;; Return the package's requirements by guessing them from the source. > - (let ((dirname (tarball-directory source-url))) > + (let ((dirname (archive-root-directory source-url)) > + (extension (file-extension source-url))) > (if (string? dirname) > (call-with-temporary-directory > (lambda (dir) > (let* ((pypi-name (string-take dirname (string-rindex dirname #\-))) > (requires.txt (string-append dirname "/" pypi-name > ".egg-info" "/requires.txt")) > - (exit-code (parameterize ((current-error-port (%make-void-port "rw+")) > - (current-output-port (%make-void-port "rw+"))) > - (system* "tar" "xf" tarball "-C" dir requires.txt)))) > + (exit-code > + (parameterize ((current-error-port (%make-void-port "rw+")) > + (current-output-port (%make-void-port "rw+"))) > + (if (string=? "zip" extension) > + (system* "unzip" archive "-d" dir requires.txt) > + (system* "tar" "xf" archive "-C" dir requires.txt))))) I guess this is why I’m not too happy with this: we’re checking in multiple places if the format is supported but then forget about this again until the next time we need to do something to the file. I wonder if we could do better and answer the question just once. -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Tue, 28 May 2019 11:05:01 GMT) Full text and rfc822 format available.Message #70 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Tue, 28 May 2019 13:04:44 +0200
Patch number 6: > From fb0547ef225103c0f8355a7eccc41e0d028f6563 Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> > Date: Thu, 28 Mar 2019 00:26:03 -0400 > Subject: [PATCH 6/9] import: pypi: Parse wheel METADATA instead of > metadata.json. > With newer Wheel releases, there is no more metadata.json file; the METADATA > file should be used instead (see: https://github.com/pypa/wheel/issues/195). > This change updates our PyPI importer so that it uses the later. Typo: should be “latter” instead of “later”. > * guix/import/pypi.scm (define-module): Remove unnecessary modules and export > the PARSE-WHEEL-METADATA method. Please remove the indentation here. Also, please don’t use “method” (because it’s not); use “procedure” instead. > (parse-wheel-metadata): Add method. Same here. > + (define (requires-dist-header? line) > + ;; Return #t if the given LINE is a Requires-Dist header. > + (regexp-match? (string-match "^Requires-Dist: " line))) > + > + (define (requires-dist-value line) > + (string-drop line (string-length "Requires-Dist: "))) > + > + (define (extra? line) > + ;; Return #t if the given LINE is an "extra" requirement. > + (regexp-match? (string-match "extra == " line))) The use of “regexp-match?” here isn’t strictly necessary as the return value is true-ish anyway. > + (call-with-input-file metadata > + (lambda (port) > + (let loop ((requirements '())) > + (let ((line (read-line port))) > + ;; Stop at the first 'Provides-Extra' section: the non-optional > + ;; requirements appear before the optional ones. > + (if (eof-object? line) > + (reverse (delete-duplicates requirements)) > + (cond > + ((and (requires-dist-header? line) (not (extra? line))) > + (loop (cons (specification->requirement-name > + (requires-dist-value line)) > + requirements))) > + (else > + (loop requirements))))))))) > + As before you can simplify the nested let and merge “if” and "cond“. > (define (read-wheel-metadata wheel-archive) > ;; Given WHEEL-ARCHIVE, a ZIP Python wheel archive, return the package's > - ;; requirements. > + ;; requirements, or #f if the metadata file contained therein couldn't be > + ;; extracted. > (let* ((dirname (wheel-url->extracted-directory wheel-url)) > - (json-file (string-append dirname "/metadata.json"))) > - (and (zero? (system* "unzip" "-q" wheel-archive json-file)) > - (dynamic-wind > - (const #t) > - (lambda () > - (call-with-input-file json-file > - (lambda (port) > - (let* ((metadata (json->scm port)) > - (run_requires (hash-ref metadata "run_requires")) > - (requirements (if run_requires > - (hash-ref (list-ref run_requires 0) > - "requires") > - '()))) > - (map specification->requirement-name requirements))))) > - (lambda () > - (delete-file json-file) > - (rmdir dirname)))))) > + (metadata (string-append dirname "/METADATA"))) > + (call-with-temporary-directory > + (lambda (dir) > + (if (zero? (system* "unzip" "-q" wheel-archive "-d" dir metadata)) > + (parse-wheel-metadata (string-append dir "/" metadata)) > + (begin > + (warning > + (G_ "Failed to extract file: ~a from wheel.~%") metadata) > + #f)))))) The old approach took care of removing the unpacked archive no matter what happened. The new code doesn’t do that. > --- a/tests/pypi.scm > +++ b/tests/pypi.scm Thanks for the tests! -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Tue, 28 May 2019 13:23:02 GMT) Full text and rfc822 format available.Message #73 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Tue, 28 May 2019 15:21:56 +0200
Next up: Seven of Nine, tertiary adjunct of unimatrix zero one:
> From 37e499d5d5d5f690aa0a065c730e13f6a31dd30d Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
> Date: Thu, 28 Mar 2019 23:12:26 -0400
> Subject: [PATCH 7/9] import: pypi: Include optional test inputs as
> native-inputs.
>
> * guix/import/pypi.scm (maybe-inputs): Add INPUT-TYPE argument, and use it.
> (test-section?): New predicate.
> (parse-requires.txt): Collect the optional test inputs, and return them as the
> second element of the returned list.
AFAICT parse-requires.txt now returns a list of pairs, but used to
return a plain list before. Is this correct?
> (define (parse-requires.txt requires.txt)
> - "Given REQUIRES.TXT, a Setuptools requires.txt file, return a list of
> -requirement names."
> - ;; This is a very incomplete parser, which job is to select the non-optional
> - ;; dependencies and strip them out of any version information.
> + "Given REQUIRES.TXT, a Setuptools requires.txt file, return a pair of requirements.
> +
> +The first element of the pair contains the required dependencies while the
> +second the optional test dependencies. Note that currently, optional,
> +non-test dependencies are omitted since these can be difficult or expensive to
> +satisfy."
> +
> + ;; This is a very incomplete parser, which job is to read in the requirement
> + ;; specification lines, and strip them out of any version information.
> ;; Alternatively, we could implement a PEG parser with the (ice-9 peg)
> ;; library and the requirements grammar defined by PEP-0508
> ;; (https://www.python.org/dev/peps/pep-0508/).
Does it really return a pair? Or a list of pairs? Or is it a
two-element list of lists?
> (call-with-input-file requires.txt
> (lambda (port)
> - (let loop ((result '()))
> + (let loop ((required-deps '())
> + (test-deps '())
> + (inside-test-section? #f)
> + (optional? #f))
> (let ((line (read-line port)))
> - ;; Stop when a section is encountered, as sections contains optional
> - ;; (extra) requirements. Non-optional requirements must appear
> - ;; before any section is defined.
> - (if (or (eof-object? line) (section-header? line))
> + (if (eof-object? line)
> ;; Duplicates can occur, since the same requirement can be
> ;; listed multiple times with different conditional markers, e.g.
> ;; pytest >= 3 ; python_version >= "3.3"
> ;; pytest < 3 ; python_version < "3.3"
> - (reverse (delete-duplicates result))
> + (map (compose reverse delete-duplicates)
> + (list required-deps test-deps))
Looks like a list of lists to me. “delete-duplicates” now won’t delete
a name that is in both “required-deps” as well as in “test-deps”. Is
this acceptable?
Personally, I’m not a fan of using data structures for returning
multiple values, because we can simply return multiple values.
Or we could have more than just strings. The meaning of these strings
is provided by the bin into which they are thrown — either
“required-deps” or “test-deps”. It could be an option to collect tagged
values instead and have the caller deal with filtering.
> (define (parse-wheel-metadata metadata)
> - "Given METADATA, a Wheel metadata file, return a list of requirement names."
> + "Given METADATA, a Wheel metadata file, return a pair of requirements.
> +
> +The first element of the pair contains the required dependencies while the second the optional
> +test dependencies. Note that currently, optional, non-test dependencies are
> +omitted since these can be difficult or expensive to satisfy."
> ;; METADATA is a RFC-2822-like, header based file.
This sounds like this is going to duplicate the previous procedures.
> (define (requires-dist-header? line)
> ;; Return #t if the given LINE is a Requires-Dist header.
> - (regexp-match? (string-match "^Requires-Dist: " line)))
> + (string-match "^Requires-Dist: " line))
>
> (define (requires-dist-value line)
> (string-drop line (string-length "Requires-Dist: ")))
>
> (define (extra? line)
> ;; Return #t if the given LINE is an "extra" requirement.
> - (regexp-match? (string-match "extra == " line)))
> + (string-match "extra == '(.*)'" line))
These hunks should be part of the previous patch where they were
introduced. (See my comments there about “regexp-match?”.)
> + (define (test-requirement? line)
> + (let ((extra-label (match:substring (extra? line) 1)))
> + (and extra-label (test-section? extra-label))))
You can use “and=>” instead of binding a name:
(and=> (match:substring (extra? line) 1) test-section?)
> (call-with-input-file metadata
> (lambda (port)
> - (let loop ((requirements '()))
> + (let loop ((required-deps '())
> + (test-deps '()))
> (let ((line (read-line port)))
> - ;; Stop at the first 'Provides-Extra' section: the non-optional
> - ;; requirements appear before the optional ones.
> (if (eof-object? line)
> - (reverse (delete-duplicates requirements))
> + (map (compose reverse delete-duplicates)
> + (list required-deps test-deps))
> (cond
> ((and (requires-dist-header? line) (not (extra? line)))
> (loop (cons (specification->requirement-name
> (requires-dist-value line))
> - requirements)))
> + required-deps)
> + test-deps))
> + ((and (requires-dist-header? line) (test-requirement? line))
> + (loop required-deps
> + (cons (specification->requirement-name (requires-dist-value line))
> + test-deps)))
> (else
> - (loop requirements)))))))))
> + (loop required-deps test-deps))))))))) ;skip line
Could you refactor this so that the other parser can be reused? If not,
the same comments about “if” and “cond” and the use of pairs/lists
instead of “values” applies here.
> (define (guess-requirements source-url wheel-url archive)
> - "Given SOURCE-URL, WHEEL-URL and a ARCHIVE of the package, return a list
> + "Given SOURCE-URL, WHEEL-URL and an ARCHIVE of the package, return a list
> of the required packages specified in the requirements.txt file. ARCHIVE will
> be extracted in a temporary directory."
>
> @@ -244,7 +289,10 @@ cannot determine package dependencies") (file-extension url))
> (metadata (string-append dirname "/METADATA")))
> (call-with-temporary-directory
> (lambda (dir)
> - (if (zero? (system* "unzip" "-q" wheel-archive "-d" dir metadata))
> + (if (zero?
> + (parameterize ((current-error-port (%make-void-port "rw+"))
> + (current-output-port (%make-void-port "rw+")))
> + (system* "unzip" wheel-archive "-d" dir metadata)))
> (parse-wheel-metadata (string-append dir "/" metadata))
> (begin
> (warning
> @@ -283,32 +331,41 @@ cannot determine package dependencies") (file-extension url))
> (warning
> (G_ "Failed to extract file: ~a from source.~%")
> requires.txt)
> - '())))))
> - '())))
> + (list '() '()))))))
> + (list '() '()))))
I would like to see cosmetic changes like these three hunks in separate
commits.
> (define (compute-inputs source-url wheel-url archive)
> - "Given the SOURCE-URL of an already downloaded ARCHIVE, return a list of
> -name/variable pairs describing the required inputs of this package. Also
> + "Given the SOURCE-URL and WHEEL-URL of an already downloaded ARCHIVE, return
> +a pair of lists, each consisting of a list of name/variable pairs, for the
> +propagated inputs and the native inputs, respectively. Also
> return the unaltered list of upstream dependency names."
> - (let ((dependencies
> - (remove (cut string=? "argparse" <>)
> - (guess-requirements source-url wheel-url archive))))
> - (values (sort
> - (map (lambda (input)
> - (let ((guix-name (python->package-name input)))
> - (list guix-name (list 'unquote (string->symbol guix-name)))))
> - dependencies)
> - (lambda args
> - (match args
> - (((a _ ...) (b _ ...))
> - (string-ci<? a b)))))
> - dependencies)))
> +
> + (define (strip-argparse deps)
> + (remove (cut string=? "argparse" <>) deps))
> +
> + (define (requirement->package-name/sort deps)
> + (sort
> + (map (lambda (input)
> + (let ((guix-name (python->package-name input)))
> + (list guix-name (list 'unquote (string->symbol guix-name)))))
> + deps)
> + (lambda args
> + (match args
> + (((a _ ...) (b _ ...))
> + (string-ci<? a b))))))
> +
> + (define process-requirements
> + (compose requirement->package-name/sort strip-argparse))
> +
> + (let ((dependencies (guess-requirements source-url wheel-url archive)))
> + (values (map process-requirements dependencies)
> + (concatenate dependencies))))
Giving names to these processing steps is fine and improves clarity, but
I’m not so comfortable about returning ad-hoc pairs *and* multiple
values (like above).
> + (match guix-dependencies
> + ((required-inputs test-inputs)
> + (values
> + `(package
> + (name ,(python->package-name name))
> + (version ,version)
> + (source (origin
> + (method url-fetch)
> + ;; Sometimes 'pypi-uri' doesn't quite work due to mixed
> + ;; cases in NAME, for instance, as is the case with
> + ;; "uwsgi". In that case, fall back to a full URL.
> + (uri (pypi-uri ,(string-downcase name) version))
> + (sha256
> + (base32
> + ,(guix-hash-url temp)))))
> + (build-system python-build-system)
> + ,@(maybe-inputs required-inputs 'propagated-inputs)
> + ,@(maybe-inputs test-inputs 'native-inputs)
> + (home-page ,home-page)
> + (synopsis ,synopsis)
> + (description ,description)
> + (license ,(license->symbol license)))
> + ;; Flatten the nested lists and return the upstream
> + ;; dependencies.
> + upstream-dependencies))))))))
I don’t see anything being flattened here?
--
Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Tue, 28 May 2019 14:50:01 GMT) Full text and rfc822 format available.Message #76 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Tue, 28 May 2019 16:48:57 +0200
> From cfde6e09f8f8c692fe252d76ed27e8c50a9e5377 Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> > Date: Sat, 30 Mar 2019 23:13:26 -0400 > Subject: [PATCH 8/9] import: pypi: Scan source archive to find requires.txt > file. > * guix/import/pypi.scm (use-modules): Use invoke from (guix build utils). > (guess-requirements)[archive-root-directory]: Remove procedure. Oh, I guess I reviewed this procedure in vain :( Please modify the commits so that added procedures are not removed in later commits. This is easier on the reviewer and makes for a clearer commit history. > (define (guess-requirements-from-source) > ;; Return the package's requirements by guessing them from the source. > - (let ((dirname (archive-root-directory source-url)) > - (extension (file-extension source-url))) > - (if (string? dirname) > - (call-with-temporary-directory > - (lambda (dir) > - (let* ((pypi-name (string-take dirname (string-rindex dirname #\-))) > - (requires.txt (string-append dirname "/" pypi-name > - ".egg-info" "/requires.txt")) > - (exit-code > - (parameterize ((current-error-port (%make-void-port "rw+")) > - (current-output-port (%make-void-port "rw+"))) > - (if (string=? "zip" extension) > - (system* "unzip" archive "-d" dir requires.txt) > - (system* "tar" "xf" archive "-C" dir requires.txt))))) > - (if (zero? exit-code) > - (parse-requires.txt (string-append dir "/" requires.txt)) > - (begin > - (warning > - (G_ "Failed to extract file: ~a from source.~%") > - requires.txt) > - (list '() '())))))) > + (if (compressed-file? source-url) > + (call-with-temporary-directory > + (lambda (dir) > + (parameterize ((current-error-port (%make-void-port "rw+")) > + (current-output-port (%make-void-port "rw+"))) > + (if (string=? "zip" (file-extension source-url)) > + (invoke "unzip" archive "-d" dir) > + (invoke "tar" "xf" archive "-C" dir))) > + (let ((requires.txt-files > + (find-files dir (lambda (abs-file-name _) > + (string-match "\\.egg-info/requires.txt$" > + abs-file-name))))) > + (if (> (length requires.txt-files) 0) Let’s work on the empty list directly. Here “match” would be better. > + (begin > + (parse-requires.txt (first requires.txt-files))) No need for “begin” here. > + (begin (warning (G_ "Cannot guess requirements from source archive:\ > + no requires.txt file found.~%")) > + (list '() '())))))) I know that this is from an earlier commit, but I don’t like the look of “(list '() '())” at all :) > + (begin > + (warning (G_ "Unsupported archive format; \ > +cannot determine package dependencies from source archive: ~a~%") > + (basename source-url)) > (list '() '())))) Same here. Certainly there’s a better return value. -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Tue, 28 May 2019 14:54:01 GMT) Full text and rfc822 format available.Message #79 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Tue, 28 May 2019 16:53:17 +0200
And finally: Number 9! > From 1290f9d1f0d594fdd4723d76b94116be25da9dd5 Mon Sep 17 00:00:00 2001 > From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> > Date: Sat, 30 Mar 2019 20:27:35 -0400 > Subject: [PATCH 9/9] import: pypi: Preserve package name case when forming > pypi-uri. > > Fixes issue: #33046. Please change this to: Fixes <https://bugs.gnu.org/33046>. > * guix/build-system/python.scm (pypi-uri): Update the host URI to > "files.pythonhosted.org". > * guix/import/pypi.scm (make-pypi-sexp): Preserve the package name case when > the source URL calls for it. Is the first change to use files.pythonhosted.org required to fix this? Or is this unrelated? If it is required this looks fine to me. Thank you! -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Thu, 30 May 2019 02:25:02 GMT) Full text and rfc822 format available.Message #82 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Wed, 29 May 2019 22:24:13 -0400
Hello Ricardo! Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: > And finally: Number 9! > >> From 1290f9d1f0d594fdd4723d76b94116be25da9dd5 Mon Sep 17 00:00:00 2001 >> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> >> Date: Sat, 30 Mar 2019 20:27:35 -0400 >> Subject: [PATCH 9/9] import: pypi: Preserve package name case when forming >> pypi-uri. >> >> Fixes issue: #33046. > > Please change this to: > > Fixes <https://bugs.gnu.org/33046>. > >> * guix/build-system/python.scm (pypi-uri): Update the host URI to >> "files.pythonhosted.org". >> * guix/import/pypi.scm (make-pypi-sexp): Preserve the package name case when >> the source URL calls for it. > > Is the first change to use files.pythonhosted.org required to fix this? > Or is this unrelated? > > If it is required this looks fine to me. > > Thank you! Thank you for this thorough review! I'll need some time to go through it, and an upcoming travel will delay it some more, so don't fret if you don't hear back from me before a couple days. Sorry! I'll keep you posted. Thanks a bunch! Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 10 Jun 2019 02:12:02 GMT) Full text and rfc822 format available.Message #85 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 10 Jun 2019 11:10:58 +0900
Hello Ricardo!
Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes:
> Hi Maxim,
>
>> Subject: [PATCH 1/9] import: pypi: Do not consider requirements.txt files.
>>
>> * guix/import/pypi.scm (guess-requirements): Update comment.
>> [guess-requirements-from-source]: Do not attempt to parse the file
>> requirements.txt. Streamline logic.
>
> Why remove the handling of the requirements.txt? Is it no longer
> popular enough to expect its availability in the source archives?
>
> Please also mention in the commit message that and how you adjusted the
> tests.
The commit message now explains the above:
import: pypi: Do not consider requirements.txt files.
PyPI packages are mandated to have a setup.py file, which contains a listing
of the required dependencies. The setuptools/distutils machinery embed
metadata in the archives they produce, which contains this information. There
is no need nor gain to collect the requirements from a "requirements.txt"
file, as it is not the true record of dependencies for PyPI packages and may
contain extraneous requirements or not exist at all.
* guix/import/pypi.scm (guess-requirements): Update comment.
[guess-requirements-from-source]: Do not attempt to parse the file
requirements.txt. Streamline logic.
* tests/pypi.scm (test-requires.txt): Rename from test-requirements, to hint
at the file being tested.
("pypi->guix-package"): Adapt so that the fake package contains a requires.txt
file rather than a requirements.txt file.
("pypi->guix-package, wheels"): Likewise.
> You removed the comments from the example requires.txt — are
> comments no longer permitted in these files? If they are, please don’t
> include those changes.
The comments are now preserved. Thank you!
Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 10 Jun 2019 03:31:01 GMT) Full text and rfc822 format available.Message #88 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 10 Jun 2019 12:30:47 +0900
Hello again!
Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes:
> Hi Maxim,
>
> on to patch number 2!
Yay!
>> From 5f79b0502f62bd1dacc8ea143c1dbd9ef7cfc29d Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
>> Date: Thu, 28 Mar 2019 00:26:00 -0400
>> Subject: [PATCH 2/9] import: pypi: Do not parse optional requirements from
>> source.
>>
>> * guix/import/pypi.scm: Export PARSE-REQUIRES.TXT.
>> (guess-requirements): Move the READ-REQUIREMENTS procedure to the top level,
>> and rename it to PARSE-REQUIRES.TXT. Move the CLEAN-REQUIREMENT and COMMENT?
>> functions inside the READ-REQUIREMENTS procedure.
>> (parse-requires.txt): Add a SECTION-HEADER? predicate, and use it to prevent
>> parsing optional requirements.
>>
>> * tests/pypi.scm (test-requires-with-sections): New variable.
>> ("parse-requires.txt, with sections"): New test.
>> ("pypi->guix-package"): Mute tar output to stdout.
>
> The commit log does not match the changes. CLEAN-REQUIREMENT is now a
> top-level procedure, not a local procedure inside of READ-REQUIREMENTS
> as reported in the commit message. Which is correct?
Fixed.
>> + (call-with-input-file requires.txt
>> + (lambda (port)
>> + (let loop ((result '()))
>> + (let ((line (read-line port)))
>> + ;; Stop when a section is encountered, as sections contains optional
>
> Should be “contain”.
Fixed.
>> + ;; (extra) requirements. Non-optional requirements must appear
>> + ;; before any section is defined.
>> + (if (or (eof-object? line) (section-header? line))
>> + (reverse result)
>> + (cond
>> + ((or (string-null? line) (comment? line))
>> + (loop result))
>> + (else
>> + (loop (cons (clean-requirement line)
>> + result))))))))))
>> +
>
> I think it would be better to use “match” here instead of nested “let”,
> “if” and “cond”. At least you can drop the “if” and just use cond.
>
> The loop let and the inner let can be merged.
I'm not sure I understand; wouldn't merging the named let with the plain
let mean adding an extra LINE argument to my LOOP procedure? I don't
want that.
Also, how could the above code be expressed using "match"? I'm using
predicates which tests for (special) characters in a string; I don't see
how the more primitive pattern language of "match" will enable me to do
the same.
>> +(define (parse-requires.txt requires.txt)
>> + "Given REQUIRES.TXT, a Setuptools requires.txt file, return a list of
>> +requirement names."
>> + ;; This is a very incomplete parser, which job is to select the non-optional
>
> “which” –> “whose”
Fixed, with due diligence reading on English grammar ;-)
>> + ;; dependencies and strip them out of any version information.
>> + ;; Alternatively, we could implement a PEG parser with the (ice-9 peg)
>> + ;; library and the requirements grammar defined by PEP-0508
>> + ;; (https://www.python.org/dev/peps/pep-0508/).
>
> Let’s remove the sentence starting with “Alternatively…”. We could do
> that but we didn’t :)
Alright; done!
>> + (define (section-header? line)
>> + ;; Return #t if the given LINE is a section header, #f otherwise.
>> + (let ((trimmed-line (string-trim line)))
>> + (and (not (string-null? trimmed-line))
>> + (eq? (string-ref trimmed-line 0) #\[))))
>> +
>
> How about using string-prefix? instead? This looks more complicated
> than it deserves. You can get rid of string-null? and eq? and string-ref
> and all that.
>
> Same here:
>
>> + (define (comment? line)
>> + ;; Return #t if the given LINE is a comment, #f otherwise.
>> + (eq? (string-ref (string-trim line) 0) #\#))
>
> I’d just use string-prefix? here.
Neat! Adjusted, for both counts.
>> +(define (clean-requirement s)
>> + ;; Given a requirement LINE, as can be found in a setuptools requires.txt
>> + ;; file, remove everything other than the actual name of the required
>> + ;; package, and return it.
>> + (string-take s (or (string-index s (lambda (chr)
>> + (member chr '(#\space #\> #\= #\<))))
>> + (string-length s))))
>
> “string-take” with “string-length” is not very elegant. The char
> predicate in string-index could better be a char set:
>
> (define (clean-requirement s)
> (cond
> ((string-index s (char-set #\space #\> #\= #\<)) => (cut string-take s <>))
> (else s)))
That's nicer, thanks!
>> ("pypi->guix-package"): Mute tar output to stdout.
>
> Finally, I think it would be better to keep this separate because it’s
> really orthogonal to the other changes in this patch.
OK, done!
> What do you think?
All good points; I just don't understand the one about using a match
and/or merging the regular "let" with the named "let".
Thanks,
Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 10 Jun 2019 08:33:02 GMT) Full text and rfc822 format available.Message #91 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 10 Jun 2019 17:32:01 +0900
Hello!
Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes:
> Patch number 3!
Yay!
>> From 0c62b541a3e8925b5ca31fe55dbe7536cf95151f Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
>> Date: Thu, 28 Mar 2019 00:26:01 -0400
>> Subject: [PATCH 3/9] import: pypi: Improve parsing of requirement
>> specifications.
>>
>> The previous solution was fragile and could leave unwanted characters in a
>> requirement name, such as '[' or ']'.
>
> Wouldn’t it be sufficient to add [ and ] to the list of forbidden
> characters? The tests pass with this implementation of
> clean-requirements:
>
> (define (clean-requirements s)
> (cond
> ((string-index s (char-set #\space #\> #\= #\< #\[ #\])) => (cut string-take s <>))
> (else s)))
Indeed this would be sufficient to make the tests pass, but the tests
don't cover all the cases; as an example, consider:
--8<---------------cut here---------------start------------->8---
argparse;python_version<"2.7"
--8<---------------cut here---------------end--------------->8---
While we could make it work with the current logic by adding more
invalid characters (such as ';' here) to the character set, it seems
less error prone to use the upstream provided regex to match a package
name. [0]
>> +(define %requirement-name-regexp
>> + ;; Regexp to match the requirement name in a requirement specification.
>> +
>> + ;; Some grammar, taken from PEP-0508 (see:
>> + ;; https://www.python.org/dev/peps/pep-0508/).
>> +
>> + ;; The unified rule can be expressed as:
>> + ;; specification = wsp* ( url_req | name_req ) wsp*
>> +
>> + ;; where url_req is:
>> + ;; url_req = name wsp* extras? wsp* urlspec wsp+ quoted_marker?
>> +
>> + ;; and where name_req is:
>> + ;; name_req = name wsp* extras? wsp* versionspec? wsp* quoted_marker?
>> +
>> + ;; Thus, we need only matching NAME, which is expressed as:
>> + ;; identifer_end = letterOrDigit | (('-' | '_' | '.' )* letterOrDigit)
>> + ;; identifier = letterOrDigit identifier_end*
>> + ;; name = identifier
>> + (let* ((letter-or-digit "[A-Za-z0-9]")
>> + (identifier-end (string-append "(" letter-or-digit "|"
>> + "[-_.]*" letter-or-digit ")"))
>> + (identifier (string-append "^" letter-or-digit identifier-end "*"))
>> + (name identifier))
>> + (make-regexp name)))
>
> This seems a little too complicated. Translating a grammar into a
> regexp is probably not a good idea in general. Since we don’t care
> about anything other than the name it seems easier to just chop off
> the string tail as soon as we find an invalid character.
While I agree that a regexp is a bigger hammer than basic string
manipulation, I see some merit to it here:
1) We can be assured of conformance with upstream, again, per PEP-0508.
2) It is easier to extend; we might want to add parsing for the version
spec in order to disregard dependencies specified for Python < 3, for
example.
The use of the PEP-0508 grammar to define the regexp is useful to detail
in a more human-friendly language the components of the regexp. We
could have otherwise used the more cryptic regexp for Python
distribution names:
--8<---------------cut here---------------start------------->8---
^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$
--8<---------------cut here---------------end--------------->8---
So I guess that what I'm saying is that I prefer this approach to using
string-index with invalid characters, for the reasons above.
[0] https://www.python.org/dev/peps/pep-0508/
Thanks!
Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 10 Jun 2019 09:13:01 GMT) Full text and rfc822 format available.Message #94 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 10 Jun 2019 11:12:03 +0200
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes: > While I agree that a regexp is a bigger hammer than basic string > manipulation, I see some merit to it here: > > 1) We can be assured of conformance with upstream, again, per PEP-0508. > 2) It is easier to extend; we might want to add parsing for the version > spec in order to disregard dependencies specified for Python < 3, for > example. > > The use of the PEP-0508 grammar to define the regexp is useful to detail > in a more human-friendly language the components of the regexp. We > could have otherwise used the more cryptic regexp for Python > distribution names: > > --8<---------------cut here---------------start------------->8--- > ^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$ > --8<---------------cut here---------------end--------------->8--- > > So I guess that what I'm saying is that I prefer this approach to using > string-index with invalid characters, for the reasons above. > > [0] https://www.python.org/dev/peps/pep-0508/ Okay, sounds good. Please make sure to note this in a comment, so that I won’t be asking myself this same question in a year :) -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 10 Jun 2019 09:24:02 GMT) Full text and rfc822 format available.Message #97 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 10 Jun 2019 11:23:10 +0200
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:
>>> + ;; (extra) requirements. Non-optional requirements must appear
>>> + ;; before any section is defined.
>>> + (if (or (eof-object? line) (section-header? line))
>>> + (reverse result)
>>> + (cond
>>> + ((or (string-null? line) (comment? line))
>>> + (loop result))
>>> + (else
>>> + (loop (cons (clean-requirement line)
>>> + result))))))))))
>>> +
>>
>> I think it would be better to use “match” here instead of nested “let”,
>> “if” and “cond”. At least you can drop the “if” and just use cond.
>>
>> The loop let and the inner let can be merged.
>
> I'm not sure I understand; wouldn't merging the named let with the plain
> let mean adding an extra LINE argument to my LOOP procedure? I don't
> want that.
Let’s forget about merging the nested “let”, because you would indeed
need to change a few more things. It’s fine to keep that as it is. But
(if … (cond …)) is not pretty. At least it could be done in one “cond”:
(cond
((or (eof-object? line) (section-header? line))
(reverse result))
((or (string-null? line) (comment? line))
(loop result))
(else
(loop (cons (clean-requirement line)
result))))
> Also, how could the above code be expressed using "match"? I'm using
> predicates which tests for (special) characters in a string; I don't see
> how the more primitive pattern language of "match" will enable me to do
> the same.
“match” has support for predicates, so you could do something like this:
(match line
((or (eof-object) (? section-header?))
(reverse result))
((or '() (? comment?))
(loop result))
(_ (loop (cons (clean-requirement line) result))))
This allows you to match “eof-object” and '() directly. Whenever I see
“string-null?” I think it might be better to “match” on the empty list
directly.
But really, that’s up to you. I only feel strongly about avoiding “(if
… (cond …))”.
--
Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 10 Jun 2019 13:32:02 GMT) Full text and rfc822 format available.Message #100 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 10 Jun 2019 22:30:45 +0900
Hello again!
Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes:
> On to the next:
>
>> From 73e27235cac1275ba7671fd2364325cf5788cb3c Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
>> Date: Thu, 28 Mar 2019 00:26:02 -0400
>> Subject: [PATCH 5/9] import: pypi: Support more types of archives.
>>
>> This change enables the PyPI importer to look for requirements in a source
>> archive of a different type than "tar.gz" or "tar.bz2".
>
> Okay.
>
>> * guix/import/pypi.scm: (guess-requirements)[tarball-directory]: Rename to...
>> [archive-root-directory]: this. Use COMPRESSED-FILED? to determine if an
>> archive is supported or not.
>
> Nitpick: please use “...this.” and leave two spaces between sentences.
Done.
> Typo: it should be COMPRESSED-FILE?
Fixed.
>> [guess-requirements-from-source]: Adapt to use the new method, and use unzip
>> to extract ZIP archives.
>
> s/method/procedure/
Done.
> Please also mention that “compute-inputs” has been adjusted.
Done.
>> - (define (tarball-directory url)
>> - ;; Given the URL of the package's tarball, return the name of the directory
>> + (define (archive-root-directory url)
>> + ;; Given the URL of the package's archive, return the name of the directory
>> ;; that will be created upon decompressing it. If the filetype is not
>> ;; supported, return #f.
>> - ;; TODO: Support more archive formats.
>> - (let ((basename (substring url (+ 1 (string-rindex url #\/)))))
>> - (cond
>> - ((string-suffix? ".tar.gz" basename)
>> - (string-drop-right basename 7))
>> - ((string-suffix? ".tar.bz2" basename)
>> - (string-drop-right basename 8))
>> - (else
>> + (if (compressed-file? url)
>> + (let ((root-directory (file-sans-extension (basename url))))
>> + (if (string=? "tar" (file-extension root-directory))
>> + (file-sans-extension root-directory)
>> + root-directory))
>> (begin
>> - (warning (G_ "Unsupported archive format: \
>> -cannot determine package dependencies"))
>> - #f)))))
>> + (warning (G_ "Unsupported archive format (~a): \
>> +cannot determine package dependencies") (file-extension url))
>> + #f)))
>
> I think the double application of file-sans-extension and the
> intermediate variable name “root-directory” for something that is a file
> is a little confusing, but I don’t have a better proposal (other than to
> replace file-extension and file-sans-extension with a match expression).
Done, w.r.t. using "match":
--8<---------------cut here---------------start------------->8---
@@ -198,10 +198,12 @@ be extracted in a temporary directory."
;; that will be created upon decompressing it. If the filetype is not
;; supported, return #f.
(if (compressed-file? url)
- (let ((root-directory (file-sans-extension (basename url))))
- (if (string=? "tar" (file-extension root-directory))
- (file-sans-extension root-directory)
- root-directory))
+ (match (file-sans-extension (basename url))
+ (root-directory
+ (match (file-extension root-directory)
+ ("tar"
+ (file-sans-extension root-directory))
+ (_ root-directory))))
(begin
(warning (G_ "Unsupported archive format (~a): \
cannot determine package dependencies") (file-extension url))
--8<---------------cut here---------------end--------------->8---
>> (define (read-wheel-metadata wheel-archive)
>> ;; Given WHEEL-ARCHIVE, a ZIP Python wheel archive, return the package's
>> @@ -246,16 +243,20 @@ cannot determine package dependencies"))
>
>> (define (guess-requirements-from-source)
>> ;; Return the package's requirements by guessing them from the source.
>> - (let ((dirname (tarball-directory source-url)))
>> + (let ((dirname (archive-root-directory source-url))
>> + (extension (file-extension source-url)))
>> (if (string? dirname)
>> (call-with-temporary-directory
>> (lambda (dir)
>> (let* ((pypi-name (string-take dirname (string-rindex dirname #\-)))
>> (requires.txt (string-append dirname "/" pypi-name
>> ".egg-info" "/requires.txt"))
>> - (exit-code (parameterize ((current-error-port (%make-void-port "rw+"))
>> - (current-output-port (%make-void-port "rw+")))
>> - (system* "tar" "xf" tarball "-C" dir requires.txt))))
>> + (exit-code
>> + (parameterize ((current-error-port (%make-void-port "rw+"))
>> + (current-output-port (%make-void-port "rw+")))
>> + (if (string=? "zip" extension)
>> + (system* "unzip" archive "-d" dir requires.txt)
>> + (system* "tar" "xf" archive "-C" dir requires.txt)))))
>
> I guess this is why I’m not too happy with this: we’re checking in
> multiple places if the format is supported but then forget about this
> again until the next time we need to do something to the file.
I don't see much of a problem with the current design since there are
two questions being answered:
1) What should be the directory name of the extracted package (retrieved
from the base name of the archive).
2) What extractor should be used (zip vs tar).
These two questions are orthogonal, and that the same primitive get used
to answer both is an implementation, or rather, an optimization detail.
> I wonder if we could do better and answer the question just once.
The questions are different :-). We could optimize, but that would be at
the price of expressiveness (squash the two questions into one solving
space).
What do you think?
Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 10 Jun 2019 20:14:01 GMT) Full text and rfc822 format available.Message #103 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 10 Jun 2019 22:13:38 +0200
Hi Maxim,
thanks for your patience in addressing my comments. I appreciate it!
>> I think the double application of file-sans-extension and the
>> intermediate variable name “root-directory” for something that is a file
>> is a little confusing, but I don’t have a better proposal (other than to
>> replace file-extension and file-sans-extension with a match expression).
>
> Done, w.r.t. using "match":
>
> --8<---------------cut here---------------start------------->8---
> @@ -198,10 +198,12 @@ be extracted in a temporary directory."
> ;; that will be created upon decompressing it. If the filetype is not
> ;; supported, return #f.
> (if (compressed-file? url)
> - (let ((root-directory (file-sans-extension (basename url))))
> - (if (string=? "tar" (file-extension root-directory))
> - (file-sans-extension root-directory)
> - root-directory))
> + (match (file-sans-extension (basename url))
> + (root-directory
> + (match (file-extension root-directory)
> + ("tar"
> + (file-sans-extension root-directory))
> + (_ root-directory))))
> (begin
> (warning (G_ "Unsupported archive format (~a): \
> cannot determine package dependencies") (file-extension url))
> --8<---------------cut here---------------end--------------->8---
The first application of “match” matches anything. What I had in mind
was really a slightly different approach, namely to split up the “url”
string at dots and then match the resulting list of strings.
Something like this:
(match (string-split "hello.tar.gz" #\.)
((base "tar" (or "bz2" "gz")) base)
((base ext) base))
> I don't see much of a problem with the current design since there are
> two questions being answered:
>
> 1) What should be the directory name of the extracted package (retrieved
> from the base name of the archive).
> 2) What extractor should be used (zip vs tar).
>
> These two questions are orthogonal, and that the same primitive get used
> to answer both is an implementation, or rather, an optimization detail.
>
>> I wonder if we could do better and answer the question just once.
>
> The questions are different :-). We could optimize, but that would be at
> the price of expressiveness (squash the two questions into one solving
> space).
Okay. I guess I’m too picky :)
I’d be happy if we could move the checks to tiny named procedures, but
it’s probably fine the way it is.
Thanks!
--
Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Tue, 11 Jun 2019 00:41:02 GMT) Full text and rfc822 format available.Message #106 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Tue, 11 Jun 2019 09:39:48 +0900
Hello!
Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes:
> Patch number 6:
>
>> From fb0547ef225103c0f8355a7eccc41e0d028f6563 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
>> Date: Thu, 28 Mar 2019 00:26:03 -0400
>> Subject: [PATCH 6/9] import: pypi: Parse wheel METADATA instead of
>> metadata.json.
>
>> With newer Wheel releases, there is no more metadata.json file; the METADATA
>> file should be used instead (see: https://github.com/pypa/wheel/issues/195).
>
>> This change updates our PyPI importer so that it uses the later.
>
> Typo: should be “latter” instead of “later”.
Fixed.
>> * guix/import/pypi.scm (define-module): Remove unnecessary modules and export
>> the PARSE-WHEEL-METADATA method.
>
> Please remove the indentation here. Also, please don’t use “method”
> (because it’s not); use “procedure” instead.
Done. Thanks for fixing my terminology :-).
>> (parse-wheel-metadata): Add method.
>
> Same here.
Done.
>> + (define (requires-dist-header? line)
>> + ;; Return #t if the given LINE is a Requires-Dist header.
>> + (regexp-match? (string-match "^Requires-Dist: " line)))
>> +
>> + (define (requires-dist-value line)
>> + (string-drop line (string-length "Requires-Dist: ")))
>> +
>> + (define (extra? line)
>> + ;; Return #t if the given LINE is an "extra" requirement.
>> + (regexp-match? (string-match "extra == " line)))
>
> The use of “regexp-match?” here isn’t strictly necessary as the return
> value is true-ish anyway.
Done.
>> + (call-with-input-file metadata
>> + (lambda (port)
>> + (let loop ((requirements '()))
>> + (let ((line (read-line port)))
>> + ;; Stop at the first 'Provides-Extra' section: the non-optional
>> + ;; requirements appear before the optional ones.
>> + (if (eof-object? line)
>> + (reverse (delete-duplicates requirements))
>> + (cond
>> + ((and (requires-dist-header? line) (not (extra? line)))
>> + (loop (cons (specification->requirement-name
>> + (requires-dist-value line))
>> + requirements)))
>> + (else
>> + (loop requirements)))))))))
>> +
>
> As before you can simplify the nested let and merge “if” and "cond“.
Oh, I get it now, I think:
--8<---------------cut here---------------start------------->8---
(call-with-input-file metadata
(lambda (port)
(let loop ((requirements '()))
- (let ((line (read-line port)))
- ;; Stop at the first 'Provides-Extra' section: the non-optional
- ;; requirements appear before the optional ones.
- (if (eof-object? line)
- (reverse (delete-duplicates requirements))
- (cond
- ((and (requires-dist-header? line) (not (extra? line)))
- (loop (cons (specification->requirement-name
- (requires-dist-value line))
- requirements)))
- (else
- (loop requirements)))))))))
+ (match (read-line port)
+ (line
+ ;; Stop at the first 'Provides-Extra' section: the non-optional
+ ;; requirements appear before the optional ones.
+ (cond
+ ((eof-object? line)
+ (reverse (delete-duplicates requirements)))
+ ((and (requires-dist-header? line) (not (extra? line)))
+ (loop (cons (specification->requirement-name
+ (requires-dist-value line))
+ requirements)))
+ (else
+ (loop requirements)))))))))
(define (guess-requirements source-url wheel-url archive)
"Given SOURCE-URL, WHEEL-URL and a ARCHIVE of the package, return a list
--8<---------------cut here---------------end--------------->8---
>> (define (read-wheel-metadata wheel-archive)
>> ;; Given WHEEL-ARCHIVE, a ZIP Python wheel archive, return the package's
>> - ;; requirements.
>> + ;; requirements, or #f if the metadata file contained therein couldn't be
>> + ;; extracted.
>> (let* ((dirname (wheel-url->extracted-directory wheel-url))
>> - (json-file (string-append dirname "/metadata.json")))
>> - (and (zero? (system* "unzip" "-q" wheel-archive json-file))
>> - (dynamic-wind
>> - (const #t)
>> - (lambda ()
>> - (call-with-input-file json-file
>> - (lambda (port)
>> - (let* ((metadata (json->scm port))
>> - (run_requires (hash-ref metadata "run_requires"))
>> - (requirements (if run_requires
>> - (hash-ref (list-ref run_requires 0)
>> - "requires")
>> - '())))
>> - (map specification->requirement-name requirements)))))
>> - (lambda ()
>> - (delete-file json-file)
>> - (rmdir dirname))))))
>> + (metadata (string-append dirname "/METADATA")))
>> + (call-with-temporary-directory
>> + (lambda (dir)
>> + (if (zero? (system* "unzip" "-q" wheel-archive "-d" dir metadata))
>> + (parse-wheel-metadata (string-append dir "/" metadata))
>> + (begin
>> + (warning
>> + (G_ "Failed to extract file: ~a from wheel.~%") metadata)
>> + #f))))))
>
> The old approach took care of removing the unpacked archive no matter
> what happened. The new code doesn’t do that.
The temporary directory where the archive is unpacked should be cleared
when leaving upon leaving its scope; the docstring of
"call-with-temporary-directory" says:
"Call PROC with a name of a temporary directory; close the directory and
delete it when leaving the dynamic extent of this call."
>> --- a/tests/pypi.scm
>> +++ b/tests/pypi.scm
>
> Thanks for the tests!
:-)
Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Tue, 11 Jun 2019 11:57:01 GMT) Full text and rfc822 format available.Message #109 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Tue, 11 Jun 2019 13:56:46 +0200
Hi Maxim, >>> + (call-with-input-file metadata >>> + (lambda (port) >>> + (let loop ((requirements '())) >>> + (let ((line (read-line port))) >>> + ;; Stop at the first 'Provides-Extra' section: the non-optional >>> + ;; requirements appear before the optional ones. >>> + (if (eof-object? line) >>> + (reverse (delete-duplicates requirements)) >>> + (cond >>> + ((and (requires-dist-header? line) (not (extra? line))) >>> + (loop (cons (specification->requirement-name >>> + (requires-dist-value line)) >>> + requirements))) >>> + (else >>> + (loop requirements))))))))) >>> + >> >> As before you can simplify the nested let and merge “if” and "cond“. > > Oh, I get it now, I think: > > --8<---------------cut here---------------start------------->8--- > > (call-with-input-file metadata > (lambda (port) > (let loop ((requirements '())) > - (let ((line (read-line port))) > - ;; Stop at the first 'Provides-Extra' section: the non-optional > - ;; requirements appear before the optional ones. > - (if (eof-object? line) > - (reverse (delete-duplicates requirements)) > - (cond > - ((and (requires-dist-header? line) (not (extra? line))) > - (loop (cons (specification->requirement-name > - (requires-dist-value line)) > - requirements))) > - (else > - (loop requirements))))))))) > + (match (read-line port) > + (line > + ;; Stop at the first 'Provides-Extra' section: the non-optional > + ;; requirements appear before the optional ones. > + (cond > + ((eof-object? line) > + (reverse (delete-duplicates requirements))) > + ((and (requires-dist-header? line) (not (extra? line))) > + (loop (cons (specification->requirement-name > + (requires-dist-value line)) > + requirements))) > + (else > + (loop requirements))))))))) > > (define (guess-requirements source-url wheel-url archive) > "Given SOURCE-URL, WHEEL-URL and a ARCHIVE of the package, return a list > --8<---------------cut here---------------end--------------->8--- Not quite. Your ‘match’ expression here doesn’t do anything that a ‘let’ wouldn’t have done. It really just binds the return value of (read-line port) to ‘line’; that’s the same as (let ((line (read-line port))) …). I gave a match example using predicate matchers in a previous reply. In any case, using ‘cond’ inside of a let would be just fine. If you wanted to go with ‘match’, though, you’d replace the ‘cond’. -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Wed, 12 Jun 2019 03:02:02 GMT) Full text and rfc822 format available.Message #112 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Wed, 12 Jun 2019 12:00:50 +0900
Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: > Next up: Seven of Nine, tertiary adjunct of unimatrix zero one: Ehe! I had to look up the reference; I'm not much of a Star Trek fan obviously :-P. >> From 37e499d5d5d5f690aa0a065c730e13f6a31dd30d Mon Sep 17 00:00:00 2001 >> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> >> Date: Thu, 28 Mar 2019 23:12:26 -0400 >> Subject: [PATCH 7/9] import: pypi: Include optional test inputs as >> native-inputs. >> >> * guix/import/pypi.scm (maybe-inputs): Add INPUT-TYPE argument, and use it. >> (test-section?): New predicate. >> (parse-requires.txt): Collect the optional test inputs, and return them as the >> second element of the returned list. > > AFAICT parse-requires.txt now returns a list of pairs, but used to > return a plain list before. Is this correct? Right, a list of two lists to be technically correct. >> (define (parse-requires.txt requires.txt) >> - "Given REQUIRES.TXT, a Setuptools requires.txt file, return a list of >> -requirement names." >> - ;; This is a very incomplete parser, which job is to select the non-optional >> - ;; dependencies and strip them out of any version information. >> + "Given REQUIRES.TXT, a Setuptools requires.txt file, return a pair of requirements. >> + >> +The first element of the pair contains the required dependencies while the >> +second the optional test dependencies. Note that currently, optional, >> +non-test dependencies are omitted since these can be difficult or expensive to >> +satisfy." >> + >> + ;; This is a very incomplete parser, which job is to read in the requirement >> + ;; specification lines, and strip them out of any version information. >> ;; Alternatively, we could implement a PEG parser with the (ice-9 peg) >> ;; library and the requirements grammar defined by PEP-0508 >> ;; (https://www.python.org/dev/peps/pep-0508/). > > Does it really return a pair? Or a list of pairs? Or is it a > two-element list of lists? The latter! I've fixed the docstring accordingly. >> (call-with-input-file requires.txt >> (lambda (port) >> - (let loop ((result '())) >> + (let loop ((required-deps '()) >> + (test-deps '()) >> + (inside-test-section? #f) >> + (optional? #f)) >> (let ((line (read-line port))) >> - ;; Stop when a section is encountered, as sections contains optional >> - ;; (extra) requirements. Non-optional requirements must appear >> - ;; before any section is defined. >> - (if (or (eof-object? line) (section-header? line)) >> + (if (eof-object? line) >> ;; Duplicates can occur, since the same requirement can be >> ;; listed multiple times with different conditional markers, e.g. >> ;; pytest >= 3 ; python_version >= "3.3" >> ;; pytest < 3 ; python_version < "3.3" >> - (reverse (delete-duplicates result)) >> + (map (compose reverse delete-duplicates) >> + (list required-deps test-deps)) > > Looks like a list of lists to me. “delete-duplicates” now won’t delete > a name that is in both “required-deps” as well as in “test-deps”. Is > this acceptable? It is acceptable, as this corner case cannot exist given the current code (a requirement can exist in either required-deps or test-deps, but never in both). It also doesn't make sense that a run time requirement would also be listed as a test requirement, so that corner case is not likely to exist in the future either. > Personally, I’m not a fan of using data structures for returning > multiple values, because we can simply return multiple values. I thought the Guile supported multiple values return value would be great here as well, but I've found that for this specific case here, a list of lists worked better, since the two lists contain requirements to be processed the same, which "map" can readily do (i.e. less ceremony is required). > Or we could have more than just strings. The meaning of these strings > is provided by the bin into which they are thrown — either > “required-deps” or “test-deps”. It could be an option to collect tagged > values instead and have the caller deal with filtering. Sounds neat, but I'd rather punt on this one for now. >> (define (parse-wheel-metadata metadata) >> - "Given METADATA, a Wheel metadata file, return a list of requirement names." >> + "Given METADATA, a Wheel metadata file, return a pair of requirements. >> + >> +The first element of the pair contains the required dependencies while the second the optional >> +test dependencies. Note that currently, optional, non-test dependencies are >> +omitted since these can be difficult or expensive to satisfy." >> ;; METADATA is a RFC-2822-like, header based file. > > This sounds like this is going to duplicate the previous procedures. Instead of duplicating the docstring, I'm now referring to that of PARSE-REQUIRES.TXT for PARSE-WHEEL-METADATA. The two procedures share the same interface, but implement a different parser, which consist of mostly a loop + conditional branches. IMHO, it's not worth, or even, desirable to try to merge those two parsers into one; I prefer to keep the logic of two distinct parsers separate. >> (define (requires-dist-header? line) >> ;; Return #t if the given LINE is a Requires-Dist header. >> - (regexp-match? (string-match "^Requires-Dist: " line))) >> + (string-match "^Requires-Dist: " line)) >> >> (define (requires-dist-value line) >> (string-drop line (string-length "Requires-Dist: "))) >> >> (define (extra? line) >> ;; Return #t if the given LINE is an "extra" requirement. >> - (regexp-match? (string-match "extra == " line))) >> + (string-match "extra == '(.*)'" line)) > > These hunks should be part of the previous patch where they were > introduced. (See my comments there about “regexp-match?”.) Done. >> + (define (test-requirement? line) >> + (let ((extra-label (match:substring (extra? line) 1))) >> + (and extra-label (test-section? extra-label)))) > > You can use “and=>” instead of binding a name: > > (and=> (match:substring (extra? line) 1) test-section?) Neat! I still don't have the reflex to use "and=>", thanks for reminding me about it! >> (call-with-input-file metadata >> (lambda (port) >> - (let loop ((requirements '())) >> + (let loop ((required-deps '()) >> + (test-deps '())) >> (let ((line (read-line port))) >> - ;; Stop at the first 'Provides-Extra' section: the non-optional >> - ;; requirements appear before the optional ones. >> (if (eof-object? line) >> - (reverse (delete-duplicates requirements)) >> + (map (compose reverse delete-duplicates) >> + (list required-deps test-deps)) >> (cond >> ((and (requires-dist-header? line) (not (extra? line))) >> (loop (cons (specification->requirement-name >> (requires-dist-value line)) >> - requirements))) >> + required-deps) >> + test-deps)) >> + ((and (requires-dist-header? line) (test-requirement? line)) >> + (loop required-deps >> + (cons (specification->requirement-name (requires-dist-value line)) >> + test-deps))) >> (else >> - (loop requirements))))))))) >> + (loop required-deps test-deps))))))))) ;skip line > > Could you refactor this so that the other parser can be reused? If not, > the same comments about “if” and “cond” and the use of pairs/lists > instead of “values” applies here. Done w.r.t. using only "cond" rather than "if" + "cond". As explained above, the docstring's body now refer to the documentation of PARSE-REQUIRES.TXT; otherwise I prefer to keep the parsers separate. >> (define (guess-requirements source-url wheel-url archive) >> - "Given SOURCE-URL, WHEEL-URL and a ARCHIVE of the package, return a list >> + "Given SOURCE-URL, WHEEL-URL and an ARCHIVE of the package, return a list >> of the required packages specified in the requirements.txt file. ARCHIVE will >> be extracted in a temporary directory." >> >> @@ -244,7 +289,10 @@ cannot determine package dependencies") (file-extension url)) >> (metadata (string-append dirname "/METADATA"))) >> (call-with-temporary-directory >> (lambda (dir) >> - (if (zero? (system* "unzip" "-q" wheel-archive "-d" dir metadata)) >> + (if (zero? >> + (parameterize ((current-error-port (%make-void-port "rw+")) >> + (current-output-port (%make-void-port "rw+"))) >> + (system* "unzip" wheel-archive "-d" dir metadata))) >> (parse-wheel-metadata (string-append dir "/" metadata)) >> (begin >> (warning >> @@ -283,32 +331,41 @@ cannot determine package dependencies") (file-extension url)) >> (warning >> (G_ "Failed to extract file: ~a from source.~%") >> requires.txt) >> - '()))))) >> - '()))) >> + (list '() '())))))) >> + (list '() '())))) > > I would like to see cosmetic changes like these three hunks in separate > commits. Done for the first two hunks; the last one isn't cosmetic; it changes the default return from an empty list to a list of two empty lists. >> (define (compute-inputs source-url wheel-url archive) >> - "Given the SOURCE-URL of an already downloaded ARCHIVE, return a list of >> -name/variable pairs describing the required inputs of this package. Also >> + "Given the SOURCE-URL and WHEEL-URL of an already downloaded ARCHIVE, return >> +a pair of lists, each consisting of a list of name/variable pairs, for the >> +propagated inputs and the native inputs, respectively. Also >> return the unaltered list of upstream dependency names." >> - (let ((dependencies >> - (remove (cut string=? "argparse" <>) >> - (guess-requirements source-url wheel-url archive)))) >> - (values (sort >> - (map (lambda (input) >> - (let ((guix-name (python->package-name input))) >> - (list guix-name (list 'unquote (string->symbol guix-name))))) >> - dependencies) >> - (lambda args >> - (match args >> - (((a _ ...) (b _ ...)) >> - (string-ci<? a b))))) >> - dependencies))) >> + >> + (define (strip-argparse deps) >> + (remove (cut string=? "argparse" <>) deps)) >> + >> + (define (requirement->package-name/sort deps) >> + (sort >> + (map (lambda (input) >> + (let ((guix-name (python->package-name input))) >> + (list guix-name (list 'unquote (string->symbol guix-name))))) >> + deps) >> + (lambda args >> + (match args >> + (((a _ ...) (b _ ...)) >> + (string-ci<? a b)))))) >> + >> + (define process-requirements >> + (compose requirement->package-name/sort strip-argparse)) >> + >> + (let ((dependencies (guess-requirements source-url wheel-url archive))) >> + (values (map process-requirements dependencies) >> + (concatenate dependencies)))) > > Giving names to these processing steps is fine and improves clarity, but > I’m not so comfortable about returning ad-hoc pairs *and* multiple > values (like above). Using "values" is required by the API of the recursive importer. >> + (match guix-dependencies >> + ((required-inputs test-inputs) >> + (values >> + `(package >> + (name ,(python->package-name name)) >> + (version ,version) >> + (source (origin >> + (method url-fetch) >> + ;; Sometimes 'pypi-uri' doesn't quite work due to mixed >> + ;; cases in NAME, for instance, as is the case with >> + ;; "uwsgi". In that case, fall back to a full URL. >> + (uri (pypi-uri ,(string-downcase name) version)) >> + (sha256 >> + (base32 >> + ,(guix-hash-url temp))))) >> + (build-system python-build-system) >> + ,@(maybe-inputs required-inputs 'propagated-inputs) >> + ,@(maybe-inputs test-inputs 'native-inputs) >> + (home-page ,home-page) >> + (synopsis ,synopsis) >> + (description ,description) >> + (license ,(license->symbol license))) >> + ;; Flatten the nested lists and return the upstream >> + ;; dependencies. >> + upstream-dependencies)))))))) > > I don’t see anything being flattened here? Good catch! Seems a remnant of something that is now gone :-). Thanks for the review. Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Wed, 12 Jun 2019 06:40:02 GMT) Full text and rfc822 format available.Message #115 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Wed, 12 Jun 2019 08:39:12 +0200
Hi Maxim,
>>> (call-with-input-file requires.txt
>>> (lambda (port)
>>> - (let loop ((result '()))
>>> + (let loop ((required-deps '())
>>> + (test-deps '())
>>> + (inside-test-section? #f)
>>> + (optional? #f))
>>> (let ((line (read-line port)))
>>> - ;; Stop when a section is encountered, as sections contains optional
>>> - ;; (extra) requirements. Non-optional requirements must appear
>>> - ;; before any section is defined.
>>> - (if (or (eof-object? line) (section-header? line))
>>> + (if (eof-object? line)
>>> ;; Duplicates can occur, since the same requirement can be
>>> ;; listed multiple times with different conditional markers, e.g.
>>> ;; pytest >= 3 ; python_version >= "3.3"
>>> ;; pytest < 3 ; python_version < "3.3"
>>> - (reverse (delete-duplicates result))
>>> + (map (compose reverse delete-duplicates)
>>> + (list required-deps test-deps))
>>
>> Looks like a list of lists to me. “delete-duplicates” now won’t delete
>> a name that is in both “required-deps” as well as in “test-deps”. Is
>> this acceptable?
>
> It is acceptable, as this corner case cannot exist given the current
> code (a requirement can exist in either required-deps or test-deps, but
> never in both). It also doesn't make sense that a run time requirement
> would also be listed as a test requirement, so that corner case is not
> likely to exist in the future either.
I mentioned it because I believe I’ve seen this in the past where the
importer would return some of the same inputs as both regular inputs and
test dependencies.
>> Personally, I’m not a fan of using data structures for returning
>> multiple values, because we can simply return multiple values.
>
> I thought the Guile supported multiple values return value would be
> great here as well, but I've found that for this specific case here, a
> list of lists worked better, since the two lists contain requirements to
> be processed the same, which "map" can readily do (i.e. less ceremony is
> required).
“map” can also operate on more than one list at a time:
(call-with-values
(lambda ()
(values (list 1 2 3)
(list 9 8 7)))
(lambda (a b) (map + a b)))
=> (10 10 10)
Of course, it would be simpler to just use a single list of tagged
items.
--
Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Sun, 16 Jun 2019 05:12:02 GMT) Full text and rfc822 format available.Message #118 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Sun, 16 Jun 2019 14:10:52 +0900
Hello Ricardo! Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: >> From cfde6e09f8f8c692fe252d76ed27e8c50a9e5377 Mon Sep 17 00:00:00 2001 >> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> >> Date: Sat, 30 Mar 2019 23:13:26 -0400 >> Subject: [PATCH 8/9] import: pypi: Scan source archive to find requires.txt >> file. > >> * guix/import/pypi.scm (use-modules): Use invoke from (guix build utils). >> (guess-requirements)[archive-root-directory]: Remove procedure. > > Oh, I guess I reviewed this procedure in vain :( > > Please modify the commits so that added procedures are not removed in > later commits. This is easier on the reviewer and makes for a clearer > commit history. Indeed; I'll be more careful about this is the future; sorry! I've squashed this commit along with the one enabling more archive types support, as this is where the modified (and later removed) procedure originated. >> (define (guess-requirements-from-source) >> ;; Return the package's requirements by guessing them from the source. >> - (let ((dirname (archive-root-directory source-url)) >> - (extension (file-extension source-url))) >> - (if (string? dirname) >> - (call-with-temporary-directory >> - (lambda (dir) >> - (let* ((pypi-name (string-take dirname (string-rindex dirname #\-))) >> - (requires.txt (string-append dirname "/" pypi-name >> - ".egg-info" "/requires.txt")) >> - (exit-code >> - (parameterize ((current-error-port (%make-void-port "rw+")) >> - (current-output-port (%make-void-port "rw+"))) >> - (if (string=? "zip" extension) >> - (system* "unzip" archive "-d" dir requires.txt) >> - (system* "tar" "xf" archive "-C" dir requires.txt))))) >> - (if (zero? exit-code) >> - (parse-requires.txt (string-append dir "/" requires.txt)) >> - (begin >> - (warning >> - (G_ "Failed to extract file: ~a from source.~%") >> - requires.txt) >> - (list '() '())))))) >> + (if (compressed-file? source-url) >> + (call-with-temporary-directory >> + (lambda (dir) >> + (parameterize ((current-error-port (%make-void-port "rw+")) >> + (current-output-port (%make-void-port "rw+"))) >> + (if (string=? "zip" (file-extension source-url)) >> + (invoke "unzip" archive "-d" dir) >> + (invoke "tar" "xf" archive "-C" dir))) >> + (let ((requires.txt-files >> + (find-files dir (lambda (abs-file-name _) >> + (string-match "\\.egg-info/requires.txt$" >> + abs-file-name))))) >> + (if (> (length requires.txt-files) 0) > > Let’s work on the empty list directly. Here “match” would be better. Done, like this: --8<---------------cut here---------------start------------->8--- - (if (> (length requires.txt-files) 0) - (parse-requires.txt (first requires.txt-files)) - (begin (warning (G_ "Cannot guess requirements from source archive:\ + (match requires.txt-files + (() + (warning (G_ "Cannot guess requirements from source archive:\ no requires.txt file found.~%")) - '()))))) + '()) + (else (parse-requires.txt (first requires.txt-files))))))) --8<---------------cut here---------------end--------------->8--- >> + (begin >> + (parse-requires.txt (first requires.txt-files))) > > No need for “begin” here. Fixed. >> + (begin (warning (G_ "Cannot guess requirements from source archive:\ >> + no requires.txt file found.~%")) >> + (list '() '())))))) > > I know that this is from an earlier commit, but I don’t like the look of > “(list '() '())” at all :) > >> + (begin >> + (warning (G_ "Unsupported archive format; \ >> +cannot determine package dependencies from source archive: ~a~%") >> + (basename source-url)) >> (list '() '())))) > > Same here. Certainly there’s a better return value. This might look ugly, but I can't think of a better return value, since using anything else would mean having to introduce extra logic in the callers, while it is now a correct value that needs no special case. Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Sun, 16 Jun 2019 05:54:01 GMT) Full text and rfc822 format available.Message #121 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Sun, 16 Jun 2019 14:53:33 +0900
Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: > And finally: Number 9! Yay! >> From 1290f9d1f0d594fdd4723d76b94116be25da9dd5 Mon Sep 17 00:00:00 2001 >> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> >> Date: Sat, 30 Mar 2019 20:27:35 -0400 >> Subject: [PATCH 9/9] import: pypi: Preserve package name case when forming >> pypi-uri. >> >> Fixes issue: #33046. > > Please change this to: > > Fixes <https://bugs.gnu.org/33046>. Done! >> * guix/build-system/python.scm (pypi-uri): Update the host URI to >> "files.pythonhosted.org". >> * guix/import/pypi.scm (make-pypi-sexp): Preserve the package name case when >> the source URL calls for it. > > Is the first change to use files.pythonhosted.org required to fix this? > Or is this unrelated? > > If it is required this looks fine to me. > > Thank you! The permanent redirection was found while fixing the issue; but it's better to have the fix separate. I've separated it into its own commit. Thank you!
bug-guix <at> gnu.org:bug#24450; Package guix.
(Sun, 16 Jun 2019 06:06:02 GMT) Full text and rfc822 format available.Message #124 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Sun, 16 Jun 2019 15:05:27 +0900
Hi again, Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: > Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes: > >> While I agree that a regexp is a bigger hammer than basic string >> manipulation, I see some merit to it here: >> >> 1) We can be assured of conformance with upstream, again, per PEP-0508. >> 2) It is easier to extend; we might want to add parsing for the version >> spec in order to disregard dependencies specified for Python < 3, for >> example. >> >> The use of the PEP-0508 grammar to define the regexp is useful to detail >> in a more human-friendly language the components of the regexp. We >> could have otherwise used the more cryptic regexp for Python >> distribution names: >> >> --8<---------------cut here---------------start------------->8--- >> ^([A-Z0-9]|[A-Z0-9][A-Z0-9._-]*[A-Z0-9])$ >> --8<---------------cut here---------------end--------------->8--- >> >> So I guess that what I'm saying is that I prefer this approach to using >> string-index with invalid characters, for the reasons above. >> >> [0] https://www.python.org/dev/peps/pep-0508/ > > Okay, sounds good. Please make sure to note this in a comment, so that > I won’t be asking myself this same question in a year :) Done! Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Sun, 16 Jun 2019 14:12:02 GMT) Full text and rfc822 format available.Message #127 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Sun, 16 Jun 2019 23:11:01 +0900
Hello! Continued feedback about your much appreciated comments! :-) Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: > Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes: > >>>> + ;; (extra) requirements. Non-optional requirements must appear >>>> + ;; before any section is defined. >>>> + (if (or (eof-object? line) (section-header? line)) >>>> + (reverse result) >>>> + (cond >>>> + ((or (string-null? line) (comment? line)) >>>> + (loop result)) >>>> + (else >>>> + (loop (cons (clean-requirement line) >>>> + result)))))))))) >>>> + >>> >>> I think it would be better to use “match” here instead of nested “let”, >>> “if” and “cond”. At least you can drop the “if” and just use cond. >>> >>> The loop let and the inner let can be merged. >> >> I'm not sure I understand; wouldn't merging the named let with the plain >> let mean adding an extra LINE argument to my LOOP procedure? I don't >> want that. > > Let’s forget about merging the nested “let”, because you would indeed > need to change a few more things. It’s fine to keep that as it is. But > (if … (cond …)) is not pretty. At least it could be done in one “cond”: > > (cond > ((or (eof-object? line) (section-header? line)) > (reverse result)) > ((or (string-null? line) (comment? line)) > (loop result)) > (else > (loop (cons (clean-requirement line) > result)))) Agreed and fixed, thanks. >> Also, how could the above code be expressed using "match"? I'm using >> predicates which tests for (special) characters in a string; I don't see >> how the more primitive pattern language of "match" will enable me to do >> the same. > > “match” has support for predicates, so you could do something like this: > > (match line > ((or (eof-object) (? section-header?)) > (reverse result)) > ((or '() (? comment?)) > (loop result)) > (_ (loop (cons (clean-requirement line) result)))) Oh, that's neat! I had no idea that predicates could be used with "match". '() would need to be replaced by "" to match the empty string. Another gotcha with "match", is that the "or" seems to evaluate every component, no matter if a early true condition was found; this resulted in the following error: --8<---------------cut here---------------start------------->8--- + (wrong-type-arg + "string-trim" + "Wrong type argument in position ~A (expecting ~A): ~S" + (1 "string" #<eof>) + (#<eof>)) result: FAIL --8<---------------cut here---------------end--------------->8--- Due to the "(or (eof-object) (? section-header?)" match clause evaluating the section-header? predicate despite the line being an EOF character. > This allows you to match “eof-object” and '() directly. Whenever I see > “string-null?” I think it might be better to “match” on the empty list > directly. string-null? and an empty list are not the same, unless I'm missing something. > But really, that’s up to you. I only feel strongly about avoiding “(if > … (cond …))”. Due to the problem mentioned above, I stayed with "cond". Thanks! Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Sun, 16 Jun 2019 14:31:01 GMT) Full text and rfc822 format available.Message #130 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Sun, 16 Jun 2019 23:29:58 +0900
Hey Ricardo :-) Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> writes: > Hi Maxim, > >>>> (call-with-input-file requires.txt >>>> (lambda (port) >>>> - (let loop ((result '())) >>>> + (let loop ((required-deps '()) >>>> + (test-deps '()) >>>> + (inside-test-section? #f) >>>> + (optional? #f)) >>>> (let ((line (read-line port))) >>>> - ;; Stop when a section is encountered, as sections contains optional >>>> - ;; (extra) requirements. Non-optional requirements must appear >>>> - ;; before any section is defined. >>>> - (if (or (eof-object? line) (section-header? line)) >>>> + (if (eof-object? line) >>>> ;; Duplicates can occur, since the same requirement can be >>>> ;; listed multiple times with different conditional markers, e.g. >>>> ;; pytest >= 3 ; python_version >= "3.3" >>>> ;; pytest < 3 ; python_version < "3.3" >>>> - (reverse (delete-duplicates result)) >>>> + (map (compose reverse delete-duplicates) >>>> + (list required-deps test-deps)) >>> >>> Looks like a list of lists to me. “delete-duplicates” now won’t delete >>> a name that is in both “required-deps” as well as in “test-deps”. Is >>> this acceptable? >> >> It is acceptable, as this corner case cannot exist given the current >> code (a requirement can exist in either required-deps or test-deps, but >> never in both). It also doesn't make sense that a run time requirement >> would also be listed as a test requirement, so that corner case is not >> likely to exist in the future either. > > I mentioned it because I believe I’ve seen this in the past where the > importer would return some of the same inputs as both regular inputs and > test dependencies. OK! >>> Personally, I’m not a fan of using data structures for returning >>> multiple values, because we can simply return multiple values. >> >> I thought the Guile supported multiple values return value would be >> great here as well, but I've found that for this specific case here, a >> list of lists worked better, since the two lists contain requirements to >> be processed the same, which "map" can readily do (i.e. less ceremony is >> required). > > “map” can also operate on more than one list at a time: > > (call-with-values > (lambda () > (values (list 1 2 3) > (list 9 8 7))) > (lambda (a b) (map + a b))) > > => (10 10 10) That's what I meant by "requires more ceremony". I can simply apply "map" to the return value of the function and get what I need, rather than having to use "values" in the callee, then "call-with-values" in the caller and establish a binding for each list. > Of course, it would be simpler to just use a single list of tagged > items. Do you feel strongly about it? I don't; I'm open to try to use a tagged list if you feel this is worth it. Maxim
bug-guix <at> gnu.org:bug#24450; Package guix.
(Sun, 16 Jun 2019 14:38:01 GMT) Full text and rfc822 format available.Message #133 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450 <at> debbugs.gnu.org Subject: Re: bug#24450: [PATCHv3] Re: pypi importer outputs strange character series in optional dependency case. Date: Sun, 16 Jun 2019 23:36:49 +0900
[Message part 1 (text/plain, inline)]
Here's the current patch set, version 3, with the modifications as discussed in the previous exchanges:
[0001-import-pypi-Do-not-consider-requirements.txt-files.patch (text/x-patch, attachment)]
[0001-import-pypi-Do-not-parse-optional-requirements-from-.patch (text/x-patch, attachment)]
[0002-tests-pypi-Mute-the-output-of-tar.patch (text/x-patch, attachment)]
[0003-import-pypi-Do-not-parse-optional-requirements-from-.patch (text/x-patch, attachment)]
[0004-import-pypi-Improve-parsing-of-requirement-specifica.patch (text/x-patch, attachment)]
[0005-import-pypi-Deduplicate-requirements.patch (text/x-patch, attachment)]
[0006-import-pypi-Support-more-types-of-archives.patch (text/x-patch, attachment)]
[0007-import-pypi-Parse-wheel-METADATA-instead-of-metadata.patch (text/x-patch, attachment)]
[0008-import-pypi-Fix-typo-in-docstring.patch (text/x-patch, attachment)]
[0009-import-pypi-Completely-mute-the-output-of-the-unzip-.patch (text/x-patch, attachment)]
[0010-import-pypi-Include-optional-test-inputs-as-native-i.patch (text/x-patch, attachment)]
[0011-import-pypi-Update-the-host-URI.patch (text/x-patch, attachment)]
[0012-import-pypi-Preserve-package-name-case-when-forming-.patch (text/x-patch, attachment)]
[signature.asc (application/pgp-signature, inline)]
bug-guix <at> gnu.org:bug#24450; Package guix.
(Sun, 16 Jun 2019 17:04:02 GMT) Full text and rfc822 format available.Message #136 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: ng0 <ng0 <at> we.make.ritual.n0.is> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: ng0 <ng0 <at> we.make.ritual.n0.is>, 24450 <at> debbugs.gnu.org Subject: Re: bug#24450: pypi importer outputs strange character series in optional dependency case. Date: Sun, 16 Jun 2019 17:02:35 +0000
Hi Maxim,
great to see this fixed. Weird it bumped into my view after
almost 3 years. I trust that other people will have tested
it, I'm all over the place and mostly doing NetBSD and GNUnet
now.
Sorry if I have written a reply before, the thread was
marked as unread here.
Maxim Cournoyer transcribed 3.5K bytes:
> ng0 <ng0 <at> we.make.ritual.n0.is> writes:
>
> > I think this should not happen with pypi import:
> >
> > (inputs
> > `(("python-certifi==2016.2.28"
> > ,python-certifi==2016.2.28)
> > ("python-dateutil==2.5.3"
> > ,python-dateutil==2.5.3)
> > ("python-flask-babel==0.11.1"
> > ,python-flask-babel==0.11.1)
> > ("python-flask==0.11.1" ,python-flask==0.11.1)
> > ("python-lxml==3.6.0" ,python-lxml==3.6.0)
> > ("python-ndg-httpsclient==0.4.1"
> > ,python-ndg-httpsclient==0.4.1)
> > ("python-pyasn1-modules==0.0.8"
> > ,python-pyasn1-modules==0.0.8)
> > ("python-pyasn1==0.1.9" ,python-pyasn1==0.1.9)
> > ("python-pygments==2.1.3"
> > ,python-pygments==2.1.3)
> > ("python-pyopenssl==0.15.1"
> > ,python-pyopenssl==0.15.1)
> > ("python-pyyaml==3.11" ,python-pyyaml==3.11)
> > ("python-requests[socks]==2.10.0"
> > ,#{python-requests\x5b;socks\x5d;==2.10.0}#)
> > ("python-setuptools" ,python-setuptools)))
> >
> >
> > I can understand the version numbers, I can also understand the optional
> > socks building/module of the python-requests, but why does it read like
> > Gobbledygook? Can't we improve the output here?
> >
> > For version numbers, this is not a format which happened recently which
> > is exclusive for python build system right? This is just bad formated
> > because of the pypi query.
> > I will first try and not pin the application to these version numbers,
> > maybe itjustworks™.
> >
> >
> > To reproduce: "guix import pypi searx"
>
> This would now give (change to be sent for review soon):
>
> --8<---------------cut here---------------start------------->8---
> ./pre-inst-env guix import pypi searx
>
> Starting download of /tmp/guix-file.1wD8K4
> From https://files.pythonhosted.org/packages/75/3f/5941ad2d500ff7cf6f8da1022c78013dcd2207941d533586a8e7bfe699d3/searx-0.15.0.tar.gz...
> …5.0.tar.gz 1.6MiB 729KiB/s 00:02 [##################] 100.0%
> (package
> (name "python-searx")
> (version "0.15.0")
> (source
> (origin
> (method url-fetch)
> (uri (pypi-uri "searx" version))
> (sha256
> (base32
> "1gmww73q7wydkvlyz73wnr3sybpjn40wha7avnz9ak9m365zcjxf"))))
> (build-system python-build-system)
> (propagated-inputs
> `(("python-certifi" ,python-certifi)
> ("python-dateutil" ,python-dateutil)
> ("python-flask" ,python-flask)
> ("python-flask-babel" ,python-flask-babel)
> ("python-idna" ,python-idna)
> ("python-lxml" ,python-lxml)
> ("python-pygments" ,python-pygments)
> ("python-pyopenssl" ,python-pyopenssl)
> ("python-pyyaml" ,python-pyyaml)
> ("python-requests" ,python-requests)))
> (native-inputs
> `(("python-babel" ,python-babel)
> ("python-cov-core" ,python-cov-core)
> ("python-mock" ,python-mock)
> ("python-nose2" ,python-nose2)
> ("python-pep8" ,python-pep8)
> ("python-plone.testing" ,python-plone.testing)
> ("python-selenium" ,python-selenium)
> ("python-splinter" ,python-splinter)
> ("python-transifex-client"
> ,python-transifex-client)
> ("python-unittest2" ,python-unittest2)
> ("python-zope.testrunner"
> ,python-zope.testrunner)))
> (home-page "https://github.com/asciimoo/searx")
> (synopsis
> "A privacy-respecting, hackable metasearch engine")
> (description
> "A privacy-respecting, hackable metasearch engine")
> (license #f))
> --8<---------------cut here---------------end--------------->8---
>
>
>
>
bug-guix <at> gnu.org:bug#24450; Package guix.
(Mon, 17 Jun 2019 01:42:02 GMT) Full text and rfc822 format available.Message #139 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 24450 <at> debbugs.gnu.org Subject: Re: [PATCHv2] Re: pypi importer outputs strange character series in optional dependency case. Date: Mon, 17 Jun 2019 03:41:16 +0200
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes: >> This allows you to match “eof-object” and '() directly. Whenever I see >> “string-null?” I think it might be better to “match” on the empty list >> directly. > > string-null? and an empty list are not the same, unless I'm missing something. Yes, sorry, I meant “null?”. Using “string-null?” is equivalent to matching the empty string, of course. >> But really, that’s up to you. I only feel strongly about avoiding “(if >> … (cond …))”. > > Due to the problem mentioned above, I stayed with "cond". Okay! Thanks. -- Ricardo
bug-guix <at> gnu.org:bug#24450; Package guix.
(Wed, 26 Jun 2019 04:13:01 GMT) Full text and rfc822 format available.Message #142 received at 24450 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: ng0 <ng0 <at> we.make.ritual.n0.is> Cc: 24450 <at> debbugs.gnu.org Subject: Re: bug#24450: pypi importer outputs strange character series in optional dependency case. Date: Wed, 26 Jun 2019 13:12:11 +0900
Hey ng0! ng0 <ng0 <at> we.make.ritual.n0.is> writes: > Hi Maxim, > > great to see this fixed. Weird it bumped into my view after > almost 3 years. Yes! Bugs don't really expire, do they? :-) > I trust that other people will have tested > it, I'm all over the place and mostly doing NetBSD and GNUnet > now. I'm glad to know you're still involved with GNUnet :-) > Sorry if I have written a reply before, the thread was > marked as unread here. No problem! I'll wait a few more days to allow for commenting some more, then push. Cheers! Maxim
Maxim Cournoyer <maxim.cournoyer <at> gmail.com>:ng0 <ng0 <at> we.make.ritual.n0.is>:Message #147 received at 24450-done <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ricardo Wurmus <ricardo.wurmus <at> mdc-berlin.de> Cc: 24450-done <at> debbugs.gnu.org Subject: Re: bug#24450: [PATCHv3] Re: pypi importer outputs strange character series in optional dependency case. Date: Tue, 02 Jul 2019 10:54:30 +0900
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes: > Here's the current patch set, version 3, with the modifications > as discussed in the previous exchanges [...] I've now merged this patch series (version 3) into master, with top commit 4b60ab8c006964d026dee8cf5f1260eba0b2bb81. Closing. Thank you! Maxim
Maxim Cournoyer <maxim.cournoyer <at> gmail.com>:ng0 <ngillmann <at> runbox.com>:Maxim Cournoyer <maxim.cournoyer <at> gmail.com>:Julien Lepiller <julien <at> lepiller.eu>:Maxim Cournoyer <maxim.cournoyer <at> gmail.com>:swedebugia <swedebugia <at> riseup.net>:Maxim Cournoyer <maxim.cournoyer <at> gmail.com>:swedebugia <swedebugia <at> riseup.net>:Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Tue, 30 Jul 2019 11:24:05 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.