Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore case in package directory #16415

Closed
vbraun opened this issue May 30, 2014 · 22 comments
Closed

Ignore case in package directory #16415

vbraun opened this issue May 30, 2014 · 22 comments

Comments

@vbraun
Copy link
Member

vbraun commented May 30, 2014

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.

Component: build

Author: Volker Braun

Branch/Commit: 9b31585

Reviewer: John Palmieri, Leif Leonhardy

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

@vbraun vbraun added this to the sage-6.3 milestone May 30, 2014
@vbraun

This comment has been minimized.

@vbraun
Copy link
Member Author

vbraun commented May 30, 2014

Author: Volker Braun

@vbraun
Copy link
Member Author

vbraun commented May 30, 2014

@vbraun
Copy link
Member Author

vbraun commented May 30, 2014

Commit: e9aa3c6

@vbraun
Copy link
Member Author

vbraun commented May 30, 2014

New commits:

e9aa3c6ignore case when renaming source directory

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 31, 2014

comment:4

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".

@vbraun
Copy link
Member Author

vbraun commented May 31, 2014

comment:5

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.

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 31, 2014

comment:6

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.

:-)

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 31, 2014

comment:7

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).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2014

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

9b31585bash 4.1 workaround

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 31, 2014

Changed commit from e9aa3c6 to 9b31585

@vbraun
Copy link
Member Author

vbraun commented May 31, 2014

comment:9

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 31, 2014

comment:10

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.)

@vbraun
Copy link
Member Author

vbraun commented May 31, 2014

comment:11

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented May 31, 2014

comment:12

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.

@vbraun
Copy link
Member Author

vbraun commented May 31, 2014

comment:13

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

Replying to @nexttime:

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.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jun 2, 2014

comment:14

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. ;-) )

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jun 3, 2014

comment:15

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

@vbraun
Copy link
Member Author

vbraun commented Jun 4, 2014

comment:16

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.

@jhpalmieri
Copy link
Member

comment:17

Looks okay to me.

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri, Leif Leonhardy

@vbraun
Copy link
Member Author

vbraun commented Jun 18, 2014

Changed branch from u/vbraun/ignore_case_in_package_directory to 9b31585

@vbraun vbraun closed this as completed in d150838 Jun 18, 2014
@vbraun vbraun mentioned this issue Jun 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants