Ticket #8906 (needs_info enhancement)

Opened 3 years ago

Last modified 21 months ago

Optional package for gap3

Reported by: mrobado Owned by: tbd
Priority: major Milestone: sage-5.10
Component: packages: optional Keywords: gap3
Cc: burcin Work issues:
Report Upstream: N/A Reviewers: Burcin Erocal
Authors: Jean Michel, Marco Robado, Nicolas M. Thiéry Merged in:
Dependencies: Stopgaps:

Description (last modified by nthiery) (diff)

Here is an spkg wich contains a patched version of gap3 that compiles on linux/x86 and macosx/x86.

 http://thales.math.uqam.ca/~robado/gap3-0.3.spkg

Jean's spkg merged with the above:

 http://sage.math.washington.edu/home/nthiery/gap3-jm2.spkg

Change History

comment:1 Changed 3 years ago by burcin

  • Status changed from new to needs_work
  • Reviewers set to Burcin Erocal

Thanks a lot for the package, and a good example of how to write spkg-install in Python.

Here are a few comments:

  • Version numbers for the GAP4 package are of the form gap-4.4.10.p17, so we should name this gap.3.4.4.p0 and keep increasing the "patch level" at the end for updates.
  • The package has a single mercurial repository at the top, which contains spkg-install, SPKG.txt, etc. and the GAP3 source files. The convention is to keep two separate repositories, one with only "packaging" material, spkg-install and so on, another one to track changes to the upstream sources.
  • Does GAP3 have a test suite? Can we add an spkg-check which runs this? Another option is to copy the spkg-check which just tries to start GAP from the spkg mentioned in comment:9:ticket:8380.
  • SPKG.txt should be based on the template here:  http://wiki.sagemath.org/spkgTemplate In particular:
    • the changelog should have an entry conforming to the standard
    • TODO list (in the file TODO) should be moved in the SPKG.txt
    • There should be an explanation of what changes were made to the upstream sources, even though they are already documented in the mercurial repository log.
    • SPKG.txt should note that src/bin/gap.sh hardcodes version gap3r4p4 in the GAP_DIR variable
  • I don't see why these lines at the end of spkg-install are necessary:
    if not os.path.exists(gap3_base):
        os.makedirs(gap3_base)
    

comment:2 Changed 3 years ago by mrobado

  • Status changed from needs_work to needs_review

I uploaded a new version of the package with the issues fixed at this address  http://thales.math.uqam.ca/~robado/gap-3.4.4.p0.spkg . Maybe we should merge this package with the one posted on #8380.

comment:3 Changed 3 years ago by burcin

  • Status changed from needs_review to positive_review

I don't know what the exact requirements for an optional package are. Especially the difference between an optional and an experimental package is not clear to me. I can't find them anywhere in the documentation either.

The package at  http://thales.math.uqam.ca/~robado/gap-3.4.4.p0.spkg looks good to me, switching to positive review. :)

comment:4 follow-up: ↓ 5 Changed 3 years ago by mvngu

  • Status changed from positive_review to needs_work

Some problems with gap-3.4.4.p0.spkg:

  1. All changes have not been checked in:
    [mvngu@sage gap-3.4.4.p0]$ hg status
    M SPKG.txt
    ? spkg-check
    
    You must check in all changes.
  2. The license file COPYING needs to go under the directory src/.
  3. The directory src/ contains one package within another:
    [mvngu@sage src]$ pwd
    /home/mvngu/spkg/optional/gap/gap-3.4.4.p0/src
    [mvngu@sage src]$ ls
    bin            description12  description4  description8  grp  sml  tom
    description1   description13  description5  description9  htm  src  tst
    description10  description2   description6  doc           lib  tbl  two
    description11  description3   description7  etc           pkg  thr  utl
    [mvngu@sage src]$ ls src
    agcollec.c     description12  function.c     pcpresen.c  record.h    tietze.h
    agcollec.h     description13  function.h     pcpresen.h  scanner.c   tom
    aggroup.c      description2   gap.c          permutat.c  scanner.h   tst
    aggroup.h      description3   gasman.c       permutat.h  set.c       two
    bin            description4   gasman.h       pkg         set.h       unknown.c
    blister.c      description5   grp            plist.c     sml         unknown.h
    blister.h      description6   htm            plist.h     src         utl
    coding.c       description7   idents.c       polynom.c   statemen.c  vecffe.c
    coding.h       description8   idents.h       polynom.h   statemen.h  vecffe.h
    costab.c       description9   integer.c      range.c     string.c    vector.c
    costab.h       doc            integer.h      range.h     string.h    vector.h
    cyclotom.c     etc            lib            rational.c  system.c    word.c
    cyclotom.h     eval.c         list.c         rational.h  system.h    word.h
    description1   eval.h         list.h         read.c      tbl
    description10  finfield.c     Makefile       read.h      thr
    description11  finfield.h     Makefile.orig  record.c    tietze.c
    
  4. You don't have any patches on top of the upstream GAP 3 package, so you need to start the spkg numbering at gap-3.4.4.spkg, not at gap-3.4.4.p0.spkg. The name gap-3.4.4.p0.spkg implies that you have a patch to be applied on top of the upstream GAP 3 package. See this chapter Producing New Sage Packages and chapter Patching a Sage Package of the Developer's Guide.

comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 3 years ago by burcin

Hi Minh,

Replying to mvngu:

Some problems with gap-3.4.4.p0.spkg:

  1. All changes have not been checked in:
    [mvngu@sage gap-3.4.4.p0]$ hg status
    M SPKG.txt
    ? spkg-check
    
    You must check in all changes.
  2. The license file COPYING needs to go under the directory src/.

Fair enough, I didn't check these again myself. Note that the COPYING file doesn't exist in the original gap3 distribution. We decided to make one after it took us a while to discover the license has a non-commercial use restriction.

  1. The directory src/ contains one package within another:

This is just how the gap3 source is laid out. There are gap library files, and gap packages in the other directories. I don't see why this is a problem.

  1. You don't have any patches on top of the upstream GAP 3 package, so you need to start the spkg numbering at gap-3.4.4.spkg, not at gap-3.4.4.p0.spkg. The name gap-3.4.4.p0.spkg implies that you have a patch to be applied on top of the upstream GAP 3 package.

What is in the spkg is very far from the original gap3 distribution. The changes couldn't be tracked with patches, so there is a mercurial repository in the src directory. Marco did a tremendous job creating a GAP package which actually compiles and works on different platforms, and includes the latest versions of various GAP packages still being maintained.

I agree with your points 1 and 2, but IMHO 3 and 4 are not problems that need to be addressed before this is accepted.

Thanks.
Burcin

comment:6 in reply to: ↑ 5 Changed 3 years ago by mvngu

Replying to burcin:

Note that the COPYING file doesn't exist in the original gap3 distribution. We decided to make one after it took us a while to discover the license has a non-commercial use restriction.

Could you document that in the file SPKG.txt? Future reviewers/maintainers might not know about this.

  1. The directory src/ contains one package within another:

This is just how the gap3 source is laid out. There are gap library files, and gap packages in the other directories. I don't see why this is a problem.

Could you document that in the file SPKG.txt? Future reviewers/maintainers might not know about this.

What is in the spkg is very far from the original gap3 distribution. The changes couldn't be tracked with patches, so there is a mercurial repository in the src directory.

Could you document that in the file SPKG.txt? Future reviewers/maintainers might not know about this.

As you can tell, I raised the above objections because the file SPKG.txt did not document the reasons why the spkg was structured as given.

comment:7 Changed 3 years ago by nthiery

Hi!

Thanks Marco for your work!

With Jean-Michel, we have merged Marco's changes into our original spkg (with the changes to the sources upstreamed in Jean's distribution). We will upload the result shortly here.

About the spkg name. Since the gap3 spkg is completely different from the gap4 one, and is supposed to cohabit with it, I would argue for calling the spkg gap3-???? to make a clear distinction. It is further based on Jean's gap3 distribution. So, would you recommend: calling it gap3-jm2.spkg or gap3-3.4.4-jm2.spkg ?

Jean would prefer the first option, since anyway gap3's version has not changed since 1997, and won't ever change again.

comment:8 Changed 3 years ago by nthiery

  • Description modified (diff)
  • Authors changed from Marco Robado to Jean Michel, Marco Robado, Nicolas M. Thiéry

comment:9 Changed 3 years ago by nthiery

Hi!

I just obtained the following with gap3 on winxp1 (win XP + cygwin host in the Sage build farm):

gap> W := CoxeterGroup("E",8);
CoxeterGroup("E",8)
gap> Size(W);                
696729600

For the record: I did not have Sage installed, so I did not try directly the spkg. Instead, I built and ran it by hand, using something like:

tar xvf gap3-jm2.spkg
cd gap3-jm2/src/src
make ibm-i386-linux-gcc
cd ..
cp src/gap.exe bin
<EDIT bin/gap.sh>
bin/gap.sh

In bin/gap.sh, I set GAP_PRG=gap.exe, and downgraded GAP_MEM=512m to GAP_MEM=64m (otherwise I was getting an error Gasman: can not get memory for the initial workspace).

So I assume that it should be easy to adapt the spkg to also work on windows+cygwin once sage will be more publicly available there (windows 7 still to be tested).

comment:10 Changed 21 months ago by saliola

  • Status changed from needs_work to needs_info

What is the status of this spkg? I've used this with several versions of sage with Ubuntu and MacOSX (pre-Lion), and I have had no problems. I would be great if this appeared in the optional spkg list.

[Aside: the trac preview claims I deleted the dependencies, but I did not.]

Note: See TracTickets for help on using tickets.