Opened 12 years ago
Closed 11 years ago
#10719 closed defect (fixed)
Symmetrica does not build on OpenSUSE 11.2 x86_64 and Ubuntu 11.10
Reported by: | vbraun | Owned by: | GeorgSWeber |
---|---|---|---|
Priority: | blocker | Milestone: | sage-4.7.2 |
Component: | build | Keywords: | library order linker semantics Ubuntu 11.10 Oneiric Ocelot sd34 |
Cc: | mhansen | Merged in: | sage-4.7.2.alpha4 |
Authors: | Volker Braun, Leif Leonhardy | Reviewers: | Leif Leonhardy, Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
The updated spkg moves the -lm
compiler flag past -lsymmetrica
when building the test program, adds an spkg-check
file, honors LDFLAGS
, and removes the obsolete Debian dist/
directory.
New spkg: http://sage.math.washington.edu/home/leif/Sage/spkgs/symmetrica-2.0.p7.spkg
md5sum: 03a4d2ec75eb3595ebba568419872e64
Attachments (4)
Change History (27)
comment:1 Changed 12 years ago by
Status: | new → needs_review |
---|
comment:2 Changed 12 years ago by
Cc: | mhansen added |
---|
Ok I noticed that patches/*patch
wasn't actually applied?!?. Fixed now. Also, I rewrote the makefile and we are actually only building the static library now. Updated spkg is at the same location as above.
comment:3 follow-up: 4 Changed 12 years ago by
The above spkg successfully builds on the current Ubuntu 11.04 beta, which fixes a linking problem from the p5 spkg. I'll see if I can properly review this, but the spkg does fix an important problem.
comment:4 Changed 12 years ago by
Replying to ddrake:
The above spkg successfully builds on the current Ubuntu 11.04 beta, which fixes a linking problem from the p5 spkg. I'll see if I can properly review this, but the spkg does fix an important problem.
sage-devel thread: https://groups.google.com/d/topic/sage-devel/chWfpkqj6xU/discussion
comment:5 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
Here's one tiny problem: most of the files in src/ all have 400 permissions, and spkg-install copies two .h files over at the end of the build. If you rebuild symmetrica, that copying will fail because it can't overwrite the "400" files. I would run a chmod 644 *
in the src/ directory of the spkg.
comment:6 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
I've upgraded the spgk. I set the permissions in src/ to 644 and added an extra chmod to the spkg-install so we don't trip over this issue again if somebody unpacks the source tarball.
comment:7 Changed 11 years ago by
Keywords: | library order linker semantics added |
---|
This patch / new spkg will also be necessary for newer Debians like e.g. Ubuntu 11.10, which feature (the good old) stricter linker semantics; cf. ticket:11674:38 ff.
Our current Singular (3-1-1-4) spkg also has issues with wrong library order, namely -ldl
and -lkernel
. (I haven't yet checked whether this is already fixed in a newer upstream release; hopefully Sage's Singular will be upgraded during the Sage/Singular Days (SD34) at the end of September, see also this sage-devel thread; cf. #10903.)
comment:8 Changed 11 years ago by
Reviewers: | → Leif Leonhardy |
---|---|
Status: | needs_review → needs_work |
A couple of more or less minor issues:
- Michael Abshoff should be removed from the spkg maintainer list.
- http://www.symmetrica.de/ is perhaps a more stable upstream link (currently redirected to http://www.algorithm.uni-bayreuth.de/en/research/SYMMETRICA/).
s/sort tp/sort to/
inSPKG.txt
$CFLAG64
should be quoted in[ -z $CFLAG64 ]
. (And exportingCFLAGS
there is really redundant.)
- Not all user-specified
CFLAGS
should be overridden by Sage's (e.g.-O2
should precede$CFLAGS
in the assignment).
- The exit codes of
patch
(andcp
) aren't checked. (makefile
could also be patched rather than copied over.SPKG.txt
mentionspatches/makefile.patch
, though there's onlypatches/makefile
.)
- (GNU)
patch
should be added as a dependency.
$MAKE -f makefile
is redundant.
$SAGE_LOCAL
should be quoted.
cp
for installation (library, headers) should use-p
, otherwise the current umask could break permissions.
- The new
makefile
removes thetest
target, which builds an executable functioning as a minimalistic test suite. (The removal also removes-lm
completely, such that the changelog entry regarding that no longer applies.)
The "usual" superfluous trailing semicolon inside the sanity check is of courseTM also there; [ -z "$SAGE_LOCAL" ]
is also better than comparing to an empty string.
comment:9 Changed 11 years ago by
Keywords: | Ubuntu 11.10 Oneiric Ocelot added |
---|
comment:10 Changed 11 years ago by
Priority: | major → critical |
---|
comment:11 Changed 11 years ago by
Priority: | critical → blocker |
---|---|
Summary: | Symmetrica does not build on OpenSUSE 11.2 x86_64 → Symmetrica does not build on OpenSUSE 11.2 x86_64 and Ubuntu 11.10 |
Making this a blocker, since IMHO this should be fixed in the final Sage 4.7.2, likely to be released [right] before the final Ubuntu 11.10 (Oneiric Ocelot), in contrast to a final Sage 4.7.3 ...
(Hereby also pinging myself...)
P.S.: The description isn't up-to-date, as the current spkg doesn't use -lm
at all, since it doesn't build the test program (which it IMHO should, as a minimalistic test suite, unless it provides and runs some other test(s), at least if SAGE_CHECK=yes
).
comment:12 Changed 11 years ago by
Status: | needs_work → needs_review |
---|
I implemented leif's nitpicks. Also, now has a spkg-check. Updated spkg is at the same url as before.
comment:13 Changed 11 years ago by
Keywords: | sd34 added |
---|
comment:14 Changed 11 years ago by
Instead of adding ~*
to .hgignore
, you should delete spkg-check~
. ;-)
And make spkg-check
executable.
comment:15 follow-up: 16 Changed 11 years ago by
Whats the obsession with deleting backup files, geez :-P
The new spkg is at the same URL.
comment:16 Changed 11 years ago by
Status: | needs_review → needs_work |
---|
Replying to vbraun:
The new spkg is at the same URL.
Sorry, you were too quick (or I was too slow...):
The commit message looks like a copy-paste accident, but the spkg was made on 28st[sic] September anyway. :)
spkg-check
should also use $MAKE
.
If you also build test
in spkg-check
, you have to setup CFLAGS
etc. there in the same way as in spkg-install
.
s/exitting/exiting/
.
The error message should start with "Error
" (or "Failed
").
(The whole test could be simplified by
if ! diff spkg-check.actual spkg-check.expected; then echo >&2 "Error: The symmetrica test suite failed (see diff output above)." exit 1 # Optionally: #else # echo "All tests passed." # or whatever fi # EOF
Do we need LDFLAGS
when linking test
?
I.e., for example -m64
, which we could take from CFLAG64
, but using LDFLAG64
(defaulting to -m64
) would be cleaner.
I don't like the set +/-o errexit
, as it doesn't necessarily give a meaningful (and easy to search for) error message. Therefore we removed all instances of set +/-e
previously.
Instead, you could use the usual loop for applying the patches, exiting with an appropriate error message upon the first patch which doesn't apply:
for patch in ../patches; do patch < "$patch" if [ $? -ne 0 ]; then echo >&2 "Error: Patch \"$patch\" failed to apply. Exiting... exit 1 fi done # Copy over makefile and check $? # or create also a *patch* for 'makefile' to be applied # in the loop above, too.
Similar for the "installation":
spkg-install
will only exit with a non-zero status if the last copy command failed. You could either check each exit code, giving an appropriate error message for each individual case, or use
foo && bar && baz if [ $? -ne 0 ]; then echo >&2 "Error: Failed to install Symmetrica." exit 1 fi
comment:17 Changed 11 years ago by
Oh, should of course read for patch in ../patches/*.patch; do ...
.
comment:18 follow-up: 19 Changed 11 years ago by
Well you are welcome to rewrite the spkg-install and spkg-check scripts. I'll be happy to review them.
comment:19 Changed 11 years ago by
Replying to vbraun:
Well you are welcome to rewrite the spkg-install and spkg-check scripts. I'll be happy to review them.
Ok, later today.
comment:20 Changed 11 years ago by
Authors: | Volker Braun → Volker Braun, Leif Leonhardy |
---|---|
Description: | modified (diff) |
Status: | needs_work → needs_review |
New spkg is up.
If Volker is ok with my additional changes, we have a positive review.
Changed 11 years ago by
Attachment: | symmetrica-2.0.p6-p7.diff added |
---|
Diff between Volker's p6 and my p7. For reference / review only.
Changed 11 years ago by
Attachment: | symmetrica-2.0.p5-p7.diff added |
---|
Cumulative diff between the previous p5 in Sage and the p7. For reference / review only.
comment:22 Changed 11 years ago by
Reviewers: | Leif Leonhardy → Leif Leonhardy, Volker Braun |
---|---|
Status: | needs_review → positive_review |
Looks good. Even builds on Solaris/Sparc?. Positive review.
comment:23 Changed 11 years ago by
Merged in: | → sage-4.7.2.alpha4 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Works on Fedora 14 and t2.math. Is rather simple change...
See also: https://groups.google.com/d/topic/sage-devel/2jyWCWUn_30/discussion