Opened 2 years ago

Closed 2 years ago

#30011 closed enhancement (fixed)

Remove sage_setup/fpickle_setup.py

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: python3 Keywords:
Cc: chapoton, jhpalmieri Merged in:
Authors: John Palmieri Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 212d183 (Commits, GitHub, GitLab) Commit: 212d1833db7c11f01a36bab5551eac15763ec77a
Dependencies: #29702 Stopgaps:

Status badges

Description (last modified by jhpalmieri)

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.

Change History (17)

comment:1 Changed 2 years ago by mkoeppe

  • Cc jhpalmieri added

comment:2 Changed 2 years ago by mkoeppe

  • Branch set to u/mkoeppe/sage_setup__remove_use_of_six

comment:3 Changed 2 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to b1fb10c74809fdbebd7158fb73d69bef2537b065
  • Status changed from new to needs_review

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

comment:4 Changed 2 years ago by mkoeppe

Just 1 commit on top of #29702

comment:5 Changed 2 years ago by mkoeppe

  • Description modified (diff)

comment:6 Changed 2 years ago by mkoeppe

  • Description modified (diff)

comment:7 follow-up: Changed 2 years ago by jhpalmieri

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.

comment:8 Changed 2 years ago by jhpalmieri

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

comment:9 in reply to: ↑ 7 Changed 2 years ago by mkoeppe

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.

comment:10 Changed 2 years ago by jhpalmieri

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.

comment:11 follow-up: Changed 2 years ago by jhpalmieri

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

  • src/setup.py

    diff --git a/src/setup.py b/src/setup.py
    index 72c59ace71..9a41ce1535 100755
    a b from sage_setup import excepthook 
    2020sys.excepthook = excepthook
    2121
    2222# This import allows instancemethods to be pickable
    23 import sage_setup.fpickle_setup
     23# import sage_setup.fpickle_setup
    2424
    2525#########################################################
    2626### 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?

comment:12 in reply to: ↑ 11 Changed 2 years ago by mkoeppe

Replying to jhpalmieri:

Should we just get rid of fpickle_setup.py instead?

Sounds good.

comment:13 Changed 2 years ago by jhpalmieri

  • Branch changed from u/mkoeppe/sage_setup__remove_use_of_six to u/jhpalmieri/sage_setup__remove_use_of_six

comment:14 Changed 2 years ago by jhpalmieri

  • Authors changed from Matthias Koeppe to John Palmieri
  • Commit changed from b1fb10c74809fdbebd7158fb73d69bef2537b065 to 212d1833db7c11f01a36bab5551eac15763ec77a
  • Description modified (diff)
  • Summary changed from sage_setup: Remove use of six to Remove sage_setup/fpickle_setup.py

New commits:

212d183trac 30011: remove sage_setup/fpickle_setup.py

comment:15 Changed 2 years ago by mkoeppe

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

comment:16 Changed 2 years ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:17 Changed 2 years ago by vbraun

  • Branch changed from u/jhpalmieri/sage_setup__remove_use_of_six to 212d1833db7c11f01a36bab5551eac15763ec77a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.