videlec commented 4 years ago

Main changes:

https://pypi.org/project/rpy2/, supports python >= 3.6
cygwin.patch does not apply anymore
cffi>=1.13.1 is a new dependency, we install latest 1.14.0 (supports python >= 3.2)
pycparser is a new dependency (required by cffi, supports python >= 3.4)
https://pypi.org/project/tzlocal/ is a new dependency, we install latest 2.1 (supports python >= 2.7)
https://pypi.org/project/pytz/ is a new dependency, we update to latest 2020.1 (supports python >= 2.4)
add small patches to setup.py that
- disables printing on stdout (that perturbs pip) - accepted upstream
- removes the build dependency on pytest
some more dependencies (pytest, numpy, tzlocal) conditional on SAGE_CHECK!=no

tarballs: see checksums.ini (to download automatically, use ./configure --enable-download-from-upstream-url)

Upstream issues and PR

Depends on #29851 Depends on #30064 Depends on #30118 Depends on #30067 Depends on #30147 Depends on #29929 Depends on #30149

CC: @mkoeppe @EmmanuelCharpentier @timokau @isuruf @slel @antonio-rojas @embray @dimpase

Component: packages: standard

Author: Vincent Delecroix, Matthias Koeppe

Branch: 54d0f99

Reviewer: Emmanuel Charpentier, Dima Pasechnik

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

videlec commented 4 years ago

comment:1

The cygwin.patch does not longer apply.

videlec commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
-The release 3.2.7 is only Python 3 compatible
+The release 3.2.7 is only Python 3 compatible.

 tarball: https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz

videlec commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,11 @@
 The release 3.2.7 is only Python 3 compatible.

