Opened 2 years ago

Closed 19 months ago

Last modified 19 months ago

#27295 closed enhancement (fixed)

Add GAP's Semigroups package to gap_packages

Reported by: nthiery Owned by:
Priority: major Milestone: sage-8.9
Component: interfaces Keywords: GAP packages, semigroups
Cc: dimpase, isuruf Merged in:
Authors: Dima Pasechnik Reviewers: Isuru Fernando
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: 426edfe (Commits) Commit:
Dependencies: #27396, #28088 Stopgaps:

Description (last modified by dimpase)

Semigroup depends on a number of GAP packages; io (cf. #26930), Digraphs (that uses bliss), orb, and genss. So they all need to be added.

Change History (42)

comment:1 Changed 2 years ago by dimpase

  • Dependencies set to #26930
  • Description modified (diff)

comment:2 Changed 2 years ago by dimpase

of these, only Semigroups doesn't want to get installed.

#W dlopen() error: libsemigroups.so.0: cannot open shared object file: No such file or directory
Error, module '/mnt/opt/Sage/sage-dev/local/share/gap/pkg/semigroups-3.0.20/bin/x86_64-pc-linux-gnu-default64/semigroups.so' not found in
  if not LOAD_DYN( arg[1], false ) then
    Error( "no support for dynamic loading" );
fi; at /mnt/opt/Sage/sage-dev/local/share/gap/lib/files.gd:583 called from 
<function "LoadDynamicModule">( <arguments> )
 called from read-eval loop at /mnt/opt/Sage/sage-dev/local/share/gap/pkg/semigroups-3.0.20/init.g:26
Error, was not in any namespace at /mnt/opt/Sage/sage-dev/local/share/gap/lib/variable.g:291 called from
LEAVE_NAMESPACE(  ); at /mnt/opt/Sage/sage-dev/local/share/gap/lib/package.gi:1253 called from
ReadPackage( pkgname, "init.g" ); at /mnt/opt/Sage/sage-dev/local/share/gap/lib/package.gi:1616 called from
<function "LoadPackage">( <arguments> )
 called from read-eval loop at *stdin*:1
...

And indeed:

$ find local/ -name semigroups.so
local/lib/gap/pkg/semigroups-3.0.20/bin/x86_64-pc-linux-gnu-default64/semigroups.so
$ find local/ -name libsemigroups.so
local/lib/gap/pkg/semigroups-3.0.20/bin/lib/libsemigroups.so
$ ldd `find local/ -name libsemigroups.so`
	linux-vdso.so.1 (0x00007ffd85de3000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fa1ffc9e000)
	libstdc++.so.6 => /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/libstdc++.so.6 (0x00007fa1ffa95000)
	libm.so.6 => /lib64/libm.so.6 (0x00007fa1ff91a000)
	libc.so.6 => /lib64/libc.so.6 (0x00007fa1ff74b000)
	libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/libgcc_s.so.1 (0x00007fa1ff731000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fa1ffd35000)
$ ldd `find local/ -name semigroups.so`
	linux-vdso.so.1 (0x00007ffd8e160000)
	libsemigroups.so.0 => not found
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007fd7baaf4000)
	libstdc++.so.6 => /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/libstdc++.so.6 (0x00007fd7ba8eb000)
	libm.so.6 => /lib64/libm.so.6 (0x00007fd7ba770000)
	libc.so.6 => /lib64/libc.so.6 (0x00007fd7ba5a1000)
	libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/8.3.0/libgcc_s.so.1 (0x00007fd7ba585000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fd7bab88000)

comment:3 Changed 2 years ago by dimpase

This is after applying

  • build/pkgs/gap_packages/spkg-install

    diff --git a/build/pkgs/gap_packages/spkg-install b/build/pkgs/gap_packages/spkg-install
    index d8260b9aa0..4592675553 100644
    a b sdh_install \ 
    1919    crystcat \
    2020    design \
    2121    gbnp \
     22    genss-* \
    2223    Hap* \
    2324    HAPcryst \
    2425    hecke-* \
    done 
    7677# These packages have a new-style autoconf ./configure
    7778# that takes --with-gaproot
    7879
    79 for pkg in nq-* io-*
     80for pkg in nq-* io-* orb-* digraphs-* semigroups-*
    8081do
    8182    cd "$PKG_SRC_DIR/$pkg"
    8283    sdh_configure --with-gaproot="$GAP_ROOT"

to the branch of #26930.

comment:4 Changed 2 years ago by dimpase

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

This is a sort of upstream issue - they install a libsemigroup, a C++ library, in the tree, etc etc. I've opened https://github.com/gap-packages/Semigroups/issues/580 ---a clean way would be to install in advance, libsemigroup separately.

comment:5 Changed 2 years ago by dimpase

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

upstream welcomes PRs, so I'll try doing this in a proper way.

comment:6 Changed 2 years ago by dimpase

  • Dependencies changed from #26930 to #26930, #27396

comment:7 Changed 2 years ago by dimpase

  • Keywords GAP packages semigroups added

comment:8 Changed 2 years ago by fbissey

digraphs includes its own copy of bliss and just trying to do the right thing they just digraphs name_spaced bliss functions.

fbissey@moonloop ~/sandbox/gap-4.10.0/pkg/digraphs-0.13.0 $ diff -u /dev/shm/portage/sci-libs/bliss-0.73-r1/work/bliss-0.73/bliss_C.h src/bliss-0.73/bliss_C.h 
--- /dev/shm/portage/sci-libs/bliss-0.73-r1/work/bliss-0.73/bliss_C.h   2015-09-01 19:23:10.000000000 +1200
+++ src/bliss-0.73/bliss_C.h    2018-11-02 11:56:12.000000000 +1300
@@ -1,5 +1,5 @@
-#ifndef BLISS_C_H
-#define BLISS_C_H
+#ifndef bliss_digraphs_C_H
+#define bliss_digraphs_C_H
 
 /*
   Copyright (c) 2003-2015 Tommi Junttila
@@ -37,14 +37,14 @@
 /**
  * \brief The true bliss graph is hiding behind this typedef.
  */
-typedef struct bliss_graph_struct BlissGraph;
+typedef struct bliss_digraphs_graph_struct BlissGraph;
....

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

I don't seem to have a problem with digraphs, it installs just fine, and uses a static copy of bliss, it seems.

Do you mean to say one has to watch out for something, still?

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

Replying to dimpase:

I don't seem to have a problem with digraphs, it installs just fine, and uses a static copy of bliss, it seems.

Do you mean to say one has to watch out for something, still?

No, just don't like it I guess.

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

At least with semigroups, hopefully after my https://github.com/gap-packages/Semigroups/pull/584 is in, it can be done in a clean way.

comment:12 in reply to: ↑ 11 Changed 2 years ago by fbissey

Replying to dimpase:

At least with semigroups, hopefully after my https://github.com/gap-packages/Semigroups/pull/584 is in, it can be done in a clean way.

Looks good. It looks like that will have to wait gap-4.10.2 for it. I just finished updating all my sage-on-gentoo gap packages to offer 4.10.1 and I must say I was shocked when orb ditched autotools for a gac setup a la crypting which I think is an inferior solution. I hope it doesn't become a standard practise across gap packages.

comment:13 Changed 2 years ago by dimpase

Upstream has https://github.com/gap-packages/Semigroups/tree/v3.1.2 which has all the needed changes. Should we make a separate optional package for this?

comment:14 Changed 2 years ago by embray

  • Milestone changed from sage-8.7 to sage-8.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:15 Changed 21 months ago by embray

  • Milestone sage-8.8 deleted

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

comment:16 Changed 20 months ago by dimpase

  • Dependencies changed from #26930, #27396 to #27396, #28088
  • Milestone set to sage-8.9

comment:17 Changed 20 months ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/packages/semigroupsgap
  • Commit set to 6fbdec99d1414e53e539188782b45f65b256e92b
  • Status changed from new to needs_review

