#29289 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: build Keywords:
Cc: dimpase, embray, vbraun, jhpalmieri, mjo Merged in:
Authors: Matthias Koeppe, John Palmieri Reviewers: Dima Pasechnik, John Palmieri, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: c0fb6d4 (Commits, GitHub, GitLab) Commit: c0fb6d47362a1d52ede61e57ef5b7777045ce238
Dependencies: #26029, #29834 Stopgaps:

Status badges

Description (last modified by mkoeppe)

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

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

  • polytopes_db_4d - added as new-style package in #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.

Change History (35)

comment:1 Changed 14 months ago by mkoeppe

  • Cc jhpalmieri added

comment:2 Changed 14 months ago by mkoeppe

  • Description modified (diff)

comment:3 Changed 14 months ago by mkoeppe

  • Summary changed from Remove support for old-style SPKGs, deprecated in Sage 6.9 to Remove support for installing old-style SPKGs, deprecated in Sage 6.9

comment:4 Changed 13 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

comment:5 Changed 11 months ago by mkoeppe

  • Branch set to u/mkoeppe/remove_support_for_installing_old_style_spkgs__deprecated_in_sage_6_9

comment:6 Changed 11 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 6820fa7fb9eb9a9468b44b5b962f4562ef835cc0
  • Status changed from new to needs_review

New commits:

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

comment:7 Changed 11 months ago by mkoeppe

  • Cc mjo added

comment:8 Changed 11 months ago by dimpase

  • Status changed from needs_review to needs_work

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.

comment:9 Changed 11 months ago by dimpase

  • Dependencies set to #26029

see #26029 for the new-style polytopes_db_4d

I'll do the same with cunningham_tables

comment:10 Changed 11 months ago by dimpase

  • Dependencies changed from #26029 to #26029, #29834
  • Status changed from needs_work to needs_review

#29834 - converted cunningham_tables

comment:11 Changed 11 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

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

comment:12 Changed 11 months ago by mkoeppe

Thanks!

comment:13 Changed 11 months ago by git

  • Commit changed from 6820fa7fb9eb9a9468b44b5b962f4562ef835cc0 to e0b6112766e17ca2a4c72698d866afea6a3693db
  • Status changed from positive_review to needs_review

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

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

comment:14 Changed 11 months ago by mkoeppe

  • Status changed from needs_review to positive_review

Rebased on 9.2.beta1

comment:15 Changed 11 months ago by jhpalmieri

  • Status changed from positive_review to needs_work

Fails doctests. From tests/cmdline.py:

        sage: out, err, ret = test_executable(["sage", "-p", "--info", "--info", "sqlite"])  # optional - build
        sage: print(out)  # optional - build
        Found local metadata for sqlite-...

comment:16 Changed 11 months ago by mkoeppe

Thanks for catching this!

comment:17 Changed 11 months ago by jhpalmieri

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?

comment:18 Changed 11 months ago by mkoeppe

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 .

comment:19 Changed 11 months ago by mkoeppe

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.

comment:20 Changed 11 months ago by mkoeppe

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

comment:21 Changed 11 months ago by mkoeppe

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

comment:22 Changed 11 months ago by jhpalmieri

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.

comment:23 Changed 11 months ago by git

  • Commit changed from e0b6112766e17ca2a4c72698d866afea6a3693db to f4f112115f423f7414757b81803854d86ae19589

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

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

comment:24 Changed 11 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:25 Changed 11 months ago by mkoeppe

  • Reviewers changed from Dima Pasechnik to Dima Pasechnik, John Palmieri

comment:26 Changed 11 months ago by jhpalmieri

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

  • build/bin/sage-spkg

    diff --git a/build/bin/sage-spkg b/build/bin/sage-spkg
    index c8d800fcae..620925a319 100755
    a b if echo "$PKG_SRC" | grep / >/dev/null; then 
    255255    echo >&2 "Error: Installing old-style SPKGs is no longer supported"
    256256    exit 1
    257257fi
    258 # PKG_NAME is the last path component without .spkg
    259 # This already reduces case 2b to case 2a.
    260 PKG_NAME=`basename "$PKG_SRC" | sed 's/\.spkg$//'`
    261 PKG_BASE=`echo "$PKG_NAME" | sed 's/-.*//'`
     258PKG_NAME="$PKG_SRC"
     259PKG_BASE=`echo "$PKG_NAME" | sed 's/-.*//'` # strip version number
    262260
    263261# USE_LOCAL_SCRIPTS is a flag that if non-empty will cause
    264262# this script to try to install the package using local metadata
    PKG_BASE=`echo "$PKG_NAME" | sed 's/-.*//'` 
    266264# the value of this flag is set in the next codeblock
    267265USE_LOCAL_SCRIPTS=
    268266
    269 if [ -f "$PKG_SRC" ]; then
    270     # PKG_SRC is a file.  If it is given by a relative path, prepend `pwd`
    271     # (reduce case 1b to 1a)
    272     if ! echo "$PKG_SRC" | grep '^/' >/dev/null; then
    273         PKG_SRC="`pwd`/$PKG_SRC"
    274     fi
    275 elif [ -z "$PKG_HAS_PATH" ]; then
    276     # If PKG_SRC is not an existing file and doesn't contain a slash,
    277     # we are in case 2a or 3.  If version in 2a matches the version in
    278     # build/pkgs or we are in case 3 use the local scripts, otherwise
    279     # we try to find a package in upstream
    280     PKG_VER="${PKG_NAME#${PKG_BASE}}"
    281     PKG_VER="${PKG_VER#-}"
    282     PKG_SCRIPTS="$SAGE_ROOT/build/pkgs/$PKG_BASE"
    283     LOCAL_PKG_VER=`cat $PKG_SCRIPTS/package-version.txt 2>/dev/null`
    284     if [ -n "$LOCAL_PKG_VER" ] && [ -z "$PKG_VER" -o "$PKG_VER" = "$LOCAL_PKG_VER" ]; then
    285         PKG_VER="$LOCAL_PKG_VER"
    286         if [ -z "$PKG_VER" ]; then
    287             PKG_NAME="${PKG_BASE}"
    288         else
    289             PKG_NAME="${PKG_BASE}-${PKG_VER}"
    290         fi
    291         USE_LOCAL_SCRIPTS=yes
    292         PKG_BASE_VER=`echo $PKG_VER | sed 's/\.p[0-9][0-9]*$//'`
    293         PKG_NAME_UPSTREAM=`lookup_param tarball "$PKG_SCRIPTS/checksums.ini" | sed "s/VERSION/$PKG_BASE_VER/"`
    294         echo "Found local metadata for $PKG_NAME"
    295 
    296         # Warning for experimental packages
    297         if [ x`cat "$PKG_SCRIPTS/type"` = x"experimental" -a $INFO = 0 ]; then
    298             if [ $YES != 1 ]; then
    299                 # We use /dev/tty here because our output may be redirected
    300                 # to a logfile, or line-buffered.
    301                 write_to_tty <<EOF
     267# PKG_SRC should look like "package-VERSION" or just "package".
     268# If VERSION matches the version in build/pkgs or there is no version
     269# specified, use the local scripts; otherwise we try to find a package
     270# in upstream.
     271PKG_VER="${PKG_NAME#${PKG_BASE}}"
     272PKG_VER="${PKG_VER#-}"
     273PKG_SCRIPTS="$SAGE_ROOT/build/pkgs/$PKG_BASE"
     274LOCAL_PKG_VER=`cat $PKG_SCRIPTS/package-version.txt 2>/dev/null`
     275PKG_VER="$LOCAL_PKG_VER"
     276if [ -z "$PKG_VER" ]; then
     277    PKG_NAME="${PKG_BASE}"
     278else
     279    PKG_NAME="${PKG_BASE}-${PKG_VER}"
     280fi
     281USE_LOCAL_SCRIPTS=yes
     282PKG_BASE_VER=`echo $PKG_VER | sed 's/\.p[0-9][0-9]*$//'`
     283PKG_NAME_UPSTREAM=`lookup_param tarball "$PKG_SCRIPTS/checksums.ini" | sed "s/VERSION/$PKG_BASE_VER/"`
     284echo "Found local metadata for $PKG_NAME"
     285
     286# Warning for experimental packages
     287if [ x`cat "$PKG_SCRIPTS/type"` = x"experimental" -a $INFO = 0 ]; then
     288    if [ $YES != 1 ]; then
     289        # We use /dev/tty here because our output may be redirected
     290        # to a logfile, or line-buffered.
     291        write_to_tty <<EOF
    302292=========================== WARNING ===========================
    303293You are about to download and install the experimental package
    304294$PKG_NAME.  This probably won't work at all for you! There
    is no guarantee that it will build correctly, or behave as 
    306296expected. Use at your own risk!
    307297===============================================================
    308298EOF
    309                 if [ $? -ne 0 ]; then
    310                     echo "Terminal not available for prompting.  Use 'sage -i -y $PKG_BASE'"
    311                     echo "to install experimental packages in non-interactive mode."
    312                     YES=-1
    313                 fi
    314                 if [ $YES != -1 ]; then
    315                     read -p "Are you sure you want to continue [Y/n]? " answer < /dev/tty > /dev/tty 2>&1
    316                 else
    317                     answer=n
    318                 fi
    319                 case "$answer" in
    320                     n*|N*) exit 1;;
    321                 esac
    322                 # Confirm the user's input.  (This gives important
    323                 # feedback to the user when output is redirected to a logfile.)
    324                 echo > /dev/tty "OK, installing $PKG_NAME now..."
    325             fi
     299        if [ $? -ne 0 ]; then
     300            echo "Terminal not available for prompting.  Use 'sage -i -y $PKG_BASE'"
     301            echo "to install experimental packages in non-interactive mode."
     302            YES=-1
    326303        fi
    327 
    328     else
    329         cd "$SAGE_DISTFILES"
    330         for spkg in `ls -1t ${PKG_NAME}.spkg ${PKG_NAME}-*.spkg 2>/dev/null`; do
    331             if [ -f "$spkg" ]; then
    332                 # Found a good package
    333                 echo "Found package $PKG_NAME in $SAGE_DISTFILES/$spkg"
    334                 PKG_SRC="`pwd`/$spkg"
    335                 PKG_NAME=`basename "$spkg" | sed 's/\.spkg$//'`
    336                 break
    337             fi
    338         done
     304        if [ $YES != -1 ]; then
     305            read -p "Are you sure you want to continue [Y/n]? " answer < /dev/tty > /dev/tty 2>&1
     306        else
     307            answer=n
     308        fi
     309        case "$answer" in
     310            n*|N*) exit 1;;
     311        esac
     312        # Confirm the user's input.  (This gives important
     313        # feedback to the user when output is redirected to a logfile.)
     314        echo > /dev/tty "OK, installing $PKG_NAME now..."
    339315    fi
    340316fi
    341317