-tarball: https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
+Main changes:
+- `cygwin.patch` does not apply anymore
+- [https://pypi.org/project/pytest/](https://pypi.org/project/pytest/) is a new dependency (!?)
+- [https://pypi.org/project/cffi/ cffi>=1.13.1](https://pypi.org/project/cffi/ cffi>=1.13.1) is a new dependency
+
+tarballs: 
+- https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
+- https://files.pythonhosted.org/packages/6d/4e/572aed20422dee7fa2bd27995b2a53a32de90c1826e5531c9df6d3ea77ed/pytest-5.4.1.tar.gz
+- https://files.pythonhosted.org/packages/05/54/3324b0c46340c31b909fcec598696aaec7ddc8c18a63f2db352562d3354c/cffi-1.14.0.tar.gz

videlec commented 4 years ago

Description changed:

--- 
+++ 
@@ -4,8 +4,10 @@
 - `cygwin.patch` does not apply anymore
 - [https://pypi.org/project/pytest/](https://pypi.org/project/pytest/) is a new dependency (!?)
 - [https://pypi.org/project/cffi/ cffi>=1.13.1](https://pypi.org/project/cffi/ cffi>=1.13.1) is a new dependency
+- [https://pypi.org/project/pycparser/ pycparser](https://pypi.org/project/pycparser/ pycparser) is a new dependency (required by cffi)

 tarballs: 
 - https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
 - https://files.pythonhosted.org/packages/6d/4e/572aed20422dee7fa2bd27995b2a53a32de90c1826e5531c9df6d3ea77ed/pytest-5.4.1.tar.gz
 - https://files.pythonhosted.org/packages/05/54/3324b0c46340c31b909fcec598696aaec7ddc8c18a63f2db352562d3354c/cffi-1.14.0.tar.gz
+- https://files.pythonhosted.org/packages/0f/86/e19659527668d70be91d0369aeaa055b4eb396b0f387a4f92293a20035bd/pycparser-2.20.tar.gz

videlec commented 4 years ago

comment:4

pytest has a lot of dependencies

attrs
importlib-metadata
more-itertools
pluggy
py
zipp The easiest might be to patch rpy2 to not depend on pytest. I suspect that only the test suite of rpy2 depends on pytest.

videlec commented 4 years ago

Description changed:

--- 
+++ 
@@ -2,12 +2,15 @@

 Main changes:
 - `cygwin.patch` does not apply anymore
+- the `setup.py` script is broken in several ways (!!)
 - [https://pypi.org/project/pytest/](https://pypi.org/project/pytest/) is a new dependency (!?)
 - [https://pypi.org/project/cffi/ cffi>=1.13.1](https://pypi.org/project/cffi/ cffi>=1.13.1) is a new dependency
 - [https://pypi.org/project/pycparser/ pycparser](https://pypi.org/project/pycparser/ pycparser) is a new dependency (required by cffi)
+- [https://pypi.org/project/tzlocal/ tzlocal](https://pypi.org/project/tzlocal/ tzlocal)

 tarballs: 
 - https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
 - https://files.pythonhosted.org/packages/6d/4e/572aed20422dee7fa2bd27995b2a53a32de90c1826e5531c9df6d3ea77ed/pytest-5.4.1.tar.gz
 - https://files.pythonhosted.org/packages/05/54/3324b0c46340c31b909fcec598696aaec7ddc8c18a63f2db352562d3354c/cffi-1.14.0.tar.gz
 - https://files.pythonhosted.org/packages/0f/86/e19659527668d70be91d0369aeaa055b4eb396b0f387a4f92293a20035bd/pycparser-2.20.tar.gz
+- https://files.pythonhosted.org/packages/c6/52/5ec375d4efcbe4e31805f3c4b301bdfcff9dcbdb3605d4b79e117a16b38d/tzlocal-2.0.0.tar.gz

videlec commented 4 years ago

comment:6

Good news: tests pass on my machine!

mkoeppe commented 4 years ago

comment:8

See #28998 for pytest

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 4 years ago

comment:9

Replying to @videlec:

pytest has a lot of dependencies

attrs

importlib-metadata

more-itertools

pluggy

py

zipp

Tall order, indeed...

The easiest might be to patch rpy2 to not depend on pytest. I suspect that only the test suite of rpy2 depends on pytest.

My first reaction was: No, no, no, no, no. And no.". A patched version has a very heavy price in terms of maintenance each and every update entails patch checking/upgrading, up to the point the whole mess becomes unmaintainable.

The maintenance price of the added dependencies should be considered, of course, but I suspect that it may be lower than the price of a patch to rpy2.

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 4 years ago

Description changed:

--- 
+++ 
@@ -2,15 +2,12 @@

 Main changes:
 - `cygwin.patch` does not apply anymore
-- the `setup.py` script is broken in several ways (!!)
 - [https://pypi.org/project/pytest/](https://pypi.org/project/pytest/) is a new dependency (!?)
 - [https://pypi.org/project/cffi/ cffi>=1.13.1](https://pypi.org/project/cffi/ cffi>=1.13.1) is a new dependency
 - [https://pypi.org/project/pycparser/ pycparser](https://pypi.org/project/pycparser/ pycparser) is a new dependency (required by cffi)
-- [https://pypi.org/project/tzlocal/ tzlocal](https://pypi.org/project/tzlocal/ tzlocal)

 tarballs: 
 - https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
 - https://files.pythonhosted.org/packages/6d/4e/572aed20422dee7fa2bd27995b2a53a32de90c1826e5531c9df6d3ea77ed/pytest-5.4.1.tar.gz
 - https://files.pythonhosted.org/packages/05/54/3324b0c46340c31b909fcec598696aaec7ddc8c18a63f2db352562d3354c/cffi-1.14.0.tar.gz
 - https://files.pythonhosted.org/packages/0f/86/e19659527668d70be91d0369aeaa055b4eb396b0f387a4f92293a20035bd/pycparser-2.20.tar.gz
-- https://files.pythonhosted.org/packages/c6/52/5ec375d4efcbe4e31805f3c4b301bdfcff9dcbdb3605d4b79e117a16b38d/tzlocal-2.0.0.tar.gz

videlec commented 4 years ago

comment:10

Replying to @mkoeppe:

See #28998 for pytest

Thanks for the pointer. Would pytest be installed as a "standard" package?

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 4 years ago

comment:11

Replying to @mkoeppe:

See #28998 for pytest

Dors it need much work ?

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 4 years ago

comment:12

Replying to @videlec:

Replying to @mkoeppe:

See #28998 for pytest

Thanks for the pointer. Would pytest be installed as a "standard" package?

It would have to : r depends on rpy2, which would depend on pytest. ISTR that you yourself stated that having a standard package depend on an optional package would be madness (and I agree...). Therefore...

videlec commented 4 years ago

New commits:

`de753f8`	`upgrade rpy2 version`
`7ca1336`	`remove cygwin.patch (rpy2)`
`c525fc2`	`add setup.patch (rpy2)`

videlec commented 4 years ago

Branch: public/29441

videlec commented 4 years ago

Commit: c525fc2

videlec commented 4 years ago

comment:14

As far as tests are concerned, the design of rpy2 is weird. Tests should be separate from the rpy2 module (and hence the dependency of pytest superfluous).

mkoeppe commented 4 years ago

comment:15

Replying to @EmmanuelCharpentier:

Milestone changed from sage-9.2 to sage-9.1

Was this intentional? We definitely cannot drop py2 support for 9.1

videlec commented 4 years ago

comment:16

Replying to @mkoeppe:

Replying to @EmmanuelCharpentier:

Milestone changed from sage-9.2 to sage-9.1

Was this intentional? We definitely cannot drop py2 support for 9.1

Agreed. This was a mistake.

videlec commented 4 years ago

Description changed:

--- 
+++ 
@@ -2,12 +2,15 @@

 Main changes:
 - `cygwin.patch` does not apply anymore
-- [https://pypi.org/project/pytest/](https://pypi.org/project/pytest/) is a new dependency (!?)
-- [https://pypi.org/project/cffi/ cffi>=1.13.1](https://pypi.org/project/cffi/ cffi>=1.13.1) is a new dependency
-- [https://pypi.org/project/pycparser/ pycparser](https://pypi.org/project/pycparser/ pycparser) is a new dependency (required by cffi)
+- [pytest](https://pypi.org/project/pytest/) is a new dependency (!?)
+- [cffi>=1.13.1](https://pypi.org/project/cffi/) is a new dependency
+- [pycparser](https://pypi.org/project/pycparser/) is a new dependency (required by cffi)

 tarballs: 
 - https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
 - https://files.pythonhosted.org/packages/6d/4e/572aed20422dee7fa2bd27995b2a53a32de90c1826e5531c9df6d3ea77ed/pytest-5.4.1.tar.gz
 - https://files.pythonhosted.org/packages/05/54/3324b0c46340c31b909fcec598696aaec7ddc8c18a63f2db352562d3354c/cffi-1.14.0.tar.gz
 - https://files.pythonhosted.org/packages/0f/86/e19659527668d70be91d0369aeaa055b4eb396b0f387a4f92293a20035bd/pycparser-2.20.tar.gz
+
+upstream issues and PR
+- [issues #670 (about tests in rpy2 and pytest dependency)](https://github.com/rpy2/rpy2/issues/670)

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 4 years ago

comment:18

Replying to @videlec:

As far as tests are concerned, the design of rpy2 is weird. Tests should be separate from the rpy2 module (and hence the dependency of pytest superfluous).

Replying to @videlec:

As far as tests are concerned, the design of rpy2 is weird. Tests should be separate from the rpy2 module (and hence the dependency of pytest superfluous).

1) Can rpy2 use packages hypothetically installed by an hypothetical package implementing #28998 ?

2) should we instead wait for an upstream-fixed rpy2 (supposing that upstream agrees with you...) ?

videlec commented 4 years ago

comment:19

Replying to @EmmanuelCharpentier:

Replying to @videlec:

As far as tests are concerned, the design of rpy2 is weird. Tests should be separate from the rpy2 module (and hence the dependency of pytest superfluous).

2) should we instead wait for an upstream-fixed rpy2 (supposing that upstream agrees with you...) ?

YES

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -2,15 +2,12 @@

 Main changes:
 - `cygwin.patch` does not apply anymore
-- [pytest](https://pypi.org/project/pytest/) is a new dependency (!?)
-- [cffi>=1.13.1](https://pypi.org/project/cffi/) is a new dependency
-- [pycparser](https://pypi.org/project/pycparser/) is a new dependency (required by cffi)
+- [https://pypi.org/project/pytest/](https://pypi.org/project/pytest/) is a new dependency (!?)
+- [https://pypi.org/project/cffi/ cffi>=1.13.1](https://pypi.org/project/cffi/ cffi>=1.13.1) is a new dependency
+- [https://pypi.org/project/pycparser/ pycparser](https://pypi.org/project/pycparser/ pycparser) is a new dependency (required by cffi)

 tarballs: 
 - https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
 - https://files.pythonhosted.org/packages/6d/4e/572aed20422dee7fa2bd27995b2a53a32de90c1826e5531c9df6d3ea77ed/pytest-5.4.1.tar.gz
 - https://files.pythonhosted.org/packages/05/54/3324b0c46340c31b909fcec598696aaec7ddc8c18a63f2db352562d3354c/cffi-1.14.0.tar.gz
 - https://files.pythonhosted.org/packages/0f/86/e19659527668d70be91d0369aeaa055b4eb396b0f387a4f92293a20035bd/pycparser-2.20.tar.gz
-
-upstream issues and PR
-- [issues #670 (about tests in rpy2 and pytest dependency)](https://github.com/rpy2/rpy2/issues/670)

mkoeppe commented 4 years ago

comment:20

rpy2 should be patched so that pytest is not an actual dependency, but test-only.

rpy2 is missing modern metadata such as requirements.txt, test-requirements.txt, tox.ini where things like this are commonly declared.

28998 would install pytest only when SAGE_CHECK=yes.

mkoeppe commented 4 years ago

comment:21

By the way, I recommend that you add the new upstream_url fields to checksum.ini. Makes it easier to test the ticket.

mkoeppe commented 4 years ago

comment:22

(See #29425 for an example)

videlec commented 4 years ago

comment:23

Replying to @mkoeppe:

Description modified (diff)

@mkoeppe: Did you intentionally erase my modifications to the ticket description!?

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

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

e672b6e add upstream_url field to rpy2 checksums.ini

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

Changed commit from c525fc2 to e672b6e

videlec commented 4 years ago

Description changed:

--- 
+++ 
@@ -2,12 +2,13 @@

 Main changes:
 - `cygwin.patch` does not apply anymore
-- [https://pypi.org/project/pytest/](https://pypi.org/project/pytest/) is a new dependency (!?)
-- [https://pypi.org/project/cffi/ cffi>=1.13.1](https://pypi.org/project/cffi/ cffi>=1.13.1) is a new dependency
-- [https://pypi.org/project/pycparser/ pycparser](https://pypi.org/project/pycparser/ pycparser) is a new dependency (required by cffi)
+- [cffi>=1.13.1](https://pypi.org/project/cffi/) is a new dependency
+- [pycparser](https://pypi.org/project/pycparser/) is a new dependency (required by cffi)

 tarballs: 
 - https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
-- https://files.pythonhosted.org/packages/6d/4e/572aed20422dee7fa2bd27995b2a53a32de90c1826e5531c9df6d3ea77ed/pytest-5.4.1.tar.gz
 - https://files.pythonhosted.org/packages/05/54/3324b0c46340c31b909fcec598696aaec7ddc8c18a63f2db352562d3354c/cffi-1.14.0.tar.gz
 - https://files.pythonhosted.org/packages/0f/86/e19659527668d70be91d0369aeaa055b4eb396b0f387a4f92293a20035bd/pycparser-2.20.tar.gz
+
+Upstream issues and PR
+- https://github.com/rpy2/rpy2/issues/670

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

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

86d0d17 let rpy2 not depend on pytest

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

Changed commit from e672b6e to 86d0d17

mkoeppe commented 4 years ago

comment:27

Replying to @videlec:

Replying to @mkoeppe:

Description modified (diff)

@mkoeppe: Did you intentionally erase my modifications to the ticket description!?

No, sorry! Looks like on this ticket trac is playing funny tricks

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

Changed commit from 86d0d17 to 5eee83e

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

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

`a1debab`	`cffi package (rpy2 dependency)`
`a8b6e66`	`pycparser (rpy2 dependency)`
`5eee83e`	`update rpy2 dependencies`

videlec commented 4 years ago

comment:29

Ready for tests (especially on cygwin), but not for merging!

mkoeppe commented 4 years ago

comment:30

If you want to test cygwin via github actions, be sure to use #29403

videlec commented 4 years ago

comment:32

upstream is not (totally) convinced by dropping the pytest dependency. I propose to however keep the patch in Sage (on archlinux rpy2 does not depend on pytest either).

I will add a commit that still installs the test as part of rpy2 (it did not make much sense to remove it).

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

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

308d39c still install tests along rpy2

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

Changed commit from 5eee83e to 308d39c

videlec commented 4 years ago

Author: Vincent Delecroix

videlec commented 4 years ago

Description changed:

--- 
+++ 
@@ -4,6 +4,9 @@
 - `cygwin.patch` does not apply anymore
 - [cffi>=1.13.1](https://pypi.org/project/cffi/) is a new dependency
 - [pycparser](https://pypi.org/project/pycparser/) is a new dependency (required by cffi)
+- add a small patch to `setup.py` that
+  - disables printing on `stdout` (that perturbs `pip`)
+  - removes the dependency to `pytest`

 tarballs: 
 - https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz

mkoeppe commented 4 years ago

comment:35

Needs rebasing

mkoeppe commented 4 years ago

comment:36

29813 adds `pytest`

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -8,6 +8,8 @@
   - disables printing on `stdout` (that perturbs `pip`)
   - removes the dependency to `pytest`

+https://pypi.org/project/rpy2/
+
 tarballs: 
 - https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
 - https://files.pythonhosted.org/packages/05/54/3324b0c46340c31b909fcec598696aaec7ddc8c18a63f2db352562d3354c/cffi-1.14.0.tar.gz

mkoeppe commented 4 years ago

comment:38

Latest rpy2 is now 3.3.3

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

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

`68ea7be`	`upgrade rpy2 version`
`f1cd4e2`	`remove cygwin.patch (rpy2)`
`68f6f92`	`add setup.patch (rpy2)`
`b550f43`	`add upstream_url field to rpy2 checksums.ini`
`2cd2191`	`let rpy2 not depend on pytest`
`b76ee25`	`cffi package (rpy2 dependency)`
`84ded30`	`pycparser (rpy2 dependency)`
`2e0fe73`	`update rpy2 dependencies`
`81f580e`	`still install tests along rpy2`

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

Changed commit from 308d39c to 81f580e

mkoeppe commented 4 years ago

comment:40

Rebased on 9.2.beta2

mkoeppe commented 4 years ago

Description changed:

--- 
+++ 
@@ -1,19 +1,15 @@
-The release 3.2.7 is only Python 3 compatible.
-
 Main changes:
+- [https://pypi.org/project/rpy2/](https://pypi.org/project/rpy2/), supports python >= 3.6
 - `cygwin.patch` does not apply anymore
-- [cffi>=1.13.1](https://pypi.org/project/cffi/) is a new dependency
-- [pycparser](https://pypi.org/project/pycparser/) is a new dependency (required by cffi)
+- [cffi>=1.13.1](https://pypi.org/project/cffi/) is a new dependency, we install latest 1.14.0 (supports python >= 3.2)
+- [pycparser](https://pypi.org/project/pycparser/) is a new dependency (required by cffi, supports python >= 3.4)
 - add a small patch to `setup.py` that
   - disables printing on `stdout` (that perturbs `pip`)
   - removes the dependency to `pytest`

-https://pypi.org/project/rpy2/

-tarballs: 
-- https://files.pythonhosted.org/packages/39/c0/61120f9dae06b4887426d229b68a7a5f0ca1f9cb3986319bb9484819a28d/rpy2-3.2.7.tar.gz
-- https://files.pythonhosted.org/packages/05/54/3324b0c46340c31b909fcec598696aaec7ddc8c18a63f2db352562d3354c/cffi-1.14.0.tar.gz
-- https://files.pythonhosted.org/packages/0f/86/e19659527668d70be91d0369aeaa055b4eb396b0f387a4f92293a20035bd/pycparser-2.20.tar.gz
+tarballs: see checksums.ini

 Upstream issues and PR
 - https://github.com/rpy2/rpy2/issues/670
+

sagemath / sage

upgrade rpy2 package 2.8.2 -> 3.3.5, upgrade R to 3.6.3, add new dependencies #29441

28998 would install pytest only when SAGE_CHECK=yes.

29813 adds `pytest`

sagemath / sage

upgrade rpy2 package 2.8.2 -> 3.3.5, upgrade R to 3.6.3, add new dependencies #29441

28998 would install pytest only when SAGE_CHECK=yes.

29813 adds pytest

29813 adds `pytest`