Bug 1185019 - Review Request: gap-pkg-tomlib - GAP Table of Marks package
Summary: Review Request: gap-pkg-tomlib - GAP Table of Marks package
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1185020
TreeView+ depends on / blocked
 
Reported: 2015-01-22 17:29 UTC by Jerry James
Modified: 2015-02-09 05:26 UTC (History)
2 users (show)

Fixed In Version: gap-pkg-tomlib-1.2.5-3.fc21
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-02-09 05:26:58 UTC
Type: ---
zbyszek: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Jerry James 2015-01-22 17:29:40 UTC
Spec URL: https://jjames.fedorapeople.org/gap-pkg-tomlib/gap-pkg-tomlib.spec
SRPM URL: https://jjames.fedorapeople.org/gap-pkg-tomlib/gap-pkg-tomlib-1.2.5-1.fc22.src.rpm
Fedora Account System Username: jjames
Description: This is a rename review.  The original review is bug 769450.

This package provides access to several hundred tables of marks of almost simple groups and their maximal subgroups.

Comment 1 Zbigniew Jędrzejewski-Szmek 2015-01-29 18:08:04 UTC
Package is nice and clean. But compression could be improved.

1. gzip --best is slow, and not very efficient. Would it be possible to switch to xz?

$ ls -1s tms13.tom*
35516 tms13.tom
 8528 tms13.tom.gz    (this is with --best, 12s)
 8696 tms13.tom.gz    (this is with default settings, 4s)
 3976 tms13.tom.xz    (this is with default settings, 31s)
 5932 tms13.tom.xz    (this is with -3, 7s)

Switching to xz would give nice space savings. Using defaults for gzip or slightly reduced compression for xz would give nice time savings.

2. Compression should be parallized. Maybe add a Makefile.gzip as Source1

%.gz: %
    gzip $<

