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

Add spkg-legacy-uninstall scripts for most standard packages #25140

Closed
embray opened this issue Apr 10, 2018 · 36 comments · Fixed by #36839
Closed

Add spkg-legacy-uninstall scripts for most standard packages #25140

embray opened this issue Apr 10, 2018 · 36 comments · Fixed by #36839

Comments

@embray
Copy link
Contributor

embray commented Apr 10, 2018

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.

Component: build

Keywords: uninstall

Author: Erik Bray

Branch/Commit: u/embray/build/spkg-uninstall/legacy-uninstallers @ e2d2b01

Reviewer: Julian Rüth

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

@embray embray added this to the sage-8.2 milestone Apr 10, 2018
@embray
Copy link
Contributor Author

embray commented Apr 10, 2018

Changed keywords from none to uninstall

@embray embray modified the milestones: sage-8.2, sage-8.3 Apr 26, 2018
@embray
Copy link
Contributor Author

embray commented Jul 10, 2018

Changed dependencies from #25139 to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2018

Changed commit from 219150c to 58ad74e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2018

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

@embray
Copy link
Contributor Author

embray commented Jul 10, 2018

comment:5

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2018

Changed commit from 58ad74e to e6f189e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2018

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

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

@embray
Copy link
Contributor Author

embray commented Jul 11, 2018

comment:7

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.

@saraedum
Copy link
Member

comment:8

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

@saraedum
Copy link
Member

Reviewer: Julian Rüth

@embray
Copy link
Contributor Author

embray commented Jul 17, 2018

comment:9

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...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2018

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2018

Changed commit from e6f189e to d45b4af

@embray
Copy link
Contributor Author

embray commented Jul 18, 2018

comment:11

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

@embray embray modified the milestones: sage-8.3, sage-8.4 Jul 18, 2018
@embray

This comment has been minimized.

@jdemeyer
Copy link

jdemeyer commented Aug 7, 2018

comment:13

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.

@vbraun vbraun closed this as completed Aug 11, 2018
@vbraun
Copy link
Member

vbraun commented Aug 12, 2018

Changed commit from bafe689 to none

@vbraun
Copy link
Member

vbraun commented Aug 12, 2018

comment:19

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...

@vbraun vbraun reopened this Aug 12, 2018
@vbraun
Copy link
Member

vbraun commented Aug 12, 2018

comment:20

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

@embray
Copy link
Contributor Author

embray commented Aug 14, 2018

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

@embray
Copy link
Contributor Author

embray commented Aug 14, 2018

Changed branch from bafe689 to u/embray/build/spkg-uninstall/legacy-uninstallers

@embray
Copy link
Contributor Author

embray commented Aug 14, 2018

Commit: bafe689

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 14, 2018

Changed commit from bafe689 to 6f82064

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 14, 2018

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

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

@embray
Copy link
Contributor Author

embray commented Aug 14, 2018

comment:23

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

@embray embray modified the milestones: sage-8.4, sage-8.5 Oct 28, 2018
@embray
Copy link
Contributor Author

embray commented Dec 28, 2018

comment:25

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

@embray embray modified the milestones: sage-8.5, sage-8.7 Dec 28, 2018
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 9, 2019

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 9, 2019

Changed commit from 6f82064 to e2d2b01

@embray
Copy link
Contributor Author

embray commented Jan 9, 2019

comment:27

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).

@embray embray self-assigned this Jan 9, 2019
@embray
Copy link
Contributor Author

embray commented Mar 25, 2019

comment:28

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

@mkoeppe
Copy link
Member

mkoeppe commented Dec 8, 2023

Different versions of the spkg-legacy-uninstall scripts were added to Sage in

vbraun pushed a commit to vbraun/sage that referenced this issue Dec 17, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

Most normal SPKGs are installed by staging in `DESTDIR`. When copying to
the final install location in `SAGE_LOCAL`, an installation record is
created, which is used later in package uninstallation.

However, when SPKG installation was switched to using staging in
`DESTDIR` (Meta-ticket sagemath#24024), the parts of `spkg-install` scripts that
used to be responsible for removing an old version of the installed
package were either kept in place or moved to `spkg-legacy-uninstall`
scripts. This was done to enable incremental builds from older
installations.

By passage of time, this is no longer needed.

Some of the removals are a partial cherry-pick from sagemath#25140 by @embray.

We also switch `frobby` to `DESTDIR` staging and *add* an `spkg-legacy-
uninstall` script.

Resolves sagemath#25140.
Resolves sagemath#30480.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36778 (merged here to resolve merge conflict)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36839
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants