#30687 closed defect (fixed)

"make python3-clean" should always remove local/bin/python3*

Reported by: dimpase Owned by:
Priority: blocker Milestone: sage-9.2
Component: build Keywords:
Cc: mkoeppe, mjo, jhpalmieri Merged in:
Authors: Matthias Koeppe Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: ad7d2c9 (Commits, GitHub, GitLab) Commit: ad7d2c929bf0a7d083a0734ad66df4b2751bda63
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

make python3-clean does not remove $SAGE_LOCAL/bin/python3* - certainly not if an external python3 is used.

We do the uninstallation in an spkg-legacy-uninstall script.

(We also repair the spkg-legacy-uninstall mechanism - it was broken in #29096.)

Critical for Sage 9.2 because it will help avoid puzzling incremental build behavior, which could include corruption of the installed system python if that is writable for the user.

Change History (34)

comment:1 Changed 20 months ago by mkoeppe

Not really make python3-clean, more like make python3_venv-clean; see #29708 ("Fix switching between --with-system-python3 and --without-system-python3"). I had hoped to fix it via #29386, but this won't be ready for Sage 9.2.

We can try a hotfix for this issue by adding build/pkgs/python3/legacy-uninstall

comment:2 Changed 20 months ago by mkoeppe

  • Priority changed from major to critical

comment:3 Changed 20 months ago by mkoeppe

  • Branch set to u/mkoeppe/_make_python3_clean__should_always_remove_local_lib_python3_

comment:4 Changed 20 months ago by mkoeppe

  • Commit set to 0157c38bcd365310e822cfb63557d8e88bb04507

Here's an attempt. Untested


New commits:

0157c38build/pkgs/python3/spkg-legacy-uninstall.in: New

comment:5 Changed 20 months ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:6 Changed 20 months ago by mkoeppe

"Of course" the spkg-legacy-uninstall mechanism is broken... It is one of the WRAPPED_SCRIPTS of build/bin/sage-spkg, but sage_bootstrap.uninstall.legacy_uninstall tries to execute spkg-legacy-uninstall directly out of SAGE_ROOT/build with bash...

comment:7 Changed 20 months ago by git

  • Commit changed from 0157c38bcd365310e822cfb63557d8e88bb04507 to ad7d2c929bf0a7d083a0734ad66df4b2751bda63

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

e55c0c1build/pkgs/*/spkg-legacy-uninstall: Make executable, add #!, remove .in extension
0a2f15ebuild/bin/sage-spkg: Do not wrap the spkg-legacy-uninstall script
ad7d2c9build/sage_bootstrap/uninstall.py (legacy_uninstall): Execute spkg-legacy-uninstall script directly, do not go through bash

comment:8 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:9 Changed 20 months ago by mkoeppe

  • Status changed from new to needs_review

comment:10 Changed 20 months ago by mkoeppe

  • Cc jhpalmieri added

comment:11 follow-up: Changed 20 months ago by jhpalmieri

So the problem with #29096 is that the uninstall script wasn't being run by sage-spkg, so the conversion from SCRIPT.in to SCRIPT didn't do anything useful?

I also don't understand how the use of the spkg-legacy-uninstall script compares to its documentation. R and gap, for example, are modern packages. Why do they need legacy uninstall scripts?

Last edited 20 months ago by jhpalmieri (previous) (diff)

comment:12 in reply to: ↑ 11 Changed 20 months ago by mkoeppe

Replying to jhpalmieri:

So the problem with #29096 is that the uninstall script wasn't being run by sage-spkg, so the conversion from SCRIPT.in to SCRIPT didn't do anything useful?

Yes, prior to #29096 there was an inconsistency - the file build/pkgs/SPKG/spkg-legacy-uninstall was transformed to the wrapped script (in the temporary build location) but that was never used. Instead, sage-spkg-uninstall was directly invoked spkg-legacy-uninstall.

I unfortunately overlooked this in #29096 and so the renaming broke spkg-legacy-uninstall for all packages - this script was no longer getting invoked for any package.

I also don't understand how the use of the spkg-legacy-uninstall script compares to its usage. R and gap, for example, are modern packages. Why do they need legacy uninstall scripts?

I think it is for cleaning out very old installs in long-lived installation trees. Given that #29096 has not led to obvious error reports, it does not seem to be an important function.

comment:13 Changed 20 months ago by jhpalmieri

Okay, good. The ticket description says that we should remove $SAGE_LOCAL/lib/python3*, but the script removes $SAGE_LOCAL/bin/python3*. Which is correct?

comment:14 Changed 20 months ago by mkoeppe

  • Description modified (diff)
  • Summary changed from "make python3-clean" should always remove local/lib/python3* to "make python3-clean" should always remove local/bin/python3*

Not sure if the original report really meant "lib", and I don't think there is any problem with it. The branch, in any case, takes care of removing the problematic symlinks in "bin". I have update ticket summary and description.

comment:15 follow-up: Changed 20 months ago by dimpase

The original report did mean lib rather than bin. I guess both should be taken care of.

comment:16 follow-ups: Changed 20 months ago by jhpalmieri

If I've built Sage with a system Python 3, I'm not sure what make python3-clean should do. Should it really remove all Python site libraries? If it does this, it has to somehow let the build system know to rebuild all of those packages.

Last edited 20 months ago by jhpalmieri (previous) (diff)

comment:17 in reply to: ↑ 15 Changed 20 months ago by mkoeppe

Replying to dimpase:

The original report did mean lib rather than bin. I guess both should be taken care of.

If you install python3 from spkg, then make clean already works because it goes through DESTDIR staging.

If you use system python3 (venv), then nothing is installed in /usr/lib/python3.8... except for stuff installed into site-packages, which have their own uninstallation procedures.

comment:18 in reply to: ↑ 16 Changed 20 months ago by mkoeppe

Replying to jhpalmieri:

If I've built Sage with a system Python 3, I'm not sure what make python3-clean should do.

It should be a no-op because python3 is not installed.

comment:19 in reply to: ↑ 16 Changed 20 months ago by mkoeppe

Replying to jhpalmieri:

Should it really remove all Python site libraries?

No, the files that are installed there belong to the individual spkgs.

comment:20 Changed 20 months ago by jhpalmieri

Right, I understand that, so Dima, what did you have in mind?

comment:21 follow-ups: Changed 20 months ago by dimpase

while these files are installed by individual packages, they "belong" to python.

Also there should be no difference in what happens to the $SAGE_LOCAL/lib/python3* whether python3 is installed by Sage or not. Isn't it true that cleaning a Sage-installed Python leads to removal of $SAGE_LOCAL/lib/python3* ?

comment:22 in reply to: ↑ 21 Changed 20 months ago by mkoeppe

Replying to dimpase:

Isn't it true that cleaning a Sage-installed Python leads to removal of $SAGE_LOCAL/lib/python3* ?

No. The files that belong to the package, recorded in SAGE_LOCAL/var/lib/sage/installed are removed. It does not remove the installed site-packages.

When you reinstall the python3 spkg, then dependencies cause all python packages to be rebuilt.

comment:23 in reply to: ↑ 21 Changed 20 months ago by mkoeppe

Replying to dimpase:

while these files are installed by individual packages, they "belong" to python.

I think we need to start from the beginning here. What failure do you observe, with or without the branch on this ticket.

comment:24 Changed 20 months ago by dimpase

weird things happen if you switch from one external python3 to another.

comment:25 Changed 20 months ago by mkoeppe

I think the branch of this ticket addresses this by making make python3-clean remove the interpreter symlinks and the pyvenv.cfg. Please try

comment:26 Changed 20 months ago by mkoeppe

Let's please get this ticket in.

comment:27 Changed 20 months ago by jhpalmieri

I don't have any objections to it. Dima?

comment:28 follow-up: Changed 20 months ago by dimpase

A nitpick, but I don't like mass renaming of spkg-legacy-uninstall.in involving adding boilerplate into each of them. Couldn't build/sage_bootstrap/uninstall.py, the only place it is called from, just prepend the #! line?

comment:29 in reply to: ↑ 28 Changed 20 months ago by jhpalmieri

Replying to dimpase:

A nitpick, but I don't like mass renaming of spkg-legacy-uninstall.in involving adding boilerplate into each of them. Couldn't build/sage_bootstrap/uninstall.py, the only place it is called from, just prepend the #! line?

This is literally just undoing a change from #29096 which completely broke the use of the spkg-legacy-uninstall scripts. Otherwise I would agree with you.

comment:30 Changed 20 months ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

comment:31 Changed 20 months ago by mkoeppe

Thanks!

comment:32 Changed 20 months ago by mkoeppe

  • Description modified (diff)

comment:33 Changed 20 months ago by mkoeppe

  • Description modified (diff)
  • Priority changed from critical to blocker

comment:34 Changed 19 months ago by vbraun

  • Branch changed from u/mkoeppe/_make_python3_clean__should_always_remove_local_lib_python3_ to ad7d2c929bf0a7d083a0734ad66df4b2751bda63
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.