Bug 1185019

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}

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.

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

Actually parallel might be a better option:

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

Actually:
  ls -1 data/*.tom | parallel --no-notice -j12 gzip --best

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

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?

One more question: why /usr/lib/gap/pkg and not /usr/share/something? Is the place upstream expects?

(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

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.

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:

Git done (by process-git-requests).

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

gap-pkg-tomlib-1.2.5-3.fc21 has been pushed to the Fedora 21 testing repository.

gap-pkg-tomlib-1.2.5-3.fc21 has been pushed to the Fedora 21 stable repository.