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

embray commented 6 years ago

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

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

embray commented 6 years ago

Description changed:

--- 
+++ 
@@ -10,7 +10,7 @@

       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 #?????).
+      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 #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.

saraedum commented 6 years ago

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 commented 6 years ago

comment:4

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

saraedum commented 6 years ago

Reviewer: Julian Rüth

embray commented 6 years ago

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.

embray commented 6 years ago

Description changed:

--- 
+++ 
@@ -14,7 +14,7 @@

 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 post-removal steps.  Other packages will need this as well, but less crucially.  I haven't found a specific use case yet for `spkg-prerm`, but it's included for symmetry.
+   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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 6184243 to 20a57d4

embray commented 6 years ago

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 commented 6 years ago

Changed dependencies from #24645 to #24645 #25039

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 20a57d4 to 8470eba

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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

95ed45e minor correction to comment

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 8470eba to 95ed45e

embray commented 6 years ago

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 commented 6 years ago

comment:12

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 95ed45e to d364c6c

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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 commented 6 years ago

comment:14

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from d364c6c to 1c8fca3

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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 commented 6 years ago

comment:16

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

saraedum commented 6 years ago

comment:18

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

embray commented 6 years ago

comment:19

Looks like just a merge conflict, yet again.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 1c8fca3 to 7a81e70

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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 commented 6 years ago

comment:21

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

saraedum commented 6 years ago

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 commented 6 years ago

comment:24

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

vbraun commented 6 years ago

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

sagemath / sage

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