Add libhomfly as optional package #18047

miguelmarco · 2015-03-24T19:17:40Z

I modified the homfly[1] program to be run as a library[2]. It computes the homfly polynomial of knots and links.
I would like to add it as standard package so we can use it in #17030 i understand that it has to become optonal before moving to standard, but i would like to speed up the process. Of course, i volunteer to maintain it.
[1] http://burtleburtle.net/bob/knot/homfly.html
[2] https://github.com/miguelmarco/libhomfly

Tarball: attachment: libhomfly-1.0.tar.gz

CC: @vbraun @kcrisman @amitjamadagni @sagetrac-fugelde @tscrim

Component: packages: optional

Keywords: days74

Author: Miguel Marco

Branch: cc13df1

Reviewer: Jeroen Demeyer, Travis Scrimshaw

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

The text was updated successfully, but these errors were encountered:

miguelmarco · 2015-03-24T19:32:26Z

Branch: u/mmarco/libhomfly

kcrisman · 2015-03-24T19:33:19Z

Commit: 458d6bf

kcrisman · 2015-03-24T19:33:19Z

New commits:

`458d6bf`	`Initial commit`

vbraun · 2015-03-24T19:35:39Z

comment:5

Nice. Pretty sure that doesn't work on OSX. Basically there is no way to build a shared libray without libtool in a portable manner. Also, I would be very opposed to make a library standard that doesn't do the standard autotools + libtool process.

jdemeyer · 2015-03-25T22:30:13Z

Replying to @miguelmarco:

I would like to add it as standard package so we can use it in #17030

You can use it in #17030 even if it's an optional package. Just make sure you provide a good error message in case the package is needed but not installed.

sagetrac-git · 2015-03-27T10:52:10Z

Changed commit from 458d6bf to 14112a4

sagetrac-git · 2015-03-27T10:52:10Z

Branch pushed to git repo; I updated commit sha1. New commits:

`14112a4`	`Moved to autotools-libtools`

miguelmarco · 2015-03-27T10:55:02Z

comment:8

Attachment: libhomfly-1.0.tar.2.gz

I moved the package to libtools. Also uploaded the new tarball (please ignore libhomflt-1.0.tar.2.gz)

vbraun · 2015-03-27T11:08:15Z

comment:9

Looks good. Newer autoconf gives one warning, can you add AM_PROG_AR to fix it?

diff --git a/configure.ac b/configure.ac
index 6c5f369..103e585 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,8 +2,9 @@ AC_INIT([libhomfly], [1.0], [mmarco@unizar.es])
 AC_CONFIG_AUX_DIR([build-aux])
 AC_CONFIG_MACRO_DIR([m4])
 AM_INIT_AUTOMAKE([foreign -Wall])
-LT_INIT
 AC_PROG_CC
+AM_PROG_AR
+LT_INIT
 AC_CONFIG_FILES([Makefile lib/Makefile])
 AC_OUTPUT

Also, I take it you have a testcase (a minimal C program to link to your library to see if it works?) That would be nice to add, can save you quite a bit of debugging headache later.

sagetrac-git · 2015-03-27T23:54:03Z

Changed commit from 14112a4 to 2edefa1

sagetrac-git · 2015-03-27T23:54:03Z

Branch pushed to git repo; I updated commit sha1. New commits:

`2edefa1`	`Added test`

miguelmarco · 2015-03-27T23:54:53Z

comment:11

Made the change and added a little test

sagetrac-git · 2015-03-28T20:53:36Z

Changed commit from 2edefa1 to f19093f

sagetrac-git · 2015-03-28T20:53:36Z

Branch pushed to git repo; I updated commit sha1. New commits:

`f19093f`	`Fixed checksums`

vbraun · 2015-03-28T21:40:12Z

comment:13

The library uses Bohm GC, but the test program does not. Hence you shoudl link the library to libgc, and not the test program. In other words, programmers using your library shouldn't have to know what your private library dependencies are:

diff --git a/lib/Makefile.am b/lib/Makefile.am
index 5aa6416..b4341c4 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -1,3 +1,4 @@
 lib_LTLIBRARIES = libhomfly.la
 libhomfly_la_SOURCES = bound.c  control.c  dllink.c  knot.c  model.c  order.c  poly.c standard.h order.h bound.h  control.h  dllink.h knot.h model.h order.h poly.h standard.h
+libhomfly_la_LDFLAGS = -lgc
 include_HEADERS = homfly.h
diff --git a/test/Makefile.am b/test/Makefile.am
index 0394f0a..b564b7e 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -2,5 +2,4 @@ bin_PROGRAMS = test_example
 test_example_SOURCES = test_example.c homfly.h
 test_example_LDADD = @top_builddir@/lib/libhomfly.la
 test_example_DEPENDENCIES = @top_builddir@/lib/libhomfly.la
-test_example_LDFLAGS = -lgc
 TESTS = test_example

Apart from that it looks good.

There are some global variables and all symbols are exported, so there is a reasonable chance that you'll run into global state or symbol name colissions when you link it into Sage. It would probably be better to hide all symbols that you are not going to use. Possibly later when you have a better idea of what you actually want to call from Sage.

miguelmarco · 2015-03-30T11:23:08Z

comment:14

The cython interface is being done in #18057

miguelmarco · 2015-03-30T11:39:42Z

comment:15

Uploaded new tarball

miguelmarco · 2016-05-30T14:21:14Z

Changed keywords from none to days74

miguelmarco · 2016-05-30T14:31:59Z

comment:18

The new tarball should expose only the homfly symbol, which is the one used by the cython interface.

sagetrac-git · 2016-05-30T14:48:20Z

Changed commit from f19093f to 090c7fd

sagetrac-git · 2016-05-30T14:48:20Z

Branch pushed to git repo; I updated commit sha1. New commits:

`be71e3e`	`Initial commit`
`5b6fae2`	`Moved to autotools-libtools`
`7bcf75a`	`Added test`
`d14ec25`	`Fixed checksums`
`371e368`	`Initial commit`
`249c987`	`Moved to autotools-libtools`
`8eedce7`	`Added test`
`ad156b3`	`Fixed checksums`
`b550750`	`Rebased, updated checksums and added type`
`090c7fd`	`Merge branch 'u/mmarco/libhomfly' of git://trac.sagemath.org/sage into t/18047/libhomfly`

tscrim · 2016-06-01T10:23:03Z

comment:47

You have forgotten the test/data.txt file in the tarball, which is needed to run the tests.

tscrim · 2016-06-01T10:24:59Z

comment:48

Replying to @tscrim:

You have forgotten the test/data.txt file in the tarball, which is needed to run the tests.

Actually, this is my fault for not having the autotools including it.

miguelmarco · 2016-06-01T10:27:21Z

comment:49

Ok, I will handle it. Give me a few minutes.

tscrim · 2016-06-01T10:28:58Z

comment:50

I've already done it, see PR #4 on github.

sagetrac-git · 2016-06-01T10:32:22Z

comment:51

Attachment: libhomfly-1.0.tar.gz

Branch pushed to git repo; I updated commit sha1. New commits:

`cc13df1`	`New checksums`

sagetrac-git · 2016-06-01T10:32:22Z

Changed commit from c37cb34 to cc13df1

miguelmarco · 2016-06-01T10:32:59Z

comment:52

It should be included now.

New commits:

`cc13df1`	`New checksums`

tscrim · 2016-06-01T10:37:39Z

comment:53

Thank you. sage -i -c libhomfly now passes. I feel that we should have a release tag on github and use the corresponding tarball for that here. I think it is as easy as adding a tag to the git repo, but I've not used that functionality yet.

miguelmarco · 2016-06-01T10:41:19Z

comment:54

Does github automate the "autoreconf-configure-make dist" cycle?

tscrim · 2016-06-01T10:45:31Z

comment:55

IDK. From what I understand, the release tab on github looks for a (annotated?) tag of the form v1.0. I will look into it today.

tscrim · 2016-06-01T10:47:03Z

comment:56

See perhaps: https://help.github.com/articles/creating-releases/ but IDK about autocreation of tarballs.

miguelmarco · 2016-06-01T10:52:41Z

comment:57

I created one tag and release, but it just gives a tarball with the content of the repo. So the actual tarball for distribution should be created by the full autotools cycle from there

tscrim · 2016-06-01T10:53:15Z

comment:58

