Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spkg-configure.m4 for symmetrica #28208

Closed
dimpase opened this issue Jul 16, 2019 · 41 comments
Closed

spkg-configure.m4 for symmetrica #28208

dimpase opened this issue Jul 16, 2019 · 41 comments

Comments

@dimpase
Copy link
Member

dimpase commented Jul 16, 2019

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

Depends on #27827

CC: @embray @kiwifb @isuruf

Component: build: configure

Author: Dima Pasechnik

Branch: 47f09f1

Reviewer: François Bissey, Isuru Fernando

Issue created by migration from https://trac.sagemath.org/ticket/28208

@dimpase dimpase added this to the sage-8.9 milestone Jul 16, 2019
@dimpase
Copy link
Member Author

dimpase commented Jul 17, 2019

comment:1

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.

@antonio-rojas
Copy link
Contributor

comment:2

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.

@dimpase
Copy link
Member Author

dimpase commented Jul 17, 2019

comment:3

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

@dimpase
Copy link
Member Author

dimpase commented Jul 17, 2019

comment:4

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.

@dimpase
Copy link
Member Author

dimpase commented Jul 17, 2019

comment:5

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.

@isuruf
Copy link
Member

isuruf commented Jul 17, 2019

comment:6

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

@dimpase
Copy link
Member Author

dimpase commented Jul 17, 2019

comment:7

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.

@isuruf
Copy link
Member

isuruf commented Jul 17, 2019

comment:8

Source used: https://github.com/conda-forge/symmetrica-feedstock/blob/master/recipe/meta.yaml#L10-L12

Patches used: https://github.com/conda-forge/symmetrica-feedstock/tree/master/recipe/patches

We use a CMake file to build which makes it easier to build windows packages and shared libs. (I've opened a PR to build shared libs at conda-forge/symmetrica-feedstock@3607965)

Built packages are at https://anaconda.org/conda-forge/symmetrica/files

@kiwifb
Copy link
Member

kiwifb commented Jul 17, 2019

comment:9

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.

@isuruf
Copy link
Member

isuruf commented Jul 17, 2019

comment:10

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.

@dimpase
Copy link
Member Author

dimpase commented Jul 17, 2019

comment:11

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2019

Changed commit from ae19df6 to 8253abb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2019

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

8253abbspkg-configure.m4 for symmetrica

@dimpase

This comment has been minimized.

@kiwifb
Copy link
Member

kiwifb commented Jul 18, 2019

comment:14

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/ (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.

@kiwifb
Copy link
Member

kiwifb commented Jul 18, 2019

Reviewer: François Bissey

@kiwifb
Copy link
Member

kiwifb commented Jul 18, 2019

comment:15

Looks rather trivial to me.

@isuruf
Copy link
Member

isuruf commented Jul 18, 2019

comment:16

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

@kiwifb
Copy link
Member

kiwifb commented Jul 18, 2019

comment:17

Replying to @isuruf:

Thanks @kiwifb 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.

@dimpase
Copy link
Member Author

dimpase commented Jul 18, 2019

comment:18

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

@dimpase

This comment has been minimized.

@isuruf
Copy link
Member

isuruf commented Jul 21, 2019

comment:24

Checked debian and conda packages and they are picked up.

@isuruf
Copy link
Member

isuruf commented Jul 21, 2019

Changed reviewer from François Bissey to François Bissey, Isuru Fernando

@vbraun
Copy link
Member

vbraun commented Jul 23, 2019

comment:26

Merge conflict

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2019

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)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2019

Changed commit from 0aa1a7c to 4416fd4

@dimpase
Copy link
Member Author

dimpase commented Jul 29, 2019

Dependencies: #27827

@dimpase
Copy link
Member Author

dimpase commented Jul 29, 2019

comment:28

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2019

Changed commit from 4416fd4 to 47f09f1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2019

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

47f09f1bump configure version

@dimpase
Copy link
Member Author

dimpase commented Jul 29, 2019

New commits:

47f09f1bump configure version

@dimpase

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Jul 30, 2019

Changed branch from u/dimpase/packages/symmconfig to 47f09f1

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2020

Changed commit from 47f09f1 to none

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 29, 2020

comment:33

Follow-up: #29405

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants