Opened 10 years ago

Closed 10 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:

Status badges

Description (last modified by leif)

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)

makefile (785 bytes) - added by vbraun 10 years ago.
for review purposes only
spkg-install (765 bytes) - added by vbraun 10 years ago.
for review purposes
symmetrica-2.0.p6-p7.diff (5.5 KB) - added by leif 10 years ago.
Diff between Volker's p6 and my p7. For reference / review only.
symmetrica-2.0.p5-p7.diff (17.7 KB) - added by leif 10 years ago.
Cumulative diff between the previous p5 in Sage and the p7. For reference / review only.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 10 years ago by vbraun

  • Status changed from new to needs_review

Works on Fedora 14 and t2.math. Is rather simple change...

See also: https://groups.google.com/d/topic/sage-devel/2jyWCWUn_30/discussion

Changed 10 years ago by vbraun

for review purposes only

comment:2 Changed 10 years ago by vbraun

  • 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: Changed 10 years ago by 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.

comment:4 in reply to: ↑ 3 Changed 10 years ago by ddrake

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 10 years ago by ddrake

  • Status changed from needs_review to 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 10 years ago by vbraun

  • Status changed from needs_work to 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.

Changed 10 years ago by vbraun

for review purposes

comment:7 Changed 10 years ago by leif

  • 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 10 years ago by leif

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

A couple of more or less minor issues:

  • Michael Abshoff should be removed from the spkg maintainer list.
  • s/sort tp/sort to/ in SPKG.txt
  • $CFLAG64 should be quoted in [ -z $CFLAG64 ]. (And exporting CFLAGS 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 (and cp) aren't checked. (makefile could also be patched rather than copied over. SPKG.txt mentions patches/makefile.patch, though there's only patches/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 the test 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 10 years ago by leif

  • Keywords Ubuntu 11.10 Oneiric Ocelot added

comment:10 Changed 10 years ago by leif

  • Priority changed from major to critical

comment:11 Changed 10 years ago by leif

  • Priority changed from critical to blocker
  • Summary changed from Symmetrica does not build on OpenSUSE 11.2 x86_64 to 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 10 years ago by vbraun

  • Status changed from needs_work to 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 10 years ago by vbraun

  • Keywords sd34 added

comment:14 Changed 10 years ago by leif

Instead of adding ~* to .hgignore, you should delete spkg-check~. ;-)

And make spkg-check executable.

comment:15 follow-up: Changed 10 years ago by vbraun

Whats the obsession with deleting backup files, geez :-P

The new spkg is at the same URL.

comment:16 in reply to: ↑ 15 Changed 10 years ago by leif

  • Status changed from needs_review to 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 10 years ago by leif

Oh, should of course read for patch in ../patches/*.patch; do ....

comment:18 follow-up: Changed 10 years ago by vbraun

Well you are welcome to rewrite the spkg-install and spkg-check scripts. I'll be happy to review them.

comment:19 in reply to: ↑ 18 Changed 10 years ago by leif

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 10 years ago by leif

  • Authors changed from Volker Braun to Volker Braun, Leif Leonhardy
  • Description modified (diff)
  • Status changed from needs_work to needs_review

New spkg is up.

If Volker is ok with my additional changes, we have a positive review.

Changed 10 years ago by leif

Diff between Volker's p6 and my p7. For reference / review only.

comment:21 Changed 10 years ago by leif

I've attached a diff with my changes (for review).

Changed 10 years ago by leif

Cumulative diff between the previous p5 in Sage and the p7. For reference / review only.

comment:22 Changed 10 years ago by vbraun

  • Reviewers changed from Leif Leonhardy to Leif Leonhardy, Volker Braun
  • Status changed from needs_review to positive_review

Looks good. Even builds on Solaris/Sparc?. Positive review.

comment:23 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.