Opened 2 years ago

Last modified 15 months ago

#25140 new task

Add spkg-legacy-uninstall scripts for most standard packages

Reported by: embray Owned by: embray
Priority: major Milestone: sage-pending
Component: build Keywords: uninstall
Cc: Merged in:
Authors: Erik Bray Reviewers: Julian Rüth
Report Upstream: N/A Work issues:
Branch: u/embray/build/spkg-uninstall/legacy-uninstallers (Commits) Commit: e2d2b01f15e25c24334d8e6f110c3302d41cdd6f
Dependencies: Stopgaps:

Description (last modified by embray)

This is a followup to #25139 and represents work that was originally done in #22510 to convert most packages to use spkg-legacy-uninstall and remove uninstall-related commands from their spkg-install scripts.

This work was brought over from an older branch and still needs some adjustment: It should incorporate the changes from the #24024 metaticket which conflict with some of these changes. There are also some additional optional and experimental packages (and maybe a couple standard packages) that still need to be updated as well.

Probably won't bother with that until and unless #25139 is approved of in some form that still includes the spkg-legacy-uninstall feature.


To summarize what this is about: Part of the goal in implementing the new installation/uninstallation system for packages is that spkg-install should, ideally, not modify $SAGE_LOCAL in any way, at least for packages that support DESTDIR-based installation. This allows more separation from the building of packages from their runtime installation target.

However, many packages' spkg-install or spkg-build include commands (mostly in the form of rm -rf's) to clean up files from previous installs of those packages. This is no longer necessary for new-style package installations that track exactly what files are installed by a package. But it is still necessary when upgrading from old installs of packages that don't have a file manifest. This is what the spkg-legacy-uninstall script is for. It's just taking the rm -rf's from spkg-install and moving it into a separate script that is called by the uninstaller if it doesn't have a file manifest for that package.

So this ticket is just creating the spkg-legacy-uninstall for many packages that already have DESTDIR support.

Change History (28)

comment:1 Changed 2 years ago by embray

  • Component changed from PLEASE CHANGE to build
  • Keywords uninstall added

comment:2 Changed 2 years ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:3 Changed 2 years ago by embray

  • Dependencies #25139 deleted

comment:4 Changed 2 years ago by git

  • Commit changed from 219150c1039eb86fabf39a10530cc869b6924580 to 58ad74e5c9f64de5b8780083273e5d6be8f2fcb2

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

58ad74eFix spkg-installs of packages with legacy uninstall commands; moving those commands into spkg-legacy-uninstall scripts

comment:5 Changed 2 years ago by embray

  • Status changed from new to needs_review

As a reminder, the goal of this is to make it so that spkg-install scripts do not modify the contents of $SAGE_LOCAL directly.

comment:6 Changed 2 years ago by git

  • Commit changed from 58ad74e5c9f64de5b8780083273e5d6be8f2fcb2 to e6f189ed4c331456beda0ede5edf00329d90d7d8

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

e6f189eadd spkg-legacy-uninstall for a few more packages that I missed

comment:7 Changed 2 years ago by embray

Added spkg-legacy-uninstall for a few packages that could use it.

This is now done for at least most, if not all packages that have DESTDIR support from #24024. It doesn't make sense to do for packages that haven't been converted to the new install/uninstall method yet. However, for any remaining packages that need to be converted, they should also get an spkg-legacy-uninstall at the same time, where appropriate.

comment:8 Changed 23 months ago by saraedum

  • Reviewers set to Julian Rüth

Feel free to set this to positive review if you are confident that this still works.

comment:9 Changed 23 months ago by embray

  • Status changed from needs_review to needs_work

The pyflakes plugin found an actual bug in the elliptic_curves package; it wasn't otherwise caught since I didn't bump the package versions for this (it isn't really necessary to).

I think I should fix that, and test doing a forced-reinstall of all these packages. I know I did that a long time ago back when this was part of #22510, but that was then...

Last edited 23 months ago by embray (previous) (diff)

comment:10 Changed 23 months ago by git

  • Commit changed from e6f189ed4c331456beda0ede5edf00329d90d7d8 to d45b4afa37e45cef68250a64d00661cff3a750be

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

b4e6851Fix spkg-installs of packages with legacy uninstall commands; moving those commands into spkg-legacy-uninstall scripts
d45b4afadd spkg-legacy-uninstall for a few more packages that I missed

comment:11 Changed 23 months ago by embray

  • Milestone changed from sage-8.3 to sage-8.4

I believe this issue can reasonably be addressed for Sage 8.4.

comment:12 Changed 23 months ago by embray

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:13 follow-up: Changed 22 months ago by jdemeyer

  • Status changed from needs_review to needs_work

First of all, there are merge conflicts.

Second, do we really need this? I understand that you want to clean up the spkg-install scripts, but we can do that gradually as we upgrade packages anyway. I don't really see the point of adding a whole new feature only for backwards compatibility in cases where it almost never matters (typically, it's not a problem to skip the uninstall).

I'm not actively against this ticket, it's just that I don't see the point.

comment:14 in reply to: ↑ 13 Changed 22 months ago by embray

Replying to jdemeyer:

First of all, there are merge conflicts.

Well I mean, there weren't 2 weeks ago and that's obviously resolvable so it's not a big deal.

Second, do we really need this? I understand that you want to clean up the spkg-install scripts, but we can do that gradually as we upgrade packages anyway. I don't really see the point of adding a whole new feature only for backwards compatibility in cases where it almost never matters (typically, it's not a problem to skip the uninstall).

I'm not actively against this ticket, it's just that I don't see the point.

I'm not in any hurry on this and am also happy to do it gradually. Though since it's already done I don't see the point in not just going ahead and merging this now either. It would be more work at this point not to.

comment:15 Changed 22 months ago by git

  • Commit changed from d45b4afa37e45cef68250a64d00661cff3a750be to bafe689b2ded2440c12fcaba9c5928700d514114

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

801a2c3Fix spkg-installs of packages with legacy uninstall commands; moving those commands into spkg-legacy-uninstall scripts
bafe689add spkg-legacy-uninstall for a few more packages that I missed

comment:16 Changed 22 months ago by embray

  • Status changed from needs_work to needs_review

comment:17 Changed 22 months ago by saraedum

  • Status changed from needs_review to positive_review

Looks good to me.

comment:18 Changed 22 months ago by vbraun

  • Branch changed from u/embray/build/spkg-uninstall/legacy-uninstallers to bafe689b2ded2440c12fcaba9c5928700d514114
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 22 months ago by vbraun

  • Commit bafe689b2ded2440c12fcaba9c5928700d514114 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

When reinstalling an already installed elliptic_curves:

$ ./sage -p elliptic_curves
[...]
No record that 'elliptic_curves' was ever installed; skipping uninstall
Creating database /mnt/disk/home/release/Sage/local/share/cremona/cremona_mini.db
Traceback (most recent call last):
  File "spkg-install.py", line 89, in <module>
    install_cremona()
  File "spkg-install.py", line 31, in install_cremona
    con.execute('CREATE TABLE t_class(rank INTEGER, class TEXT PRIMARY KEY,'
sqlite3.OperationalError: table t_class already exists

real	0m0.161s
user	0m0.053s
sys	0m0.028s
************************************************************************
Error installing package elliptic_curves-0.8.p0
************************************************************************

This breaks incremental rebuilds...

comment:20 Changed 22 months ago by vbraun

Please check that all packages can be installed twice before settings back to positive review.

comment:21 Changed 22 months ago by embray

  • Branch changed from bafe689b2ded2440c12fcaba9c5928700d514114 to u/embray/build/spkg-uninstall/legacy-uninstallers
  • Commit set to bafe689b2ded2440c12fcaba9c5928700d514114

New commits:

801a2c3Fix spkg-installs of packages with legacy uninstall commands; moving those commands into spkg-legacy-uninstall scripts
bafe689add spkg-legacy-uninstall for a few more packages that I missed

comment:22 Changed 22 months ago by git

  • Commit changed from bafe689b2ded2440c12fcaba9c5928700d514114 to 6f82064665b2996cac072fce0152531da1271735

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

6f82064revert changes to elliptic_curves; it will be handled in a separate ticket

comment:23 Changed 22 months ago by embray

Yes, there were still several problems with the spkg-install for elliptic_curves; I think it should just be updated in a separate ticket. It should not have been included in this ticket in the first place since it was only intending to update packages that already had working DESTDIR support.


New commits:

6f82064revert changes to elliptic_curves; it will be handled in a separate ticket

comment:24 Changed 19 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:25 Changed 17 months ago by embray

  • Milestone changed from sage-8.5 to sage-8.7

Retargeting some of my tickets (somewhat optimistically for now).

comment:26 Changed 17 months ago by git

  • Commit changed from 6f82064665b2996cac072fce0152531da1271735 to e2d2b01f15e25c24334d8e6f110c3302d41cdd6f

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

e2d2b01Fix spkg-installs of packages with legacy uninstall commands; moving those commands into spkg-legacy-uninstall scripts

comment:27 Changed 17 months ago by embray

  • Owner changed from (none) to embray

Rebased this, though some of these are now handled in other tickets so I need to clean it up a bit more (focus only on packages that already otherwise have been fixed as part of #24024).

comment:28 Changed 15 months ago by embray

  • Milestone changed from sage-8.7 to sage-pending

Removing most of the rest of my open tickets out of the 8.7 milestone, which should be closed.

Note: See TracTickets for help on using tickets.