Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#18344 closed defect (fixed)

fix some case issues in sage-fix-pkg-checksums

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-6.7
Component: build Keywords:
Cc: Merged in:
Authors: John Palmieri Reviewers: Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: 02ba704 (Commits) Commit:
Dependencies: Stopgaps:

Description

The Sage developer's guide insists that the directories in build/pkgs should be all lowercase. At the same time, we have started allowing tarballs in upstream which have mixed case (e.g., Pillow and Sphinx). The script sage-fix-pkg-checksums should allow for this, converting the tarball name to lowercase before looking for the corresponding directory.

Change History (16)

comment:1 Changed 4 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/case

comment:2 Changed 4 years ago by jhpalmieri

  • Commit set to 02ba7045cf38725ca8cb5456df649b6bacdcbd8e
  • Status changed from new to needs_review

Since we want to support OS X, we cannot allow different files whose names are the same except for case differences. So I hope this change is safe.


New commits:

02ba704#18344: case issues in sage-fix-pkg-checksums

comment:3 Changed 4 years ago by leif

While your patch apparently fixes the issue with uppercase letters in upstream tarballs (haven't tested it yet though), I don't like the concept of the script, which is pretty upside-down: Instead of iterating over build/pkgs/* and taking the name of the upstream tarball from there, it does the opposite.

I'd rather have

sage-pkg-checksums [--verbose] [--create|--check|--update] [<package name>]*

with useful information about missing upstream tarballs, checksum mismatches etc., but that's presumably beyond this ticket.

(And we have a wild mixture of tr, sed, shopt -s nocaseglob, and shell parameter expansion in various shell scripts... The Python script sage-pkg also seems to be obsolete or at least not yet suited for "new style" spkgs.)

comment:4 Changed 4 years ago by leif

  • Reviewers set to Leif Leonhardy
  • Status changed from needs_review to positive_review

comment:5 follow-up: Changed 4 years ago by leif

P.S.: How am I supposed to call that script btw.? It relies on SAGE_ROOT being set, and doesn't even check whether that's the case.

./sage -fix-pkg-checksums doesn't work (and isn't documented), so one has to enter a Sage subshell or, as I did, for example run

SAGE_ROOT=. src/bin/sage-fix-pkg-checksums

./sage --sh -c sage-fix-pkg-checksums isn't less cryptic or tedious either.

comment:6 Changed 4 years ago by leif

... we could at least add

if [ -z "$SAGE_ROOT" ]; then
    if [ -d upstream ]; then # probably add further sanity checks
        SAGE_ROOT=`pwd`
    else
        echo >&2 "Error: SAGE_ROOT not set."
        exit 1
    fi
fi

Otherwise the script does nothing and exits without an error, even when we're in the Sage root directory but SAGE_ROOT isn't set (just because [ -d /build/pkgs/... ] usually yields false).

Last edited 4 years ago by leif (previous) (diff)

comment:7 in reply to: ↑ 5 Changed 4 years ago by jhpalmieri

Replying to leif:

P.S.: How am I supposed to call that script btw.? It relies on SAGE_ROOT being set, and doesn't even check whether that's the case.

The developer's guide says to do

$ sage -sh sage-fix-pkg-checksums

Anyway, feel free to open another ticket with general cleanup of the script.

comment:8 Changed 4 years ago by vbraun

  • Branch changed from u/jhpalmieri/case to 02ba7045cf38725ca8cb5456df649b6bacdcbd8e
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:9 follow-up: Changed 4 years ago by jdemeyer

  • Commit 02ba7045cf38725ca8cb5456df649b6bacdcbd8e deleted

What's the point of this

 ** Note: case mismatch between upstream/Pillow-2.2.2.tar.gz and build/pkgs/pillow. **

comment:10 in reply to: ↑ 9 Changed 4 years ago by leif

Replying to jdemeyer:

What's the point of this

 ** Note: case mismatch between upstream/Pillow-2.2.2.tar.gz and build/pkgs/pillow. **

It's a note.

(On partially case-insensitive filesystems, using uppercase letters in tarball names might be unintentional, I think. It's not an error though, since we can deal with such. But we IMHO shouldn't randomly change case upon upgrades.)

comment:11 follow-up: Changed 4 years ago by jhpalmieri

One point behind the note is to somehow warn people that because of OS X, we can't have packages whose names depend on case. So at least this way they see that "Pillow.tar.gz" is associated with the directory "pillow". Maybe if someone tries to create a package "GAP", in addition to the "gap" package, rather than have things perhaps mysteriously break during building, a message like this will pop up earlier letting them know something is going on.

comment:12 Changed 4 years ago by jdemeyer

I don't really like notes which don't seem to have any meaning...

Is it a warning? Is there a problem? Should I avoid that?

Do you agree to remove that note in a follow-up ticket?

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:13 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by jdemeyer

Replying to jhpalmieri:

Maybe if someone tries to create a package "GAP", in addition to the "gap" package, rather than have things perhaps mysteriously break during building, a message like this will pop up earlier letting them know something is going on.

Let me explain this one more time: sage-fix-pkg-checksums has nothing to do with building Sage. It's a development tool and only that.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by jhpalmieri

Replying to jdemeyer:

Replying to jhpalmieri:

Maybe if someone tries to create a package "GAP", in addition to the "gap" package, rather than have things perhaps mysteriously break during building, a message like this will pop up earlier letting them know something is going on.

Let me explain this one more time: sage-fix-pkg-checksums has nothing to do with building Sage. It's a development tool and only that.

Do I need to explain that, presumably, someone developing a new package for Sage will try to build Sage with that package? That was the context for my comment: I want to provide some feedback when they are still developing the package (and therefore running sage-fix-pkg-checksums), before they even get to the building/testing stage. And there may be developers who never use OS X and are unaware of how it treats case.

comment:15 Changed 4 years ago by jhpalmieri

In any case, I don't care too much about the note, so if you want to remove it, go ahead.

comment:16 in reply to: ↑ 14 Changed 4 years ago by jdemeyer

Replying to jhpalmieri:

I want to provide some feedback when they are still developing the package (and therefore running sage-fix-pkg-checksums), before they even get to the building/testing stage. And there may be developers who never use OS X and are unaware of how it treats case.

OK, got it. But the current note is completely meaningless: I still don't understand what it really is trying to tell me...

Follow-up at #18402

Note: See TracTickets for help on using tickets.