Remove support for installing old-style SPKGs, deprecated in Sage 6.9

[mkoeppe]

Old-style SPKGs were deprecated in #19158, merged in Sage 6.9.

There is only sparse evidence of non-broken old-style SPKGs:

• cunningham_tables - https://groups.google.com/d/msg/sage-support/T4IO-AmOsCM/HYy_N6UdAQAJ - added as new-style package in convert cunningham_tables to a new style package #29834

• polytopes_db_4d - added as new-style package in polytopes_db_4d converted to new style package #26029

We remove support for old-style packages, but we keep the command sage -p, which can also be used for installing a new-style package without dependencies.

Depends on #26029
Depends on #29834

CC: @dimpase @embray @vbraun @jhpalmieri @orlitzky

Component: build

Author: Matthias Koeppe, John Palmieri

Branch/Commit: c0fb6d4

Reviewer: Dima Pasechnik, John Palmieri, Matthias Koeppe

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

[mkoeppe]

Branch: u/mkoeppe/remove_support_for_installing_old_style_spkgs__deprecated_in_sage_6_9

[mkoeppe]

Commit: 6820fa7

[mkoeppe]

Author: Matthias Koeppe

[mkoeppe]

New commits:

6820fa7

build/bin/sage-spkg, src/bin/sage: Remove support for installing old-style SPKGs

[dimpase]

comment:8

there is also an old style polytopes_db_4d, a huge download, 8Gb, see e.g. https://doc.sagemath.org/html/en/reference/discrete_geometry/sage/geometry/polyhedron/palp_database.html

let me see if I can have a go converting it.

[dimpase]

Dependencies: #26029

[dimpase]

comment:9

see #26029 for the new-style polytopes_db_4d

I'll do the same with cunningham_tables

[dimpase]

Changed dependencies from #26029 to #26029, #29834

[dimpase]

comment:10

#29834 - converted cunningham_tables

[dimpase]

comment:11

lgtm - modulo the deps (which are not merged in the branch).

[dimpase]

Reviewer: Dima Pasechnik

[mkoeppe]

comment:12

Thanks!

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

e0b6112

build/bin/sage-spkg, src/bin/sage: Remove support for installing old-style SPKGs

[sagetrac-git]

Changed commit from 6820fa7 to e0b6112

[jhpalmieri]

comment:17

Jeroen was a fan of sage -p, but I'm not sure why. Does it serve any purpose now? It was used (as in tests/cmdline.py) for non-old-style packages. Is it indeed redundant?

[mkoeppe]

comment:18

I don't think they serve any purpose at this point, other than to confuse users. All known packages seem to have been converted (Dima did two remaining ones in #26029, #29834), and it's easy to convert a package should another one be spotted in the wild.

I would say that the whole model of encouraging users to distribute their code in a packaging format specific to Sage (in fact, specific to Sage-the-distribution!) is outdated and far from best practices. Users have found better ways to package code - see https://wiki.sagemath.org/SageMathExternalPackages .

[mkoeppe]

comment:19

Oh, rereading your comment, are you referring to the use of sage -p to install new-style packages? I was actually not aware that that works, but it seems to do exactly the same as sage -i.

[mkoeppe]

comment:20

OK, I see -p was documented as -p [opts] [packages]-- install the given Sage packages, without dependency checking ...

[mkoeppe]

comment:21

I'll restore this option. Let's figure out if it should be deprecated.

[jhpalmieri]

comment:22

Sounds good. I'm completely fine with getting rid of the old-style packaging machinery, but I also think it's a good idea to keep sage -p for now, for its other use.

[sagetrac-git]

Changed commit from e0b6112 to f4f1121

[sagetrac-git]

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

f4f1121

build/bin/sage-spkg, src/bin/sage: Remove support for installing old-style SPKGs

[mkoeppe]

Changed reviewer from Dima Pasechnik to Dima Pasechnik, John Palmieri

[jhpalmieri]

comment:26

It looks like more can be removed. I haven't tested these changes thoroughly; what do you think?