New commits:

32aab07update to GAP 4.10.2
e1d5ab1add libsemigroups package
6fbdec9add GAP package semigroups and its deps

comment:18 Changed 20 months ago by git

  • Commit changed from 6fbdec99d1414e53e539188782b45f65b256e92b to d948da7aa8bbfc36483204413860474519621b58

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

d948da7bump up version

comment:19 follow-up: Changed 20 months ago by isuruf

Doesn't Digraphs have the same problem with planarity?

comment:20 in reply to: ↑ 19 ; follow-up: Changed 20 months ago by dimpase

Replying to isuruf:

Doesn't Digraphs have the same problem with planarity?

sorry, what problem?

comment:21 in reply to: ↑ 20 Changed 20 months ago by embray

Replying to dimpase:

Replying to isuruf:

Doesn't Digraphs have the same problem with planarity?

sorry, what problem?

This: https://github.com/gap-packages/Digraphs/pull/207

Any version of Digraphs we include in Sage should have this patch, and should be built with --with-external-planarity (as such, libplanarity then needs to be a dependency of gap_packages, assuming it isn't already).

comment:22 Changed 20 months ago by embray

IIRC there are also some patches to libplanarity that are needed by digraphs. Isuru would know what they are since he included them in the conda package.

comment:23 Changed 20 months ago by dimpase

Does this imply that this ticket is (abot) broken, as it adds Digraphs in an "old" way? How does one test?

Note that https://github.com/gap-packages/Digraphs/pull/207 isn't even in a released version of Digraphs.

comment:24 follow-up: Changed 20 months ago by isuruf

Only patch needed by planarity is extern patch that sage already has.

Does this imply that this ticket is (abot) broken, as it adds Digraphs in an "old" way? How does one test?

Probably. Digraphs adds planarity as a subdirectory just like semigroups does with libsemigroups and therefore planarity inside digraphs is not installed. Since planarity is installed in sage and the two versions of planarity are ABI compatible, you might not see a missing planarity issue. This would break if planarity versions differ in Digraphs and Sage which is pretty unlikely since planarity changes rarely.

Last edited 20 months ago by isuruf (previous) (diff)

comment:25 in reply to: ↑ 24 Changed 20 months ago by dimpase

Replying to isuruf:

Only patch needed by planarity is extern patch that sage already has.

Does this imply that this ticket is (abot) broken, as it adds Digraphs in an "old" way? How does one test?

Probably. Digraphs adds planarity as a subdirectory just like semigroups does with libsemigroups and therefore planarity inside digraphs is not installed. Since planarity is installed in sage and the two versions of planarity are ABI compatible, you might not see a missing planarity issue. This would break if planarity versions differ in Digraphs and Sage which is pretty unlikely since planarity changes rarely.

You're right that Digraphs pick up Sage's libplanarity.so:

$ ldd local/lib/gap/pkg/digraphs-0.15.2/bin/x86_64-pc-linux-gnu-default64-kv3/digraphs.so
	linux-vdso.so.1 (0x00007fff9bdb7000)
	libplanarity.so.0 => /mnt/opt/Sage/sage-dev/local/lib/libplanarity.so.0 (0x00007f55c349d000)
	libstdc++.so.6 => /usr/lib/gcc/x86_64-pc-linux-gnu/9.1.0/libstdc++.so.6 (0x00007f55c3200000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f55c30c3000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f55c2ef3000)
	libgcc_s.so.1 => /usr/lib/gcc/x86_64-pc-linux-gnu/9.1.0/libgcc_s.so.1 (0x00007f55c2ed9000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f55c350e000)

So in this sense the only thing to fix is to add planarity to dependencies of gap_packages. I've checked that Digraphs passes its own testsuite, just in case.

comment:26 Changed 20 months ago by embray

Would still be best to use --with-external-planarity if possible. Or if nothing else, delete the vendored copy it builds before installing into Sage if we definitely don't need it.

comment:27 follow-up: Changed 20 months ago by isuruf

Since there hasn't been a release of Digraphs, it wouldn't work, but it's still okay to add --with-external-planarity and released Digraph's configure just ignores it and you wouldn't have to do anything when a new release comes in.

comment:28 Changed 20 months ago by git

  • Commit changed from d948da7aa8bbfc36483204413860474519621b58 to cec604d9a41846df6409d7344a718cd2cc56246b

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

cec604dcorrect checksum for the tarball

comment:29 Changed 20 months ago by git

  • Commit changed from cec604d9a41846df6409d7344a718cd2cc56246b to 2e6ec3cdc1f338d3eae6e158ab42c8068deb44de

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

2e6ec3cplanarity is a dependency of Digraph GAP package

comment:30 Changed 20 months ago by git

  • Commit changed from 2e6ec3cdc1f338d3eae6e158ab42c8068deb44de to 6f88343fae4694ef32d302cbd758e11c97eb5189

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

6f88343libsemigroup and planarity are dependencies of Digraph and Semigroups GAP packages

comment:31 Changed 20 months ago by git

  • Commit changed from 6f88343fae4694ef32d302cbd758e11c97eb5189 to 426edfea20eb26d1e631f82e4e745e42412128dd

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

426edfeconfigure params array for packages (for passing --with-... if needed)

comment:32 in reply to: ↑ 27 Changed 20 months ago by dimpase

Replying to isuruf:

Since there hasn't been a release of Digraphs, it wouldn't work, but it's still okay to add --with-external-planarity and released Digraph's configure just ignores it and you wouldn't have to do anything when a new release comes in.

I've added this, and missing deps, and the correct tarball checksum for libsemigroups from #27396. I didn't try to remove dead wood copy of planarity installed by Digraph, as this appears to be harmless, and won't be needed once we get updated Digraph.

comment:33 Changed 20 months ago by fbissey

Dear me, I forgot those horrors so fast. No only does Digraphs use plain "planarity" it also include its own vendored (and prefixed so it doesn't clash) version of bliss-0.73. I had some fun patching that in 0.13.0 and 0.15.0. No I may have to reproduce it for 0.15.2.

comment:34 Changed 20 months ago by dimpase

  • Cc isuruf added

comment:35 Changed 20 months ago by isuruf

This package vendors bliss. Is that okay?

comment:36 Changed 20 months ago by dimpase

We know about vendored bliss. It builds it statically, so it should not be a show-stopper. It should be unvendored upstream.

Some of us actually do research in semigroup algorithms, so we want it in...

comment:37 Changed 20 months ago by isuruf

  • Reviewers set to Isuru Fernando
  • Status changed from needs_review to positive_review

Do you mind creating a new trac ticket for unvendoring bliss?

comment:38 Changed 20 months ago by dimpase

To be more precise, it's Digraphs, a dependency of Semigroups, that vendors bliss.

So this is in fact https://github.com/gap-packages/Digraphs/issues/205

Last edited 20 months ago by dimpase (previous) (diff)

comment:39 Changed 20 months ago by fbissey

I have an unvendoring patch in sage-on-gentoo, but it is minimal. I pass LIBS="-lbliss -lplanarity -lgap" to configure, it should be improved to get configure to figure it itself. https://github.com/cschwan/sage-on-gentoo/blob/master/dev-gap/digraphs/files/digraphs-0.15.2-system_pkg.patch

comment:40 Changed 20 months ago by fbissey

Improved unvendoring patch: https://github.com/cschwan/sage-on-gentoo/blob/master/dev-gap/digraphs/files/digraphs-0.15.2-system_pkg-r1.patch

configure now searches for the libraries by itself.

comment:41 Changed 19 months ago by vbraun

  • Branch changed from u/dimpase/packages/semigroupsgap to 426edfea20eb26d1e631f82e4e745e42412128dd
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:42 Changed 19 months ago by embray

  • Commit 426edfea20eb26d1e631f82e4e745e42412128dd deleted

Seems to work on Cygwin too, FWIW.

Note: See TracTickets for help on using tickets.