Okay, so the created tarball will be just of the source code on github (but it will do that automatically). So we could either just do the hosting ourselves or have a spkg-src. Well for now, this works (as this might be the only release) and to add the tag.

Thank you for your work on this.

vbraun · 2016-06-02T04:13:51Z

Changed branch from u/mmarco/libhomfly to cc13df1

kcrisman · 2016-06-16T15:42:18Z

comment:60

When I do L.homfly_polynomial() I just get ImportError: No module named homfly - should that give me a message to do the optional package instead? (This is on 7.3.beta3 so it's "in" otherwise.)

kcrisman · 2016-06-16T15:42:18Z

Changed commit from cc13df1 to none

miguelmarco · 2016-06-16T15:51:41Z

comment:61

The functionality requires to have the optional package installed. Since these tickets are already closed, could you please open a new ticket for improving the error message?

kcrisman · 2016-06-16T17:41:43Z

comment:62

That's exactly what I figured, but wanted to confirm. See #20838.

jdemeyer · 2016-06-23T11:47:02Z

comment:63

@vbraun: the tarball on the mirrors in not the one from this ticket (attachment: libhomfly-1.0.tar.gz). Can you please fix this?

miguelmarco added this to the sage-6.6 milestone Mar 24, 2015

miguelmarco added the p: major / 3 label Mar 24, 2015

This comment has been minimized.

Sign in to view

miguelmarco added c: packages: optional and removed p: major / 3 labels Mar 24, 2015

miguelmarco changed the title ~~help~~ Add libhomfly as optional package Mar 24, 2015

kcrisman added the t: enhancement label Mar 24, 2015

miguelmarco added the s: needs review label May 30, 2016

sagetrac-git mannequin added the s: needs review label Jun 1, 2016

tscrim added s: needs work and removed s: needs review labels Jun 1, 2016

tscrim added s: needs info and removed s: needs work labels Jun 1, 2016

tscrim added s: positive review and removed s: needs info labels Jun 1, 2016

vbraun removed the s: positive review label Jun 2, 2016

vbraun closed this as completed in 835edb9 Jun 2, 2016

This was referenced Jun 2, 2016

Cython interface to libhomfly #18057

Closed

HOMFLY polynomial of links. #20719

Closed

Add libhomfly as optional package #18047

Add libhomfly as optional package #18047

Comments

miguelmarco commented Mar 24, 2015

This comment has been minimized.

miguelmarco commented Mar 24, 2015

kcrisman commented Mar 24, 2015

kcrisman commented Mar 24, 2015

vbraun commented Mar 24, 2015

jdemeyer commented Mar 25, 2015

sagetrac-git mannequin commented Mar 27, 2015

sagetrac-git mannequin commented Mar 27, 2015

miguelmarco commented Mar 27, 2015

vbraun commented Mar 27, 2015

sagetrac-git mannequin commented Mar 27, 2015

sagetrac-git mannequin commented Mar 27, 2015

miguelmarco commented Mar 27, 2015

sagetrac-git mannequin commented Mar 28, 2015

sagetrac-git mannequin commented Mar 28, 2015

vbraun commented Mar 28, 2015

miguelmarco commented Mar 30, 2015

miguelmarco commented Mar 30, 2015

miguelmarco commented May 30, 2016

miguelmarco commented May 30, 2016

sagetrac-git mannequin commented May 30, 2016

sagetrac-git mannequin commented May 30, 2016

tscrim commented Jun 1, 2016

tscrim commented Jun 1, 2016

miguelmarco commented Jun 1, 2016

tscrim commented Jun 1, 2016

sagetrac-git mannequin commented Jun 1, 2016

sagetrac-git mannequin commented Jun 1, 2016

miguelmarco commented Jun 1, 2016

tscrim commented Jun 1, 2016

miguelmarco commented Jun 1, 2016

tscrim commented Jun 1, 2016

tscrim commented Jun 1, 2016

miguelmarco commented Jun 1, 2016

tscrim commented Jun 1, 2016

vbraun commented Jun 2, 2016

kcrisman commented Jun 16, 2016

kcrisman commented Jun 16, 2016

miguelmarco commented Jun 16, 2016

kcrisman commented Jun 16, 2016

jdemeyer commented Jun 23, 2016