comment:27 Changed 11 months ago by mkoeppe

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

comment:28 Changed 11 months ago by jhpalmieri

  • Branch changed 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

comment:29 Changed 11 months ago by mkoeppe

  • Authors changed from Matthias Koeppe to Matthias Koeppe, John Palmieri
  • Commit changed from f4f112115f423f7414757b81803854d86ae19589 to c0fb6d47362a1d52ede61e57ef5b7777045ce238
  • Reviewers changed from Dima Pasechnik, John Palmieri to Dima Pasechnik, John Palmieri, Matthias Koeppe

This seems to work well.


New commits:

c0fb6d4trac 29289: remove more code for old-style packages

comment:30 Changed 11 months ago by mkoeppe

  • Description modified (diff)

comment:31 Changed 11 months ago by mkoeppe

Ready for review

comment:32 Changed 11 months ago by jhpalmieri

This works for me, too.

comment:33 Changed 11 months ago by jhpalmieri

  • Status changed from needs_review to positive_review

Let's move ahead with it.

comment:34 Changed 11 months ago by mkoeppe

Thanks!

comment:35 Changed 11 months ago by vbraun

  • Branch changed from u/jhpalmieri/remove_support_for_installing_old_style_spkgs__deprecated_in_sage_6_9 to c0fb6d47362a1d52ede61e57ef5b7777045ce238
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.