Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove sage_setup/fpickle_setup.py #30011

Closed
mkoeppe opened this issue Jun 28, 2020 · 21 comments
Closed

Remove sage_setup/fpickle_setup.py #30011

mkoeppe opened this issue Jun 28, 2020 · 21 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Jun 28, 2020

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

@mkoeppe mkoeppe added this to the sage-9.2 milestone Jun 28, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 3, 2020

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 3, 2020

Commit: b1fb10c

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 3, 2020

Last 10 new commits:

f9a30f6build/pkgs/sagelib/spkg-install: Fix up error exits
00a1d57Merge 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
df3f05ebuild/make/Makefile.in [SCRIPT_PACKAGE_templ]: cd into the SPKG directory; adjust spkg-install scripts
5372065Merge branch 't/29793/script_packages_should_cd_into_the_spkg_directory' into t/29411/make_sagelib_a_script_package
c166b97Merge 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
cc30471build/bin/write-dockerfile.sh: Do not ADD removed file src/Makefile.in
8a41326Merge 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
a56dc35Merge tag '9.2.beta1' into t/29702/public/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup
ee7cce8Merge commit 'a56dc35031486df9378f17c2d2ae6405946fac25' of git://trac.sagemath.org/sage into t/30011/sage_setup__remove_use_of_six
b1fb10csrc/sage_setup/fpickle_setup.py: Remove use of six

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 3, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 3, 2020

comment:4

Just 1 commit on top of #29702

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@jhpalmieri
Copy link
Member

comment:7

A few questions:

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.

@jhpalmieri
Copy link
Member

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 7, 2020

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.

@jhpalmieri
Copy link
Member

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.

@jhpalmieri
Copy link
Member

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?

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 8, 2020

comment:12

Replying to @jhpalmieri:

Should we just get rid of fpickle_setup.py instead?

Sounds good.

@jhpalmieri
Copy link
Member

@jhpalmieri
Copy link
Member

Changed commit from b1fb10c to 212d183

@jhpalmieri
Copy link
Member

New commits:

212d183trac 30011: remove sage_setup/fpickle_setup.py

@jhpalmieri
Copy link
Member

Changed author from Matthias Koeppe to John Palmieri

@jhpalmieri

This comment has been minimized.

@jhpalmieri jhpalmieri changed the title sage_setup: Remove use of six Remove sage_setup/fpickle_setup.py Jul 8, 2020
@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 8, 2020

comment:15

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Jul 8, 2020

Reviewer: Matthias Koeppe

@vbraun
Copy link
Member

vbraun commented Jul 10, 2020

Changed branch from u/jhpalmieri/sage_setup__remove_use_of_six to 212d183

@vbraun vbraun closed this as completed in 72a9b1c Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants