Symmetrica does not build on OpenSUSE 11.2 x86_64 and Ubuntu 11.10

[vbraun]

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

[vbraun]

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

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

[vbraun]

for review purposes only

[vbraun]

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.

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

[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

[ddrake]

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.

[vbraun]

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.

[vbraun]

for review purposes

[leif]

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

[leif]

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/ 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 course^TM also there; [ -z "$SAGE_LOCAL" ] is also better than comparing to an empty string.

[leif]

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

[vbraun]

I implemented leif's nitpicks. Also, now has a spkg-check. Updated spkg is at the same url as before.

[leif]

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

And make spkg-check executable.

[vbraun]

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

The new spkg is at the same URL.

[leif]

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

[leif]

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

[vbraun]

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

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

[leif]

New spkg is up.

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

[leif]

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

[leif]

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

[leif]

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

[vbraun]

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