Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#18047 closed enhancement (fixed)

Add libhomfly as optional package

Reported by: mmarco Owned by:
Priority: minor Milestone: sage-7.3
Component: packages: optional Keywords: days74
Cc: vbraun, kcrisman, amitjamadagni, fugelde, tscrim Merged in:
Authors: Miguel Marco Reviewers: Jeroen Demeyer, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: cc13df1 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

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: libhomfly-1.0.tar.gz

Attachments (2)

libhomfly-1.0.tar.2.gz (328.1 KB) - added by mmarco 6 years ago.
libhomfly-1.0.tar.gz (348.3 KB) - added by mmarco 5 years ago.

Download all attachments as: .zip

Change History (65)

comment:1 Changed 6 years ago by mmarco

  • Cc vbraun kcrisman amitjamadagni added
  • Component changed from PLEASE CHANGE to packages: optional
  • Description modified (diff)
  • Priority changed from major to minor

comment:2 Changed 6 years ago by mmarco

  • Summary changed from help to Add libhomfly as optional package

comment:3 Changed 6 years ago by mmarco

  • Branch set to u/mmarco/libhomfly

comment:4 Changed 6 years ago by kcrisman

  • Commit set to 458d6bfe134685bce08a133cfbd440b31595b272
  • Type changed from PLEASE CHANGE to enhancement

New commits:

458d6bfInitial commit

comment:5 Changed 6 years ago by vbraun

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.

comment:6 in reply to: ↑ description Changed 6 years ago by jdemeyer

Replying to mmarco:

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.

comment:7 Changed 6 years ago by git

  • Commit changed from 458d6bfe134685bce08a133cfbd440b31595b272 to 14112a4f0e85cf0e2678f958208261d76e2cc43b

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

14112a4Moved to autotools-libtools

Changed 6 years ago by mmarco

comment:8 Changed 6 years ago by mmarco

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

comment:9 Changed 6 years ago by vbraun

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.

comment:10 Changed 6 years ago by git

  • Commit changed from 14112a4f0e85cf0e2678f958208261d76e2cc43b to 2edefa1c8a5bf75db3bffd8f47f54d673b8e3f31

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

2edefa1Added test

comment:11 Changed 6 years ago by mmarco

Made the change and added a little test

comment:12 Changed 6 years ago by git

  • Commit changed from 2edefa1c8a5bf75db3bffd8f47f54d673b8e3f31 to f19093fbf3e30906eac13c928f935e97cdb9d8e7

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

f19093fFixed checksums

comment:13 Changed 6 years ago by vbraun

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.

comment:14 Changed 6 years ago by mmarco

The cython interface is being done in #18057

comment:15 Changed 6 years ago by mmarco

Uploaded new tarball

comment:16 Changed 5 years ago by mmarco

  • Cc fugelde tscrim added

comment:17 Changed 5 years ago by mmarco

  • Keywords days74 added

comment:18 Changed 5 years ago by mmarco

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

comment:19 Changed 5 years ago by git

  • Commit changed from f19093fbf3e30906eac13c928f935e97cdb9d8e7 to 090c7fd5009d64fc0f447bd3b1456b1b6fbc60d7

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

be71e3eInitial commit
5b6fae2Moved to autotools-libtools
7bcf75aAdded test
d14ec25Fixed checksums
371e368Initial commit
249c987Moved to autotools-libtools
8eedce7Added test
ad156b3Fixed checksums
b550750Rebased, updated checksums and added type
090c7fdMerge branch 'u/mmarco/libhomfly' of git://trac.sagemath.org/sage into t/18047/libhomfly

comment:20 Changed 5 years ago by mmarco

  • Status changed from new to needs_review

comment:21 Changed 5 years ago by jdemeyer

Author name missing

comment:22 Changed 5 years ago by mmarco

  • Authors set to Miguel Marco

comment:23 Changed 5 years ago by vdelecroix

Hey,

sided comment: Why do you want it a standard package? You are free to use whatever optional package into Sage source code. I am very against having one more rather marginal package as a standard package. On the other hand, I am very much for a better test/integration of optional packages.

Vincent

comment:24 Changed 5 years ago by mmarco

Right now i am only proposing it to be optional. I would like to become standard in the future because it provides an important functionality for the knot theory module. The homfly polynomial is one of the "must have" invariants of a link nowadays. If we want our module to be more or less feature complete, we need to provide this functionality out of the box. So we either write our own implementation or integrate one that is available.

comment:25 Changed 5 years ago by tscrim

  • Milestone changed from sage-6.6 to sage-7.3

I'm going through the pkg source code right now trying to see if I can make it so we can interface with it better.

comment:26 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Tarball???

comment:27 Changed 5 years ago by jdemeyer

A dependencies file is missing.

comment:28 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:29 Changed 5 years ago by jdemeyer

  • Branch changed from u/mmarco/libhomfly to u/jdemeyer/libhomfly

comment:30 Changed 5 years ago by git

  • Commit changed from 090c7fd5009d64fc0f447bd3b1456b1b6fbc60d7 to 1e550dd277dae4c117b2e44b8260f28cf6058178

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

1e550ddMove testsuite to spkg-check

comment:31 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_work to needs_review

comment:32 Changed 5 years ago by git

  • Commit changed from 1e550dd277dae4c117b2e44b8260f28cf6058178 to f77d835d71c8c9e9e3f69defc1a0196c22e5e25d

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

f77d835Move testsuite to spkg-check

comment:33 Changed 5 years ago by jdemeyer

For me, this is good.

comment:34 follow-up: Changed 5 years ago by mmarco

Is the dependencies file really necessary?

comment:35 follow-up: Changed 5 years ago by mmarco

Also, I don't see any code in spkg-test. Did you really paste the code from spkg-build?

comment:36 in reply to: ↑ 34 Changed 5 years ago by jdemeyer

Replying to mmarco:

Is the dependencies file really necessary?

Unfortunately not.

comment:37 in reply to: ↑ 35 Changed 5 years ago by jdemeyer

Replying to mmarco:

Also, I don't see any code in spkg-test. Did you really paste the code from spkg-build?

I guess you mean spkg-check. There already was code to run $MAKE check there, so I didn't need to add anything. I just deleting the running of the testsuite from spkg-install.

comment:38 Changed 5 years ago by mmarco

Ok then. Fine for me.

comment:39 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:40 Changed 5 years ago by mmarco

  • Status changed from positive_review to needs_work

We are still working with Travis in improving the library.

comment:41 Changed 5 years ago by mmarco

  • Branch changed from u/jdemeyer/libhomfly to u/mmarco/libhomfly

comment:42 Changed 5 years ago by git

  • Commit changed from f77d835d71c8c9e9e3f69defc1a0196c22e5e25d to 32ab69acccc9e6027a2d2812b51e5f5204212854

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

32ab69aUpdated checksums

comment:43 Changed 5 years ago by tscrim

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Travis Scrimshaw
  • Status changed from needs_work to positive_review

comment:44 Changed 5 years ago by git

  • Commit changed from 32ab69acccc9e6027a2d2812b51e5f5204212854 to 3ffaf7db929c15a219299d348cf045e5eda34959
  • 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:

3ffaf7dUpdated checksums

comment:45 Changed 5 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:46 Changed 5 years ago by git

  • Commit changed from 3ffaf7db929c15a219299d348cf045e5eda34959 to c37cb34fcc22aba73524a2c214f89685d3eed64a
  • 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:

c37cb34Fixed checksums

comment:47 follow-up: Changed 5 years ago by tscrim

  • Status changed from needs_review to needs_work

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

comment:48 in reply to: ↑ 47 Changed 5 years ago by tscrim

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.

comment:49 Changed 5 years ago by mmarco

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

comment:50 Changed 5 years ago by tscrim

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

Last edited 5 years ago by tscrim (previous) (diff)

Changed 5 years ago by mmarco

comment:51 Changed 5 years ago by git

  • Commit changed from c37cb34fcc22aba73524a2c214f89685d3eed64a to cc13df1e5982b86a9f7df1a340ceae08b30ffede

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

cc13df1New checksums

comment:52 Changed 5 years ago by mmarco

It should be included now.


New commits:

cc13df1New checksums

comment:53 Changed 5 years ago by tscrim

  • Status changed from needs_work to needs_info

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.

comment:54 Changed 5 years ago by mmarco

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

comment:55 Changed 5 years ago by tscrim

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.

comment:56 Changed 5 years ago by tscrim

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

comment:57 Changed 5 years ago by mmarco

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

comment:58 Changed 5 years ago by tscrim

  • Status changed from needs_info to positive_review

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.

comment:59 Changed 5 years ago by vbraun

  • Branch changed from u/mmarco/libhomfly to cc13df1e5982b86a9f7df1a340ceae08b30ffede
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:60 Changed 5 years ago by kcrisman

  • Commit cc13df1e5982b86a9f7df1a340ceae08b30ffede deleted

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

Last edited 5 years ago by kcrisman (previous) (diff)

comment:61 Changed 5 years ago by mmarco

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?

comment:62 Changed 5 years ago by kcrisman

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

comment:63 Changed 5 years ago by jdemeyer

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

Note: See TracTickets for help on using tickets.