Opened 2 years ago

Closed 2 years ago

Last modified 18 months ago

#28208 closed enhancement (fixed)

spkg-configure.m4 for symmetrica

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.9
Component: build: configure Keywords: spkg-configure
Cc: embray, fbissey, isuruf Merged in:
Authors: Dima Pasechnik Reviewers: François Bissey, Isuru Fernando
Report Upstream: N/A Work issues:
Branch: 47f09f1 (Commits, GitHub, GitLab) Commit:
Dependencies: #27827 Stopgaps:

Status badges

Description (last modified by dimpase)

Debian, Fedora and Conda package it, so we can use it at least on some boxes.

The present branch runs a test known to crash an unpatched Symmetrica version, so this allows to ignore an old Symmetrica version installed on old Fedora versions (e.g. on Fedora 26).

configure tarball: http://users.ox.ac.uk/~coml0531/sage/configure-4416fd47f080f6f849eef2ba969b69a66d651df7.tar.gz

Change History (33)

comment:1 Changed 2 years ago by dimpase

  • Cc fbissey added

I wonder whether the Symmetrica Cython interface should be converted to using dynamic library. It's silly to link in a static one, creating a 30Mb local/lib/python2.7/site-packages/sage/libs/symmetrica/symmetrica.so. For comparison, the 2nd biggest .so among local/lib/python2.7/site-packages/sage/libs/*/*.so is just 3Mb.

comment:2 Changed 2 years ago by arojas

Given that the symmetrica spkg is using a custom makefile, it's just a matter of modifying it to build a shared library instead of a static one.

comment:3 follow-up: Changed 2 years ago by dimpase

well, on OSX and Cygwin it might actually be a problem to get right. I guess it's a primary reason for having it static, it's easier. Buidling a shared library without autotoolization is a pain...

comment:4 Changed 2 years ago by dimpase

symmetrica package also needs a custom uninstall, as local/lib/libsymmetrica.a does not get cleaned for some reason. (Or perhaps it's a bug in the build system).

---

EDIT: I had a pre-sdh_install installation, on which uninstall failed (it does work on more recent installs). Perhaps we still want a custom uninstall, but at least it's something that will go away eventually by itself.

Last edited 2 years ago by dimpase (previous) (diff)

comment:5 Changed 2 years ago by dimpase

  • Cc isuruf added

At present, it tests for presense of symmetrica.h header, which apparently what Debian has added (we can change it to symmetrica/defs.h`, which is one of the two original headers). I wonder what Conda would prefer.

comment:6 follow-up: Changed 2 years ago by isuruf

conda package has symmetrica/defs.h and symmetrica/macro.h and no symmetrica.h

comment:7 in reply to: ↑ 6 Changed 2 years ago by dimpase

Replying to isuruf:

conda package has symmetrica/defs.h and symmetrica/macro.h and no symmetrica.h

Thanks. Is there a place I can look up the sources used (without an actual conda install)? I am trying to understand what patches are applied there by conda.

comment:9 in reply to: ↑ 3 Changed 2 years ago by fbissey

Replying to dimpase:

well, on OSX and Cygwin it might actually be a problem to get right. I guess it's a primary reason for having it static, it's easier. Buidling a shared library without autotoolization is a pain...

This is antic from way back in probably 2008 and 2009 when I was working with T. Abbott. I have a makefile for linux and a makefile for OS X. I only install a shared library. I should revisit it and do something to merge the makefiles.

I have the same headers than Isuru on conda.

And I'll certainly want to steal a cmake solution from conda if it is available :) . But cmake is still not an official sage package.

comment:10 follow-up: Changed 2 years ago by isuruf

And I'll certainly want to steal a cmake solution from conda if it is available :)

It is. I just made symmetrica a shared library on conda for linux, osx and windows.

It's unfortunate that sage doesn't have cmake as a standard package.

comment:11 Changed 2 years ago by dimpase

Debian has autotoolised the package, if I am not mistaken: https://salsa.debian.org/science-team/symmetrica/blob/master/debian/patches/upstream-autotoolization.patch

Anyhow, it's easy to make cmake standard; in practice this would hopefully mean that people will use system's cmake, as we have the relevant build/pkgs/cmake/spkg-configure.m4 already in place.

comment:12 Changed 2 years ago by git

  • Commit changed from ae19df6119caf4feb0d9b34b8952bab271092712 to 8253abb7c7942de2c331bc1fa5fbf9fe15c2be4b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8253abbspkg-configure.m4 for symmetrica

comment:13 Changed 2 years ago by dimpase

  • Description modified (diff)
  • Status changed from new to needs_review

comment:14 in reply to: ↑ 10 Changed 2 years ago by fbissey

Replying to isuruf:

And I'll certainly want to steal a cmake solution from conda if it is available :)

It is. I just made symmetrica a shared library on conda for linux, osx and windows.

It's unfortunate that sage doesn't have cmake as a standard package.

I have one issue with your CMakeList.txt

    ARCHIVE DESTINATION lib
    LIBRARY DESTINATION lib

this should depend on the target. Sometimes it is lib64 or lib/<arch> (debian). I usually use https://cmake.org/cmake/help/v3.15/module/GNUInstallDirs.html in my own cmake scripts. My version is now here https://github.com/cschwan/sage-on-gentoo/blob/master/sci-libs/symmetrica/files/CMakeLists.txt

One thing interesting about the debian autotool patch is that it looks like they got the test working.

comment:15 Changed 2 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Looks rather trivial to me.

comment:16 follow-up: Changed 2 years ago by isuruf

Thanks @fbissey for the fixes. I added support for running tests with ctest as well.

comment:17 in reply to: ↑ 16 Changed 2 years ago by fbissey

Replying to isuruf:

Thanks @fbissey for the fixes. I added support for running tests with ctest as well.

I think I messed up a little bit by setting SOVERSION to 2.0 instead of just 2 (I was trying to get back to what I had before, but it ended up not being possible because I was not using the right scheme). We should check what debian ends up with, but I think we should just use "2".

With the test working I will align myself onto you. No need to have too many versions around.

comment:18 Changed 2 years ago by dimpase

  • Status changed from positive_review to needs_work

Unfortunately, Fedora comes with a buggy libsymmetrica-devel package. So it needs to be recognised and ruled out.

comment:19 Changed 2 years ago by dimpase

More precisely it appears that the Sage's fixes to symmetrica have been applied to Fedora 30 (perhaps 29 too?), see Changelog in

https://fedora.pkgs.org/30/fedora-x86_64/symmetrica-devel-2.0-22.fc30.x86_64.rpm.html

but not to older ones -- I tested on Fedora 26, which is a year past its EOL.

comment:20 Changed 2 years ago by dimpase

The following C code dumps core on old symmetrica, and works on the good one.

/* compile as follows: cc tt.c -lsymmetrica -lm */

#include "symmetrica/def.h"
#include "symmetrica/macro.h"
int main() 
{
  OP b,n;
  anfang();
  n = callocobject(); b = callocobject();
  sscan_integer("4",n);
  kostka_tafel(n, b);
  println(b);
  ende();
}

The output should be (from the good one, and Sage's doctest in src/sage/libs/symmetrica/kostka.pxi):

[1:0:0:0:0]
[1:1:0:0:0]
[1:1:1:0:0]
[1:2:1:1:0]
[1:3:2:3:1]

I'll make a test in the m4 file with it.

comment:21 Changed 2 years ago by git

  • Commit changed from 8253abb7c7942de2c331bc1fa5fbf9fe15c2be4b to 0aa1a7cbd34e1a5dd2faa69c21500facd4f46ce4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

231aa4fspkg-configure.m4 for symmetrica
0aa1a7cadded a test program (crashing on unpatched Symmetrica)

comment:22 Changed 2 years ago by dimpase

  • Description modified (diff)
  • Status changed from needs_work to needs_review

OK, so this does the trick of ruling old version out.

comment:23 Changed 2 years ago by dimpase

  • Description modified (diff)

comment:24 Changed 2 years ago by isuruf

  • Status changed from needs_review to positive_review

Checked debian and conda packages and they are picked up.

comment:25 Changed 2 years ago by isuruf

  • Reviewers changed from François Bissey to François Bissey, Isuru Fernando

comment:26 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:27 Changed 2 years ago by git

  • Commit changed from 0aa1a7cbd34e1a5dd2faa69c21500facd4f46ce4 to 4416fd47f080f6f849eef2ba969b69a66d651df7

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ed84fa2spkg-configure.m4 for pkgconf
511d022spkg-configure.m4 for symmetrica
4416fd4added a test program (crashing on unpatched Symmetrica)

comment:28 Changed 2 years ago by dimpase

  • Dependencies set to #27827
  • Status changed from needs_work to positive_review

rebased over the branch of #27827 - to avoid merge conflict.

comment:29 Changed 2 years ago by git

  • Commit changed from 4416fd47f080f6f849eef2ba969b69a66d651df7 to 47f09f1b48cbd07c6e99d202cab2e52db757d4e1
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

47f09f1bump configure version

comment:30 Changed 2 years ago by dimpase

  • Description modified (diff)

New commits:

47f09f1bump configure version

comment:31 Changed 2 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:32 Changed 2 years ago by vbraun

  • Branch changed from u/dimpase/packages/symmconfig to 47f09f1b48cbd07c6e99d202cab2e52db757d4e1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:33 Changed 18 months ago by mkoeppe

  • Commit 47f09f1b48cbd07c6e99d202cab2e52db757d4e1 deleted

Follow-up: #29405

Note: See TracTickets for help on using tickets.