Remove sage_setup/fpickle_setup.py

Question

Remove sage_setup/fpickle_setup.py

Closed this issue 5 years ago · 21 comments

The file sage_setup/fpickle_setup.py does not seem to be needed anymore, so remove it. This also removes some uses of six in sage/setup.py.

Depends on #29702

CC: @fchapoton @jhpalmieri

Component: python3

Author: John Palmieri

Branch/Commit: 212d183

Reviewer: Matthias Koeppe

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

Answer 1 · 2020-07-03T07:03:36.000Z

Branch: u/mkoeppe/sage_setup__remove_use_of_six

Answer 2 · 2020-07-03T07:03:57.000Z

Commit: b1fb10c

Answer 3 · 2020-07-03T07:03:57.000Z

Last 10 new commits:

`f9a30f6`	`build/pkgs/sagelib/spkg-install: Fix up error exits`
`00a1d57`	`Merge branch 't/29411/make_sagelib_a_script_package' into t/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup`
`df3f05e`	`build/make/Makefile.in [SCRIPT_PACKAGE_templ]: cd into the SPKG directory; adjust spkg-install scripts`
`5372065`	`Merge branch 't/29793/script_packages_should_cd_into_the_spkg_directory' into t/29411/make_sagelib_a_script_package`
`c166b97`	`Merge branch 't/29411/make_sagelib_a_script_package' into t/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup`
`cc30471`	`build/bin/write-dockerfile.sh: Do not ADD removed file src/Makefile.in`
`8a41326`	`Merge branch 't/29411/make_sagelib_a_script_package' into t/29702/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup`
`a56dc35`	`Merge tag '9.2.beta1' into t/29702/public/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup`
`ee7cce8`	`Merge commit 'a56dc35031486df9378f17c2d2ae6405946fac25' of git://trac.sagemath.org/sage into t/30011/sage_setup__remove_use_of_six`
`b1fb10c`	`src/sage_setup/fpickle_setup.py: Remove use of six`

Answer 4 · 2020-07-03T07:03:57.000Z

Author: Matthias Koeppe

Answer 5 · 2020-07-03T07:04:12.000Z

comment:4

Just 1 commit on top of #29702

Answer 6 · 2020-07-03T07:04:50.000Z

Description changed:

--- 
+++ 
@@ -1 +1 @@
-
+`sage_setup.docbuild` will be done on a separate ticket.

Answer 7 · 2020-07-03T20:34:07.000Z

Description changed:

--- 
+++ 
@@ -1 +1 @@
-`sage_setup.docbuild` will be done on a separate ticket.
+`sage_setup.docbuild` will be done on a separate ticket (#30010).

Answer 8 · 2020-07-07T20:20:56.000Z

comment:7

A few questions:

can we accomplish the same thing by installing twisted (after having removed it in #29754), and then importing twisted.persisted.styles? That is, can we delete fpickle_setup.py?
if not, then should we update that file with the latest version from twisted, for py3 compatibility?

To be honest, I don't really know what fpickle_setup.py is supposed to do, or where it is tested in the Sage library, or what goes wrong if we remove its functionality.

Answer 9 · 2020-07-07T21:17:43.000Z

comment:8

Maybe it's not worth it to install the most recent twisted, since it depends on a lot of other packages, most of which we don't install right now:

zope.interface>=4.4.2
constantly>=15.1
incremental>=16.10.1
Automat>=0.3.0
hyperlink>=17.1.1
PyHamcrest!=1.10.0,>=1.9.0
attrs>=19.2.0

[all_non_platform]
pyopenssl>=16.0.0
service_identity>=18.1.0
idna!=2.3,>=0.6
pyasn1
cryptography>=2.5
appdirs>=1.4.0
bcrypt>=3.0.0
soappy
pyserial>=3.0
h2<4.0,>=3.0
priority<2.0,>=1.1.0

Answer 10 · 2020-07-07T22:10:34.000Z

comment:9

Replying to @jhpalmieri:

To be honest, I don't really know what fpickle_setup.py is supposed to do, or where it is tested in the Sage library, or what goes wrong if we remove its functionality.

Neither do I.

Answer 11 · 2020-07-07T22:25:03.000Z

comment:10

I'm testing out an updated version of fpickle_setup from version 20.3.0 of twisted. If it passes tests, I'll push the branch here.

Answer 12 · 2020-07-08T01:10:38.000Z

comment:11

It worked with an updated version, but make ptestlong also passed with this change instead:

diff --git a/src/setup.py b/src/setup.py
index 72c59ace71..9a41ce1535 100755
--- a/src/setup.py
+++ b/src/setup.py
@@ -20,7 +20,7 @@ from sage_setup import excepthook
 sys.excepthook = excepthook
 
 # This import allows instancemethods to be pickable
-import sage_setup.fpickle_setup
+# import sage_setup.fpickle_setup
 
 #########################################################
 ### List of Extensions

See #11874 for some information. I certainly don't see the failure there: comment:46, "PicklingError: Can't pickle <type 'instancemethod'>: attribute lookup builtin.instancemethod failed".

Should we just get rid of fpickle_setup.py instead?

Answer 13 · 2020-07-08T01:46:07.000Z

comment:12

Replying to @jhpalmieri:

Should we just get rid of fpickle_setup.py instead?

Sounds good.

Answer 14 · 2020-07-08T02:04:45.000Z

Changed branch from u/mkoeppe/sage_setup__remove_use_of_six to u/jhpalmieri/sage_setup__remove_use_of_six

Answer 15 · 2020-07-08T02:06:19.000Z

Changed commit from b1fb10c to 212d183

Answer 16 · 2020-07-08T02:06:19.000Z

New commits:

`212d183`	`trac 30011: remove sage_setup/fpickle_setup.py`

Answer 17 · 2020-07-08T02:06:19.000Z

Changed author from Matthias Koeppe to John Palmieri

Answer 18 · 2020-07-08T02:06:19.000Z

Description changed:

--- 
+++ 
@@ -1 +1 @@
-`sage_setup.docbuild` will be done on a separate ticket (#30010).
+The file `sage_setup/fpickle_setup.py` does not seem to be needed anymore, so remove it. This also removes some uses of `six` in `sage/setup.py`.

Answer 19 · 2020-07-08T02:16:35.000Z

comment:15

I am testing this with fedora-28-standard, which uses system python 3.6.5

Answer 20 · 2020-07-08T05:13:29.000Z

Reviewer: Matthias Koeppe

Answer 21 · 2020-07-10T19:33:40.000Z

Changed branch from u/jhpalmieri/sage_setup__remove_use_of_six to 212d183