Add sage-spkg-uninstall script and use it when possible to remove packages

[embray]

This is a cleaned up an simplified version of a portion of the work originally done in #22510. This ticket adds a few new features that I felt were fundamentally inseparable (in part due to peculiarities of specific packages meaning that uninstallation could not be supported without some additional work).

1. Adds a new script sage-spkg-uninstall which is invoked like sage-spkg-uninstall <pkgname> to uninstall a single package from $SAGE_LOCAL (this also allows uninstalling standard packages, though we might prefer to prevent that by default). This script handles all matters of package uninstallation, including the final step of removing the package stamp file from under $SAGE_SPKG_INST. There are two different ways a package can be uninstalled:

   a. "Modern" uninstallation: if a package has staged-installation support (i.e. has been updated to support Install packages in temporary root before copying to $SAGE_LOCAL #22509), this means that its stamp file contains a manifest of all files installed by that package (minus any files created by the package's spkg-postinst script). Therefore "modern" uninstallation consists mainly of looping over those files and removing them from $SAGE_LOCAL. Any directories left empty by this process are also removed.

   b. "Legacy" uinstallation: many (though not all) packages have in their spkg-install script some code to perform an ad-hoc uninstallation of any previous versions of the package before installing the new version (this was sometimes necessary in particular for old versions of libraries). Support for this functionality is retained in order to properly support upgrades from older versions of Sage, where most packages may not support "modern" uninstallation.

   However, in order to support this properly, the legacy uninstall code for those packages should be moved out of their spkg-install scripts and into a new spkg-legacy-uninstall script which is called by sage-spkg-uninstall for these packages. This ticket includes one such example for demonstration purposes, adding an spkg-legacy-uninstall for brial.

   In the future we may be able to deprecate, and ultimately remove these scripts. In the meantime all existing packages with uninstall code in their spkg-install should be updated (see followup ticket Add spkg-legacy-uninstall scripts for most standard packages #25140).

2. Another feature this adds which is handled by sage-spkg-uninstall, but with some required support from the sage-spkg script, are per-package spkg-prerm and spkg-postrm scripts that are run just before and just after a package is uninstalled.

   This is needed in particular for some packages (namely pkgconf and widgetsnbextension) which really can't be removed properly without some pre/post-removal steps. Other packages will need this as well, but less crucially. I haven't found a specific use case yet for spkg-postrm, but it's included for symmetry.

   Because these scripts are run during package uninstallation--of packages that are already installed--the scripts themselves need to be installed somewhere (e.g. Debian supports a similar feature, and keeps the scripts under /var/lib/dpkg somewhere). In this case a new path SAGE_SPKG_SCRIPTS is added under $SAGE_LOCAL/var/lib/sage/scripts by default. Any package-specific prerm and postrm scripts are stored there. After the package is uninstalled the scripts are removed as well.

3. The Makefile is updated so that the <pkgname>-clean rules for most packages call sage-spkg-uninstall, so that the packages are really thoroughly cleaned (rather than just removing their stamp files).

Depends on #24645
Depends on #25039

Component: build

Keywords: uninstall

Author: Erik Bray

Branch/Commit: 7a81e70

Reviewer: Julian Rüth

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

[saraedum]

comment:3

• There is a TODO: in your changeset. Should that be resolved now?
• I know you just copied code over in build/pkgs/pkgconf/spkg-postinst but shouldn't all return values be checked (or a set -e be set?)
• Should we check the return value in build/pkgs/widgetsnbextension/spkg-prerm?
• Why did you push the patch level in build/pkgs/widgetsnbextension/package-version.txt
• Why did you create a "docstring" here and not just a comment?

+PKGS = pth.join(SAGE_ROOT, 'build', 'pkgs')
+"""Directory where all spkg sources are found."""

• SAGE_SPKG_INST is always set, isn't it? Maybe it should then just be an error here if it isn't instead of copying the default from somewhere else?

+    spkg_inst = pth.join(sage_local, 'var', 'lib', 'sage', 'installed')
+    spkg_inst = os.environ.get('SAGE_SPKG_INST', spkg_inst)

and similarly for SAGE_SPKG_SCRIPTS.

• postrm should read prerm here I guess?

+    # Run the package's postrm script, if it exists
+    # If an error occurs here we abort the uninstallation for now
+    run_spkg_script(spkg_name, spkg_scripts, 'prerm', 'pre-uninstall')

[saraedum]

comment:4

Overall this looks good to me. It's great that we finally get a feature like that in Sage :)

[saraedum]

Reviewer: Julian Rüth

[embray]

comment:6

Thanks for having a look!

Replying to @saraedum:

• There is a TODO: in your changeset. Should that be resolved now?

I don't think that should be resolved now--the comment was just a note that it would be the correct spot, most likely, to start implementing such functionality. Thanks for reminding me about that though. There should be a follow-up ticket for it.

• I know you just copied code over in build/pkgs/pkgconf/spkg-postinst but shouldn't all return values be checked (or a set -e be set?)

I agree, there should be some error checking there. It could also use sdh_install from #25039 for the copy, which would take care of error checking.

• Should we check the return value in build/pkgs/widgetsnbextension/spkg-prerm?

I'm not really sure it's necessary or desirable. There are various reasons it could fail, such as if that action had already (whether or not for good reasons) been performed by the user, or if the installation was otherwise broken somehow. In this case other things might break too, but at the very least you want to still allow the rest of the package uninstallation to proceed.

• Why did you push the patch level in build/pkgs/widgetsnbextension/package-version.txt

The pre/postrm scripts have to be installed, so that means a version bump is needed here to make sure the package is upgraded in order to install them.

• Why did you create a "docstring" here and not just a comment?

+PKGS = pth.join(SAGE_ROOT, 'build', 'pkgs')
+"""Directory where all spkg sources are found."""

It's standard syntax understood by Sphinx for documenting module-level variables and class attributes. Some people don't like it, apparently, but I'm personally quite partial to it for documenting variables, even in code that won't necessarily be processed by Sphinx.

• SAGE_SPKG_INST is always set, isn't it? Maybe it should then just be an error here if it isn't instead of copying the default from somewhere else?

+    spkg_inst = pth.join(sage_local, 'var', 'lib', 'sage', 'installed')
+    spkg_inst = os.environ.get('SAGE_SPKG_INST', spkg_inst)

and similarly for SAGE_SPKG_SCRIPTS.

Maybe it's not necessary, but I wrote sage-spkg-uninstall such that it works without necessarily instantiating the full "Sage environment" (I don't like how many programs in Sage do rely on this). In fact you can pass it the path to $SAGE_LOCAL as an optional argument (which is otherwise read out of the environment). One thing I don't like about this is that we don't exactly have an easy-to-parse file providing the default values for various environment variables used by Sage and its tooling. But that's something to resolve elsewhere...

• postrm should read prerm here I guess?

+    # Run the package's postrm script, if it exists
+    # If an error occurs here we abort the uninstallation for now
+    run_spkg_script(spkg_name, spkg_scripts, 'prerm', 'pre-uninstall')

Yep. And since you pointed that out I also noticed some other fishy business going on at the bottom of the modern_uninstall function that it looks like I might have been sloppy about.

[sagetrac-git]

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

86f3480	Better handling of missing directories when uninstalling broken packages
7998223	Add the ability for packages to have a pre-uninstall script too.
3a26636	It seems the best way to handle notebook extensions is to disable them in a prerm script.
dad9493	These packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
12265b7	Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
b6859b1	Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
281ce23	similarly, this variable should be read out of the environment if possible
c7542de	Add a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
ee018bd	Simplify directory removal
20a57d4	use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install

[sagetrac-git]

Changed commit from 6184243 to 20a57d4

[embray]

Last 10 new commits:

86f3480	Better handling of missing directories when uninstalling broken packages
7998223	Add the ability for packages to have a pre-uninstall script too.
3a26636	It seems the best way to handle notebook extensions is to disable them in a prerm script.
dad9493	These packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
12265b7	Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
b6859b1	Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
281ce23	similarly, this variable should be read out of the environment if possible
c7542de	Add a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
ee018bd	Simplify directory removal
20a57d4	use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install

[embray]

Changed dependencies from #24645 to #24645 #25039

[sagetrac-git]

Changed commit from 20a57d4 to 8470eba

[sagetrac-git]

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

213c039	These packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
1331c73	Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
6c61d68	Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
654456d	similarly, this variable should be read out of the environment if possible
760d98b	Add a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
7597001	Simplify directory removal
9d76043	use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install
554d47a	remove this TODO comment--I have opened an issue for this task at https://github.com/sagemath/sage/issues/25202
3f528d8	Use appropariate sage-dist-helpers in the pkg-config postinst script
8470eba	Better error handling with spkg-prerm exits with an error (before it would have just had an unhandled exception and dumped a traceback)

[sagetrac-git]

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

95ed45e

minor correction to comment

[sagetrac-git]

Changed commit from 8470eba to 95ed45e

[embray]

comment:11

I believe I addressed all your comments, and made all the code updates I agreed were needed (especially cleaning up sage_bootstrap.uninstall a bit). If you don't agree with any of my responses above we can still discuss that though. I'm not too hard-set on any of the details of this.

[embray]

comment:12

Though before anyone reviews this further they should probably look at #24645 first.

[sagetrac-git]

Changed commit from 95ed45e to d364c6c

[sagetrac-git]

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

db87c9b	These packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
bf535f6	Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
82b7be5	Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
ec074d6	similarly, this variable should be read out of the environment if possible
bbdb221	Add a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
c17b46d	Simplify directory removal
8907458	use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install
98a1f8d	Use appropariate sage-dist-helpers in the pkg-config postinst script
8f500c6	Better error handling with spkg-prerm exits with an error (before it would have just had an unhandled exception and dumped a traceback)
d364c6c	minor correction to comment

[embray]

comment:14

Something in the last patchbot build went awry with brial. I'm not sure I understand the issue yet.

[sagetrac-git]

Changed commit from d364c6c to 1c8fca3

[sagetrac-git]

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

2faf376	These packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
55c21de	Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
ce6c825	Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
8c2ae0c	similarly, this variable should be read out of the environment if possible
ac5f16d	Add a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
a31606d	Simplify directory removal
886b7cb	use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install
a2f189b	Use appropariate sage-dist-helpers in the pkg-config postinst script
2c2152d	Better error handling with spkg-prerm exits with an error (before it would have just had an unhandled exception and dumped a traceback)
1c8fca3	minor correction to comment

[embray]

comment:16

Ok, the issue is fixed in #24645. The next patchbot build should be good but we'll see.

[saraedum]

comment:18

The patchbot still seems to be unhappy. I do not understand what's going on there…

[embray]

comment:19

Looks like just a merge conflict, yet again.

[sagetrac-git]

Changed commit from 1c8fca3 to 7a81e70

[sagetrac-git]

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

45e39e5	Add the ability for packages to have a pre-uninstall script too.
c167505	Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
df2af7d	Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
d720715	similarly, this variable should be read out of the environment if possible
4721f40	Add a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
700f764	Simplify directory removal
f2ef70e	use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install
e8d06a5	Use appropariate sage-dist-helpers in the pkg-config postinst script
10def6c	Better error handling with spkg-prerm exits with an error (before it would have just had an unhandled exception and dumped a traceback)
7a81e70	minor correction to comment

[embray]

comment:21

Rebased, and removed the stuff related with widgetsnbextension which as jdemeyer pointed out in #24646 should no longer be relevant.

[saraedum]

comment:23

Looks good to me. I have not tested this though. If you are confident that this works, feel free to set it to positive review.

[embray]

comment:24

Will be interesting to see what the buildbots do with it, especially OSX. I expect it will be fine.

[vbraun]

Changed branch from u/embray/build/spkg-uninstall/script to 7a81e70