Opened 7 years ago

Closed 7 years ago

#16415 closed defect (fixed)

Ignore case in package directory

Reported by: vbraun Owned by:
Priority: major Milestone: sage-6.3
Component: build Keywords:
Cc: Merged in:
Authors: Volker Braun Reviewers: John Palmieri, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: 9b31585 (Commits, GitHub, GitLab) Commit: 9b315855498609b254807fc74fc3bcfcca57af6b
Dependencies: Stopgaps:

Status badges

Description (last modified by vbraun)

The python tarball contains a upper-case Python-x.y.z directory. Our build system does not handle that case. Worse, it works differently on OSX and real OS'es because of filesystem case sensitivity.

Change History (18)

comment:1 Changed 7 years ago by vbraun

  • Authors set to Volker Braun
  • Component changed from PLEASE CHANGE to build
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 7 years ago by vbraun

  • Branch set to u/vbraun/ignore_case_in_package_directory

comment:3 Changed 7 years ago by vbraun

  • Commit set to e9aa3c608f1407fc5ff36236b442e7db42f8f871
  • Status changed from new to needs_review

New commits:

e9aa3c6ignore case when renaming source directory

comment:4 Changed 7 years ago by leif

Is this supposed to be a temporary (=ad-hoc) solution?

In the long run, the actual folder name should probably be part of Sage's "package metadata".

comment:5 follow-up: Changed 7 years ago by vbraun

If there is a single top-level directory then we don't need to store the name (and manually maintain) it elsewhere. There should be some checking, of course. And if you don't have a single top-level directory then I'd consider that to be an upstream bug.

comment:6 in reply to: ↑ 5 Changed 7 years ago by leif

Replying to vbraun:

If there is a single top-level directory then we don't need to store the name

Yep, but that's NYI.

(and manually maintain) it elsewhere.

Well, we "manually" maintain the "checksums" for each specific upstream tarball anyway (in a central repo, tied to specific Sage versions, which is IMHO a bad thing to do anyway).

There should be some checking, of course.

Sure.

And if you don't have a single top-level directory then I'd consider that to be an upstream bug.

:-)

comment:7 Changed 7 years ago by leif

P.S.: FWIW, requiring foo-x.y.pN.spkg to have a foo-x.y.pN/ folder was used as a consistency check (although I didn't like it, as it just made testing spkgs slightly harder).

comment:8 Changed 7 years ago by git

  • Commit changed from e9aa3c608f1407fc5ff36236b442e7db42f8f871 to 9b315855498609b254807fc74fc3bcfcca57af6b

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

9b31585bash 4.1 workaround

comment:9 Changed 7 years ago by vbraun

Also removed the hiding-of-errors, I want to know which packages don't work...

comment:10 Changed 7 years ago by leif

Probably immediately go with find . -type d -a -not -name . -exec mv {} src \; 2>/dev/null ;-) (and afterwards test -d ...)

(Haven't checked right now, but I guess -iname isn't portable.)

comment:11 follow-up: Changed 7 years ago by vbraun

Besides not portable, its IMHO not readable. The whole contraption should be rewritten in Python with some proper doctesting.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by leif

Replying to vbraun:

Besides not portable, its IMHO not readable.

Well, the find version above is portable, although I'd just use it to get the folder name. (Or some flavour of (cd * && basename `pwd`).)

The whole contraption should be rewritten in Python with some proper doctesting.

In Lisp, please.


Anyway, I'd really explicitly check whether src/ afterwards exists (and is a directory); we already have

    echo "Finished extraction"

    cd "$PKG_NAME"
    if [ $? -ne 0 ]; then
        echo >&2 "Error: after extracting, the directory $PKG_NAME does not exist"
        exit 1
    fi

right below for legacy spkgs.


Just out of curiosity: Which bash version(s) did the original change work with?

I tried older (3.2.*) as well as newer (4.3.11*) bashs; with none of them it worked.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by vbraun

$ bash --version GNU bash, version 4.2.47(1)-release (x86_64-redhat-linux-gnu)

Replying to leif:

In Lisp, please.

Sage is a Python project. And Python is actually pretty good for sysadmin-type scripts. Unlike lisp. And unlike Bash it can be tested in a sane way.

comment:14 in reply to: ↑ 13 Changed 7 years ago by leif

Replying to vbraun:

$ bash --version
GNU bash, version 4.2.47(1)-release (x86_64-redhat-linux-gnu)

FWIW, "${PKG_NAME_UPSTREAM%.tar*}" doesn't work with

GNU bash, version 4.2.28(1)-release (x86_64-redhat-linux-gnu)
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

either. (So it's probably a bug in your bash that it works for you. ;-) )

comment:15 Changed 7 years ago by leif

I'd still add a check (with an appropriate error message) that src/ afterwards really exists (and is a directory ;-) ); cf. my comment above.

comment:16 Changed 7 years ago by vbraun

We don't need a double check, either building the package will break if the rename fails or the rename is not necessary (and, really, wtf do we rename the tarball directory to start with, afaik nobody else does that). In particular, there is no need to check that the renamed directory is still a directory.

comment:17 Changed 7 years ago by jhpalmieri

  • Reviewers set to John Palmieri, Leif Leonhardy
  • Status changed from needs_review to positive_review

Looks okay to me.

comment:18 Changed 7 years ago by vbraun

  • Branch changed from u/vbraun/ignore_case_in_package_directory to 9b315855498609b254807fc74fc3bcfcca57af6b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.