Opened 3 years ago

Closed 3 years ago

#27223 closed enhancement (fixed)

Fix legacy uninstall

Reported by: jdemeyer Owned by:
Priority: critical Milestone: sage-8.7
Component: build Keywords:
Cc: embray Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 3c97621 (Commits, GitHub, GitLab) Commit: 3c97621ba148c17490b1e326e7d33d38ebb234ae
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

It turns out that the legacy uninstallers spkg-legacy-uninstall are never actually run due to an always-false executable-bit check.

Also, it happens regularly that the stamp file $SAGE_LOCAL/var/lib/sage/installed/$PKG-$VERSION is inconsistent with what is actually installed. This can happen for example when package installation failed or was interrupted. This can lead to problems like #26996. As a solution, we suggest to always run the legacy uninstaller as fallback if there is no stamp file. This is more in line with how installation worked before the introduction of the new-style stamp files.

Change History (17)

comment:1 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)

comment:3 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/legacy_uninstaller_should_be_run_if_no_stamp_file_is_present

comment:4 Changed 3 years ago by jdemeyer

  • Commit set to 0cf4613f76fbdfe902f053b05b5a378e4b209299
  • Description modified (diff)
  • Summary changed from Legacy uninstaller should be run if no stamp file is present to Fix legacy uninstall

New commits:

0cf4613Fix legacy uninstall

comment:5 Changed 3 years ago by jdemeyer

  • Status changed from new to needs_review

comment:6 Changed 3 years ago by git

  • Commit changed from 0cf4613f76fbdfe902f053b05b5a378e4b209299 to bcef97ee2da641cff8f3d904db721bb8a8b9acef

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

bcef97erm -rf never fails; can't use sdh_die here anyway

comment:7 Changed 3 years ago by embray

This is what I found as well. I was going to post a fix to this last night but didn't have time.

comment:8 follow-up: Changed 3 years ago by embray

I'm not sure why you changed this message:

     if keep_files:
-        print("Removing stamp file '{0}' but keeping package files".format(
-            stamp_file))
+        print("Removing stamp file but keeping package files")
         remove_stamp_files(stamp_files)
         return

Isn't it a little bit more helpful to actually specify what file the message is referring to?

comment:9 Changed 3 years ago by embray

  • Reviewers set to Erik Bray

Other than that, this is just a slightly different formatted version of what I did, so you can set positive review after addressing my one question.

comment:10 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by jdemeyer

Replying to embray:

Isn't it a little bit more helpful to actually specify what file the message is referring to?

Mainly because it's not guaranteed that there is exactly one stamp file.

Also, why should the filename of the stamp file be printed in this code path but not in other code paths? If you really want to see a message for removing stamp files, it would better be in the remove_stamp_files() function.

comment:11 follow-up: Changed 3 years ago by embray

Also, it happens regularly that the stamp file $SAGE_LOCAL/var/lib/sage/installed/$PKG-$VERSION is inconsistent with what is actually installed. This can happen for example when package installation failed or was interrupted.

I do consider this a separate issue. I would like to eventually remove the spkg-legacy-uninstall files entirely. Most of them are not well maintained and their use at all is inconsistent (this is true before the idea of moving that functionality into a separate file).

In other words, even spkg-legacy-uninstall scripts are not a reliable way to deal with partial/failed installations. I would rather fix this more directly. For example, I think the stamp file could be written before copying files into $SAGE_LOCAL. That way, even if the "installation" (which at this point is just copying files) fails, there is at least still a manifest of what files were expected to be in place, so a partial installation can be rolled back.

There should also be better error/interrupt handling in the sage-spkg script so that "partial installs" can be immediately rolled back when possible.

The downside to this approach is if an install is interrupted before it's complete, you still have a valid stamp file suggesting the installation was completed. To address this, maybe it should be written first to a different location (after all, in this case it is serving merely as a file manifest) and then moved into its final location after the successful installation.

comment:12 in reply to: ↑ 10 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

Isn't it a little bit more helpful to actually specify what file the message is referring to?

Mainly because it's not guaranteed that there is exactly one stamp file.

That is rarely the case though. And in any case the message still says "Removing stamp file", singular.

Also, why should the filename of the stamp file be printed in this code path but not in other code paths? If you really want to see a message for removing stamp files, it would better be in the remove_stamp_files() function.

That's fine; I don't care where it goes.

comment:13 in reply to: ↑ 11 Changed 3 years ago by jdemeyer

Replying to embray:

For example, I think the stamp file could be written before copying files into $SAGE_LOCAL. That way, even if the "installation" (which at this point is just copying files) fails, there is at least still a manifest of what files were expected to be in place, so a partial installation can be rolled back.

I think it's too hard to ensure that the stamp file is consistent with the actual installed files. I've had similar issues with pip too where pip would think that a package is not installed but still some files belonging to that package are installed.

So rather than improving the stamp files, I would rather improve installation in the case that the stamp files are missing or wrong.

comment:14 Changed 3 years ago by embray

Under normal circumstances it's actually exactly consistent, except in the cases of files that are created post-installation which are handled differently. So I think you'd have to be more specific what you think the problem is.

I'm also not talking about "improving stamp files", but rather strategies for managing broken installations. The "stamp file" here is really serving two purposes (where previously it served only one):

  • It is an indication that a package installed successfully.
  • It is a manifest of (expected) installed files.

Installation, when done correctly, is just copying a tree of files from one place to another. In order to roll back a partial/unsuccessful installation you want to have that manifest somewhere, but not in the actual "stamp file" as its existence indicates a successful install. So what I'm suggesting is instead writing it to something like ".<pkg-name>-<version>.tmp" prior to installation, and then following a successful installing moving it to "<pkg-name>-<version>".

The uninstall program would also be able to look for these ".<pkg-name>-<version>.tmp" files in order to clean up broken installations.

comment:15 Changed 3 years ago by git

  • Commit changed from bcef97ee2da641cff8f3d904db721bb8a8b9acef to 3c97621ba148c17490b1e326e7d33d38ebb234ae

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

3c97621Always print message when removing stamp file

comment:16 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

Looks good to me. I'm sorry I didn't say anything sooner, though I will say your fix is a little cleaner than mine--you restructured things a bit such that there was less duplication than what I had.

comment:17 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/legacy_uninstaller_should_be_run_if_no_stamp_file_is_present to 3c97621ba148c17490b1e326e7d33d38ebb234ae
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.