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: |
Description (last modified by )
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
- Component changed from PLEASE CHANGE to build
- Description modified (diff)
- Type changed from PLEASE CHANGE to defect
comment:2 Changed 7 years ago by
- Branch set to u/vbraun/ignore_case_in_package_directory
comment:3 Changed 7 years ago by
- Commit set to e9aa3c608f1407fc5ff36236b442e7db42f8f871
- Status changed from new to needs_review
comment:4 Changed 7 years ago by
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: ↓ 6 Changed 7 years ago by
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
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
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
- Commit changed from e9aa3c608f1407fc5ff36236b442e7db42f8f871 to 9b315855498609b254807fc74fc3bcfcca57af6b
Branch pushed to git repo; I updated commit sha1. New commits:
9b31585 | bash 4.1 workaround
|
comment:9 Changed 7 years ago by
Also removed the hiding-of-errors, I want to know which packages don't work...
comment:10 Changed 7 years ago by
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: ↓ 12 Changed 7 years ago by
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: ↓ 13 Changed 7 years ago by
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: ↓ 14 Changed 7 years ago by
$ 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
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
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
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
- 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
- Branch changed from u/vbraun/ignore_case_in_package_directory to 9b315855498609b254807fc74fc3bcfcca57af6b
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
ignore case when renaming source directory