and call make -f ${SOURCE1} $(for i in data/*.tom; echo $i.gz) _%{smp_flags}

Comment 2 Jerry James 2015-01-29 18:26:46 UTC
Thank you for taking this review.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> 1. gzip --best is slow, and not very efficient. Would it be possible to
> switch to xz?

Unfortunately, no.  GAP itself can read gzipped files, but does not have the capability of reading files compressed with anything else.  I will have to look at coding up xz support and sending it upstream.  For now, though, gzip is the only possibility.

Since gzip is required, I prefer to use --best for the space savings.  On my 6 year old x86_64 machine, compressing the files with gzip takes slightly over 1 minute.  Spending 1 minute at build time to save space on every user's machine seems like a good tradeoff to me.  (Of course, the package might be built on an ARM machine, which would require a greater amount of time, but it still seems like a good tradeoff, in my opinion.)

> 2. Compression should be parallized. Maybe add a Makefile.gzip as Source1
> 
> %.gz: %
>     gzip $<
> 
> and call make -f ${SOURCE1} $(for i in data/*.tom; echo $i.gz) _%{smp_flags}

I don't see the point.  The compression is done once at build time.  The package is noarch, so it only needs to be built on one architecture.  It only takes a minute (or so) to do.  There isn't much to save by compressing in parallel.  And, on the other hand, compressing in parallel will use more CPU and memory at one time, which may or may not be a problem, depending on what other builds the boxing is doing at the same time.

Comment 3 Zbigniew Jędrzejewski-Szmek 2015-01-29 18:30:18 UTC
(In reply to Jerry James from comment #2)
> Thank you for taking this review.
> 
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> > 1. gzip --best is slow, and not very efficient. Would it be possible to
> > switch to xz?
> 
> Unfortunately, no.  GAP itself can read gzipped files, but does not have the
> capability of reading files compressed with anything else.  I will have to
> look at coding up xz support and sending it upstream.  For now, though, gzip
> is the only possibility.
> 
> Since gzip is required, I prefer to use --best for the space savings.  On my
> 6 year old x86_64 machine, compressing the files with gzip takes slightly
> over 1 minute.  Spending 1 minute at build time to save space on every
> user's machine seems like a good tradeoff to me.  (Of course, the package
> might be built on an ARM machine, which would require a greater amount of
> time, but it still seems like a good tradeoff, in my opinion.)

OK, that's a valid trade-off.

> > 2. Compression should be parallized. Maybe add a Makefile.gzip as Source1
> > 
> > %.gz: %
> >     gzip $<
> > 
> > and call make -f ${SOURCE1} $(for i in data/*.tom; echo $i.gz) _%{smp_flags}
> 
> I don't see the point.  The compression is done once at build time.  The
> package is noarch, so it only needs to be built on one architecture.  It
> only takes a minute (or so) to do.  There isn't much to save by compressing
> in parallel.  And, on the other hand, compressing in parallel will use more
> CPU and memory at one time, which may or may not be a problem, depending on
> what other builds the boxing is doing at the same time.

Guidelines are pretty clear: parallelized builds are required, unless the
package cannot be built that way. But it's not a problem to do it here.

Comment 4 Zbigniew Jędrzejewski-Szmek 2015-01-29 18:35:58 UTC
Actually parallel might be a better option:

  ls data/*.tom | parallel gzip --best

Comment 5 Zbigniew Jędrzejewski-Szmek 2015-01-29 18:38:54 UTC
Actually:
  ls -1 data/*.tom | parallel --no-notice -j12 gzip --best

Comment 6 Jerry James 2015-01-29 22:00:00 UTC
Okay, I like the use of GNU parallel.  However, I don't like the magic number 12.  What about dropping the -j option entirely and letting it use whatever number of CPUs are available?  Like this:

Spec URL: https://jjames.fedorapeople.org/gap-pkg-tomlib/gap-pkg-tomlib.spec
SRPM URL: https://jjames.fedorapeople.org/gap-pkg-tomlib/gap-pkg-tomlib-1.2.5-2.fc22.src.rpm

Comment 7 Zbigniew Jędrzejewski-Szmek 2015-01-30 02:44:59 UTC
It's recommended to use %{_smp_mflags}. This allows the number of threads to be overridden if necessary:

  parallel %{?_smp_mflags} --no-notice gzip -1 ::: data/*.tom

Is the license specified anywhere upstream?

Comment 8 Zbigniew Jędrzejewski-Szmek 2015-01-30 02:59:46 UTC
One more question: why /usr/lib/gap/pkg and not /usr/share/something? Is the place upstream expects?

Comment 9 Jerry James 2015-01-30 04:16:52 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> It's recommended to use %{_smp_mflags}. This allows the number of threads to
> be overridden if necessary:
> 
>   parallel %{?_smp_mflags} --no-notice gzip -1 ::: data/*.tom

Ah, good solution.  I will make that change.

> Is the license specified anywhere upstream?

A few versions ago, there was nothing about the license anywhere in the distribution.  I wrote to Liam Naughton, one of the upstream developers, as follows:

"Hello Liam,

I am working on adding GAP and some of its packages to the Fedora
Linux distribution.  However, I don't see any indication of the
license under which TomLib is distributed.  Did you intend it to have
the same license as GAP itself, namely GPL version 2 or any later
version?  If so, would you mind adding a statement about the license
to the TomLib web site or, even better, to the tar file itself?"

He responded as follows:

"Hi Jerry,

I've added a copyright notice to the tomlib homepage and to the manual. We adopt the same copyright notice as GAP.

All the best

Liam"

And indeed, you can now see the sentence "We adopt the copyright regulations of GAP as detailed in the copyright notice in the GAP manual." on http://schmidt.nuigalway.ie/tomlib/ and in several files in doc/.  The manual is included in the gap-online-help package, or you can view the copyright section online here:

http://www.gap-system.org/Manuals/doc/ref/chap1.html#X7950EFA183E3F666

where you can see that it indicates GPLv2+ as the license.  That is much more roundabout than I would prefer, but that's the story.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #8)
> One more question: why /usr/lib/gap/pkg and not /usr/share/something? Is the
> place upstream expects?

Yes, it is.  Even though most GAP add-on packages are noarch, some (such as gap-pkg-io and gap-pkg-browse) have binary components.  Therefore, /usr/share seems inappropriate for those packages.  However, they can't be multiarch, since they depend on the gap binary, so there is no need to put them in %{_libdir} either.  Hence, they go into /usr/lib, thereby allowing the noarch packages to remain noarch.

Here is the version using %{?_smp_mflags}:
Spec URL: https://jjames.fedorapeople.org/gap-pkg-tomlib/gap-pkg-tomlib.spec
SRPM URL: https://jjames.fedorapeople.org/gap-pkg-tomlib/gap-pkg-tomlib-1.2.5-3.fc22.src.rpm

Comment 10 Zbigniew Jędrzejewski-Szmek 2015-01-30 04:27:12 UTC
License is OK, package builds in mock, sources are OK, name seems OK, right flags are used. The use of /usr/lib/gap is explained above, debuginfo package is OK, Provides and Obsoletes are correct.

rpmlint:
gap-pkg-tomlib.noarch: W: only-non-binary-in-usr-lib
2 packages and 0 specfiles checked; 0 errors, 1 warnings.
OK.

Package is APPROVED.

Comment 11 Jerry James 2015-01-30 15:27:16 UTC
New Package SCM Request
=======================
Package Name: gap-pkg-tomlib
Short Description: GAP Table of Marks package
Upstream URL: http://schmidt.nuigalway.ie/tomlib/
Owners: jjames
Branches: f21
InitialCC:

Comment 12 Gwyn Ciesla 2015-01-30 16:07:31 UTC
Git done (by process-git-requests).

Comment 13 Fedora Update System 2015-01-30 23:26:32 UTC
gap-pkg-tomlib-1.2.5-3.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/gap-pkg-tomlib-1.2.5-3.fc21

Comment 14 Fedora Update System 2015-02-01 00:25:02 UTC
gap-pkg-tomlib-1.2.5-3.fc21 has been pushed to the Fedora 21 testing repository.

Comment 15 Fedora Update System 2015-02-09 05:26:58 UTC
gap-pkg-tomlib-1.2.5-3.fc21 has been pushed to the Fedora 21 stable repository.


Note You need to log in before you can comment on or make changes to this bug.