Opened 2 years ago

Closed 23 months ago

#25139 closed enhancement (fixed)

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

Reported by: embray Owned by:
Priority: major Milestone: sage-8.3
Component: build Keywords: uninstall
Cc: Merged in:
Authors: Erik Bray Reviewers: Julian Rüth
Report Upstream: N/A Work issues:
Branch: 7a81e70 (Commits) Commit: 7a81e70eaebb7ec27f48614ab5232d7fccff4b74
Dependencies: #24645 #25039 Stopgaps:

Description (last modified by 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:

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

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

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

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

Change History (25)

comment:1 Changed 2 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 2 years ago by embray

  • Description modified (diff)

comment:3 follow-up: Changed 2 years ago by saraedum

  • 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')
    

comment:4 Changed 2 years ago by saraedum

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

comment:5 Changed 2 years ago by saraedum

  • Reviewers set to Julian Rüth
  • Status changed from needs_review to needs_work

comment:6 in reply to: ↑ 3 Changed 2 years ago by embray

  • Description modified (diff)

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.

comment:7 Changed 2 years ago by git

  • Commit changed from 6184243794db7392581b602d120e8c060d7adf9a to 20a57d430fd8bb9591dc47d4f8a317d95b23abf1

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

86f3480Better handling of missing directories when uninstalling broken packages
7998223Add the ability for packages to have a pre-uninstall script too.
3a26636It seems the best way to handle notebook extensions is to disable them in a prerm script.
dad9493These packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
12265b7Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
b6859b1Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
281ce23similarly, this variable should be read out of the environment if possible
c7542deAdd a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
ee018bdSimplify directory removal
20a57d4use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install

comment:8 Changed 2 years ago by embray

  • Dependencies changed from #24645 to #24645 #25039

Last 10 new commits:

86f3480Better handling of missing directories when uninstalling broken packages
7998223Add the ability for packages to have a pre-uninstall script too.
3a26636It seems the best way to handle notebook extensions is to disable them in a prerm script.
dad9493These packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
12265b7Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
b6859b1Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
281ce23similarly, this variable should be read out of the environment if possible
c7542deAdd a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
ee018bdSimplify directory removal
20a57d4use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install

comment:9 Changed 2 years ago by git

  • Commit changed from 20a57d430fd8bb9591dc47d4f8a317d95b23abf1 to 8470ebafa1b8a8e00b34409b2d14ec57843792e7

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

213c039These packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
1331c73Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
6c61d68Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
654456dsimilarly, this variable should be read out of the environment if possible
760d98bAdd a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
7597001Simplify directory removal
9d76043use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install
554d47aremove this TODO comment--I have opened an issue for this task at https://trac.sagemath.org/ticket/25202
3f528d8Use appropariate sage-dist-helpers in the pkg-config postinst script
8470ebaBetter error handling with spkg-prerm exits with an error (before it would have just had an unhandled exception and dumped a traceback)

comment:10 Changed 2 years ago by git

  • Commit changed from 8470ebafa1b8a8e00b34409b2d14ec57843792e7 to 95ed45ec061424d4708e724c425565e48faa479e

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

95ed45eminor correction to comment

comment:11 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

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.

comment:12 Changed 2 years ago by embray

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

comment:13 Changed 2 years ago by git

  • Commit changed from 95ed45ec061424d4708e724c425565e48faa479e to d364c6c1bcb9d21778917337e2f71cd0baeb7f3e

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

db87c9bThese packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
bf535f6Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
82b7be5Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
ec074d6similarly, this variable should be read out of the environment if possible
bbdb221Add a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
c17b46dSimplify directory removal
8907458use brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install
98a1f8dUse appropariate sage-dist-helpers in the pkg-config postinst script
8f500c6Better error handling with spkg-prerm exits with an error (before it would have just had an unhandled exception and dumped a traceback)
d364c6cminor correction to comment

comment:14 Changed 2 years ago by embray

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

comment:15 Changed 2 years ago by git

  • Commit changed from d364c6c1bcb9d21778917337e2f71cd0baeb7f3e to 1c8fca3598b4978b0e5a28a391d7f5b4e39fbead

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

2faf376These packages should have their versions bumped now that they require installing pre/post-uninstall scripts.
55c21deMove execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
ce6c825Read the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
8c2ae0csimilarly, this variable should be read out of the environment if possible
ac5f16dAdd a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
a31606dSimplify directory removal
886b7cbuse brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install
a2f189bUse appropariate sage-dist-helpers in the pkg-config postinst script
2c2152dBetter error handling with spkg-prerm exits with an error (before it would have just had an unhandled exception and dumped a traceback)
1c8fca3minor correction to comment

comment:16 Changed 2 years ago by embray

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

comment:17 Changed 2 years ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:18 Changed 23 months ago by saraedum

  • Status changed from needs_review to needs_work

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

comment:19 Changed 23 months ago by embray

Looks like just a merge conflict, yet again.

comment:20 Changed 23 months ago by git

  • Commit changed from 1c8fca3598b4978b0e5a28a391d7f5b4e39fbead to 7a81e70eaebb7ec27f48614ab5232d7fccff4b74

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

45e39e5Add the ability for packages to have a pre-uninstall script too.
c167505Move execution of spkg- scripts to a simple utility function, avoiding a bit of boilerplate
df2af7dRead the path to SAGE_SPKG_INST from the environment if possible; otherwise use the hard-coded default path
d720715similarly, this variable should be read out of the environment if possible
4721f40Add a 'verbose' flag, and changed some informative output messages to stdout instead of stderr
700f764Simplify directory removal
f2ef70euse brial as an example of splitting an spkg-legacy-uninstall script out from an spkg-install
e8d06a5Use appropariate sage-dist-helpers in the pkg-config postinst script
10def6cBetter error handling with spkg-prerm exits with an error (before it would have just had an unhandled exception and dumped a traceback)
7a81e70minor correction to comment

comment:21 Changed 23 months ago by embray

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

comment:22 Changed 23 months ago by embray

  • Status changed from needs_work to needs_review

comment:23 Changed 23 months ago by saraedum

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.

comment:24 Changed 23 months ago by embray

  • Status changed from needs_review to positive_review

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

comment:25 Changed 23 months ago by vbraun

  • Branch changed from u/embray/build/spkg-uninstall/script to 7a81e70eaebb7ec27f48614ab5232d7fccff4b74
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.