Opened 2 years ago
Closed 19 months ago
#27396 closed enhancement (fixed)
add libsemigroups package
Reported by:  dimpase  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  packages: optional  Keywords:  GAP packages, semigroups 
Cc:  nthiery, isuruf  Merged in:  
Authors:  Dima Pasechnik  Reviewers:  Travis Scrimshaw, Isuru Fernando 
Report Upstream:  N/A  Work issues:  
Branch:  300736d (Commits, GitHub, GitLab)  Commit:  300736dba9324f872110fd0f91edb29eba3a9664 
Dependencies:  #28088  Stopgaps: 
Description (last modified by )
libsemigroups is a C++ library for computing in semigroups, and is used by semigroups GAP package, which is to be included in Sage, cf. #27295
So this automatically gives it rights to become an optional package.
tarball: http://users.ox.ac.uk/~coml0531/sage/libsemigroups0.6.7.tar.gz
To the release manager, merge simultaneous with #27295.
Change History (25)
comment:1 Changed 2 years ago by
 Branch set to u/dimpase/packages/libsemigroup
 Commit set to 15bf22f537c4f5bcec0a54e8efe9f758e304493e
 Description modified (diff)
 Status changed from new to needs_review
comment:2 followup: ↓ 3 Changed 2 years ago by
It installed and package tests were okay for me (Ubuntu 18.04 LTS). However, I think this is mostly useless package for Sage without the Python bindings (or GAP's semigroups package). So I think we should add the Python bindings as part of this ticket too.
comment:3 in reply to: ↑ 2 ; followup: ↓ 4 Changed 2 years ago by
Replying to tscrim:
It installed and package tests were okay for me (Ubuntu 18.04 LTS). However, I think this is mostly useless package for Sage without the Python bindings (or GAP's semigroups package).
I don't quite understand the point you are trying to make here. This ticket is a dependency of #27295, which is for exactly this, adding GAP's semigroups package.
So I think we should add the Python bindings as part of this ticket too.
No, not here. Adding python bindings should be on another ticket.
comment:4 in reply to: ↑ 3 Changed 2 years ago by
Replying to dimpase:
Replying to tscrim:
It installed and package tests were okay for me (Ubuntu 18.04 LTS). However, I think this is mostly useless package for Sage without the Python bindings (or GAP's semigroups package).
I don't quite understand the point you are trying to make here. This ticket is a dependency of #27295, which is for exactly this, adding GAP's semigroups package.
This does not make sense to me as a standalone (at least, it seems like the Cython code would be complicated to generated examples, but maybe I didn't look closely enough). Based on that, I feel either this should be as part of #27295 or have the Python bindings added to this ticket.
comment:6 Changed 2 years ago by
A ticket should not be trying to do too much, and get too hard to review due to this.
comment:7 Changed 2 years ago by
I agree, but I also think tickets also should have some standalone meaning. Different commits can help granularize changes similar to splitting tickets. I just don't think this quite passes the standalone test.
comment:8 Changed 2 years ago by
We have added standalone C++ libraries without any bindings and a clear plan why they are needed in the past, cf e.g. mprfcx. This one is much better in this respect.
comment:9 Changed 2 years ago by
Different projects have different commits per ticket policies. We don't have any, and I seldom see people reviewing tickets per commit, either.
comment:10 Changed 2 years ago by
 Description modified (diff)
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
comment:11 Changed 2 years ago by
 Milestone changed from sage8.7 to sage8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:12 Changed 20 months ago by
 Milestone changed from sage8.8 to sagepending
comment:13 Changed 20 months ago by
 Dependencies changed from #27295 to #27295, #28088
 Milestone changed from sagepending to sage8.9
comment:14 Changed 20 months ago by
 Description modified (diff)
Updated link in the description to the current source code home of libsemigroups.
comment:16 Changed 20 months ago by
 Commit changed from 15bf22f537c4f5bcec0a54e8efe9f758e304493e to e1d5ab1b8b23eced4721964453249b478f76e64c
 Status changed from positive_review to needs_review
comment:17 Changed 20 months ago by
just a version bump and a rebase over the latest GAP
comment:18 Changed 20 months ago by
 Status changed from needs_review to needs_work
this needs a modified tarball, as the ones available do not come with ./confugure
, one needs autotools to get one.
comment:19 Changed 20 months ago by
 Commit changed from e1d5ab1b8b23eced4721964453249b478f76e64c to 300736dba9324f872110fd0f91edb29eba3a9664
Branch pushed to git repo; I updated commit sha1. New commits:
300736d  correct checksum for the tarball

comment:20 Changed 20 months ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
I've linked the correct tarball, with ready ./configure
. One needs to untar the original upstream tarball, which is bare, run ./autogen
there (having autotools installed), and retar.
comment:21 Changed 20 months ago by
 Cc isuruf added
comment:22 followup: ↓ 23 Changed 20 months ago by
 Dependencies changed from #27295, #28088 to #28088
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Isuru Fernando
 Status changed from needs_review to positive_review
comment:23 in reply to: ↑ 22 Changed 20 months ago by
 Dependencies changed from #28088 to #28088, #27295
comment:24 Changed 19 months ago by
 Dependencies changed from #28088, #27295 to #28088
comment:25 Changed 19 months ago by
 Branch changed from u/dimpase/packages/libsemigroup to 300736dba9324f872110fd0f91edb29eba3a9664
 Resolution set to fixed
 Status changed from positive_review to closed
Ready for review. On another ticket we should also add its Python bindings: https://github.com/jamesdmitchell/libsemigroupspythonbindings/
New commits:
add libsemigroups package