diff --git a/build/bin/sage-spkg b/build/bin/sage-spkg
index c8d800fcae..620925a319 100755
--- a/build/bin/sage-spkg
+++ b/build/bin/sage-spkg
@@ -255,10 +255,8 @@ if echo "$PKG_SRC" | grep / >/dev/null; then
     echo >&2 "Error: Installing old-style SPKGs is no longer supported"
     exit 1
 fi
-# PKG_NAME is the last path component without .spkg
-# This already reduces case 2b to case 2a.
-PKG_NAME=`basename "$PKG_SRC" | sed 's/\.spkg$//'`
-PKG_BASE=`echo "$PKG_NAME" | sed 's/-.*//'`
+PKG_NAME="$PKG_SRC"
+PKG_BASE=`echo "$PKG_NAME" | sed 's/-.*//'` # strip version number
 
 # USE_LOCAL_SCRIPTS is a flag that if non-empty will cause
 # this script to try to install the package using local metadata
@@ -266,39 +264,31 @@ PKG_BASE=`echo "$PKG_NAME" | sed 's/-.*//'`
 # the value of this flag is set in the next codeblock
 USE_LOCAL_SCRIPTS=
 
-if [ -f "$PKG_SRC" ]; then
-    # PKG_SRC is a file.  If it is given by a relative path, prepend `pwd`
-    # (reduce case 1b to 1a)
-    if ! echo "$PKG_SRC" | grep '^/' >/dev/null; then
-        PKG_SRC="`pwd`/$PKG_SRC"
-    fi
-elif [ -z "$PKG_HAS_PATH" ]; then
-    # If PKG_SRC is not an existing file and doesn't contain a slash,
-    # we are in case 2a or 3.  If version in 2a matches the version in
-    # build/pkgs or we are in case 3 use the local scripts, otherwise
-    # we try to find a package in upstream
-    PKG_VER="${PKG_NAME#${PKG_BASE}}"
-    PKG_VER="${PKG_VER#-}"
-    PKG_SCRIPTS="$SAGE_ROOT/build/pkgs/$PKG_BASE"
-    LOCAL_PKG_VER=`cat $PKG_SCRIPTS/package-version.txt 2>/dev/null`
-    if [ -n "$LOCAL_PKG_VER" ] && [ -z "$PKG_VER" -o "$PKG_VER" = "$LOCAL_PKG_VER" ]; then
-        PKG_VER="$LOCAL_PKG_VER"
-        if [ -z "$PKG_VER" ]; then
-            PKG_NAME="${PKG_BASE}"
-        else
-            PKG_NAME="${PKG_BASE}-${PKG_VER}"
-        fi
-        USE_LOCAL_SCRIPTS=yes
-        PKG_BASE_VER=`echo $PKG_VER | sed 's/\.p[0-9][0-9]*$//'`
-        PKG_NAME_UPSTREAM=`lookup_param tarball "$PKG_SCRIPTS/checksums.ini" | sed "s/VERSION/$PKG_BASE_VER/"`
-        echo "Found local metadata for $PKG_NAME"
-
-        # Warning for experimental packages
-        if [ x`cat "$PKG_SCRIPTS/type"` = x"experimental" -a $INFO = 0 ]; then
-            if [ $YES != 1 ]; then
-                # We use /dev/tty here because our output may be redirected
-                # to a logfile, or line-buffered.
-                write_to_tty <<EOF
+# PKG_SRC should look like "package-VERSION" or just "package".
+# If VERSION matches the version in build/pkgs or there is no version
+# specified, use the local scripts; otherwise we try to find a package
+# in upstream.
+PKG_VER="${PKG_NAME#${PKG_BASE}}"
+PKG_VER="${PKG_VER#-}"
+PKG_SCRIPTS="$SAGE_ROOT/build/pkgs/$PKG_BASE"
+LOCAL_PKG_VER=`cat $PKG_SCRIPTS/package-version.txt 2>/dev/null`
+PKG_VER="$LOCAL_PKG_VER"
+if [ -z "$PKG_VER" ]; then
+    PKG_NAME="${PKG_BASE}"
+else
+    PKG_NAME="${PKG_BASE}-${PKG_VER}"
+fi
+USE_LOCAL_SCRIPTS=yes
+PKG_BASE_VER=`echo $PKG_VER | sed 's/\.p[0-9][0-9]*$//'`
+PKG_NAME_UPSTREAM=`lookup_param tarball "$PKG_SCRIPTS/checksums.ini" | sed "s/VERSION/$PKG_BASE_VER/"`
+echo "Found local metadata for $PKG_NAME"
+
+# Warning for experimental packages
+if [ x`cat "$PKG_SCRIPTS/type"` = x"experimental" -a $INFO = 0 ]; then
+    if [ $YES != 1 ]; then
+        # We use /dev/tty here because our output may be redirected
+        # to a logfile, or line-buffered.
+        write_to_tty <<EOF
 =========================== WARNING ===========================
 You are about to download and install the experimental package
 $PKG_NAME.  This probably won't work at all for you! There
@@ -306,36 +296,22 @@ is no guarantee that it will build correctly, or behave as
 expected. Use at your own risk!
 ===============================================================
 EOF
-                if [ $? -ne 0 ]; then
-                    echo "Terminal not available for prompting.  Use 'sage -i -y $PKG_BASE'"
-                    echo "to install experimental packages in non-interactive mode."
-                    YES=-1
-                fi
-                if [ $YES != -1 ]; then
-                    read -p "Are you sure you want to continue [Y/n]? " answer < /dev/tty > /dev/tty 2>&1
-                else
-                    answer=n
-                fi
-                case "$answer" in
-                    n*|N*) exit 1;;
-                esac
-                # Confirm the user's input.  (This gives important
-                # feedback to the user when output is redirected to a logfile.)
-                echo > /dev/tty "OK, installing $PKG_NAME now..."
-            fi
+        if [ $? -ne 0 ]; then
+            echo "Terminal not available for prompting.  Use 'sage -i -y $PKG_BASE'"
+            echo "to install experimental packages in non-interactive mode."
+            YES=-1
         fi
-
-    else
-        cd "$SAGE_DISTFILES"
-        for spkg in `ls -1t ${PKG_NAME}.spkg ${PKG_NAME}-*.spkg 2>/dev/null`; do
-            if [ -f "$spkg" ]; then
-                # Found a good package
-                echo "Found package $PKG_NAME in $SAGE_DISTFILES/$spkg"
-                PKG_SRC="`pwd`/$spkg"
-                PKG_NAME=`basename "$spkg" | sed 's/\.spkg$//'`
-                break
-            fi
-        done
+        if [ $YES != -1 ]; then
+            read -p "Are you sure you want to continue [Y/n]? " answer < /dev/tty > /dev/tty 2>&1
+        else
+            answer=n
+        fi
+        case "$answer" in
+            n*|N*) exit 1;;
+        esac
+        # Confirm the user's input.  (This gives important
+        # feedback to the user when output is redirected to a logfile.)
+        echo > /dev/tty "OK, installing $PKG_NAME now..."
     fi
 fi
 

[mkoeppe]

comment:27

I agree, please go ahead and push it to the branch.

[jhpalmieri]

Changed branch from u/mkoeppe/remove_support_for_installing_old_style_spkgs__deprecated_in_sage_6_9 to u/jhpalmieri/remove_support_for_installing_old_style_spkgs__deprecated_in_sage_6_9

[mkoeppe]

comment:29

This seems to work well.

---

New commits:

c0fb6d4

trac 29289: remove more code for old-style packages

[mkoeppe]

Changed reviewer from Dima Pasechnik, John Palmieri to Dima Pasechnik, John Palmieri, Matthias Koeppe

[mkoeppe]

Changed commit from f4f1121 to c0fb6d4

[mkoeppe]

Changed author from Matthias Koeppe to Matthias Koeppe, John Palmieri

[mkoeppe]

comment:31

Ready for review

[jhpalmieri]

comment:32

This works for me, too.

[jhpalmieri]

comment:33

Let's move ahead with it.

[mkoeppe]

comment:34

Thanks!

[vbraun]

Changed branch from u/jhpalmieri/remove_support_for_installing_old_style_spkgs__deprecated_in_sage_6_9 to c0fb6d4