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

build/pkgs/sagelib: Invoke install cleaner explicitly, remove it from pkgs/sagemath-standard/setup.py #32927

Closed
mkoeppe opened this issue Nov 24, 2021 · 48 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 24, 2021

Unless ./configure --enable-editable is used, the Sage library is installed directly into site-packages using setup.py install, and at the end of sage_setup's custom sage_install_and_clean invokes the installation cleaner, which removes "stale" installed files.

The installation cleaner is problematic when we start to install optional modules in the sage.* namespace via separate distributions such as sagemath-tdlib (#29864).

  • pkgs/sagemath-standard/setup.py will no longer enable optional Cython modules based on which optional packages are installed
  • but the installation cleaner needs to know about these optional Cython modules so it does not remove them right after their installation.

In this ticket we solve this problem by removing the use of the installation cleaner from pkgs/sagemath-standard/setup.py. Instead we invoke it separately at the end of the installation script build/pkgs/sagelib/spkg-install.

(An alternative solution, replacing this installation procedure by a standard installation procedure using wheels, as proposed in #32874, would make sage -b much slower. Also, the idea of just making ./configure --enable-editable the default, #32406, has met skepticism.)

Depends on #32874

CC: @kiwifb @jhpalmieri

Component: build

Work Issues: Do not build_ext

Branch/Commit: u/mkoeppe/pkgs_sagemath_standard_setup_py__move_distributions_logic_to_a_new_package_sagemath_optional @ edd0ea1

Reviewer: Kwankyu Lee

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

@mkoeppe mkoeppe added this to the sage-9.6 milestone Nov 24, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 26, 2021

Dependencies: #31386, #32899

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title pkgs/sagemath-standard/setup.py: Move distributions logic to a new package sagemath-optional build/pkgs/sagelib: Invoke install cleaner explicitly, remove it from pkgs/sagemath-standard/setup.py Nov 27, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 27, 2021

Author: Matthias Koeppe

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.5 Nov 27, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 27, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 27, 2021

Commit: 00d5f59

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 27, 2021

Last 10 new commits:

77c957fsrc/doc/en/developer/packaging_sage_library.rst: Put hierarchy section one level higher
9375ef8pkgs/sagemath-standard/tox.ini: Use SAGE_VENV or venv symlink to find wheels
c6fc111Prettier diagram
7d6a44cUse :mod: as markup for packages/modules
f83d424Improve ABC example
817a8d0src/doc/en/developer/packaging_sage_library.rst: More :mod: and :class: markup
b555735src/doc/en/developer/packaging_sage_library.rst: Link to pypi.org and to documentation of packaging metadata
81e9c9aMerge #32899
ecc0886Merge #31386
00d5f59build/pkgs/sagelib: Invoke install cleaner explicitly, remove it from pkgs/sagemath-standard/setup.py

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 18, 2021

comment:9

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2022

Changed commit from 00d5f59 to 2d08b69

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2022

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

2d08b69Merge tag '9.5.rc3' into t/32927/pkgs_sagemath_standard_setup_py__move_distributions_logic_to_a_new_package_sagemath_optional

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jan 23, 2022

Changed dependencies from #31386, #32899 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2022

Changed commit from 2d08b69 to fad8104

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 23, 2022

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

fad8104build/pkgs/sagelib/install-cleaner/setup.py: Remove primecount

@kiwifb
Copy link
Member

kiwifb commented Feb 6, 2022

comment:14

Is this installing a nested package? The kind of stuff that we finally ditched with R/rpy a few years ago.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 7, 2022

comment:15

No, the distribution in build/pkgs/sagelib/install-cleaner/ does not install anything and is not nested in anything. If it's clearer, I can also move it to pkgs/install-cleaner/

@kiwifb
Copy link
Member

kiwifb commented Feb 7, 2022

comment:16

I think that may be best. But I must say so far I am a bit confused. I do not see where that "install-cleaner" is ever used. Is it just functionality you are keeping in reserve for now or for a dependent ticket?

@kiwifb
Copy link
Member

kiwifb commented Feb 7, 2022

comment:23

Replying to @mkoeppe:

It's invoked by build/pkgs/sagelib/spkg-install

Oh yes I see. Like it says, that's not a bit I am usually concerned with downstream.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 7, 2022

comment:24

Yes, the only effect of this ticket on downstream packaging is that sagemath-standard's setup.py no longer does weird things with site-packages.

@kiwifb
Copy link
Member

kiwifb commented Feb 7, 2022

comment:25

If I was doing a bit of archeology on the sage-on-gentoo repo I think I would find bits where I suppressed some of that clean up. I seem to have let go of that part recently.

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 2, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 15, 2022

Dependencies: #33855

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2022

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

351cccdbuild/pkgs/sagelib: Invoke install cleaner explicitly, remove it from pkgs/sagemath-standard/setup.py
731e813build/pkgs/sagelib/install-cleaner/setup.py: Remove primecount
7667836pkgs/sage-legacy-install-cleaner: Move install cleaner here

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2022

Changed commit from 1db674d to 7667836

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 12, 2022

Changed dependencies from #33855 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2022

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

e26da73build/pkgs/sagelib/spkg-install: Use --build-base for cleaning

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2022

Changed commit from 7667836 to e26da73

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2022

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

ed680c5build/pkgs/sagelib: Invoke install cleaner explicitly, remove it from pkgs/sagemath-standard/setup.py
57044b6build/pkgs/sagelib/install-cleaner/setup.py: Remove primecount
fc34e2apkgs/sage-legacy-install-cleaner: Move install cleaner here
67d3940build/pkgs/sagelib/spkg-install: Use --build-base for cleaning
3fcce86pkgs/sage-legacy-install-cleaner/setup.py: Remove use of sage_build
e802eddbuild/pkgs/sagelib/spkg-install: Fix up
8bfdaeepkgs/sage-legacy-install-cleaner/setup.py: Restore multiprocessing fix
edd0ea1pkgs/sage-legacy-install-cleaner/setup.py: Update import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 12, 2022

Changed commit from e26da73 to edd0ea1

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 12, 2022

Changed work issues from FIXME in build/pkgs/sagelib/spkg-install to Do not build_ext

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 26, 2022

comment:34

Not needed with #32874

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 26, 2022

Dependencies: #32874

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 26, 2022

Changed author from Matthias Koeppe to none

@mkoeppe mkoeppe removed this from the sage-9.8 milestone Sep 26, 2022
@kwankyu
Copy link
Collaborator

kwankyu commented Oct 14, 2022

comment:35

Before we close this ticket, to clarify why this approach was abandoned, let me ask a couple of questions:

Would setup install work when optional distributions that contain namespace packages were installed?

Would legacy-install-cleaner work when optional distributions that contain namespace packages were installed?

If answers to both are yes, then the branch of this ticket could be an alternative to #32874?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 14, 2022

comment:36

Well, the branch here would have been a temporary solution, good enough until external packages arrive that fill the namespace sage.* and are maintained out of tree. Then #30152 would have been the next step, but I did not have an implementation strategy for it.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 15, 2022

comment:37

Okay. Thanks.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 15, 2022

Reviewer: Kwankyu Lee

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