Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1367598 - Review Request: gap-pkg-guava - Computing with error-correcting codes
Summary: Review Request: gap-pkg-guava - Computing with error-correcting codes
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Paulo Andrade
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-08-16 22:05 UTC by Jerry James
Modified: 2016-09-09 16:55 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-09-04 17:39:56 UTC
paulo.cesar.pereira.de.andrade: fedora-review+


Attachments (Terms of Use)

Description Jerry James 2016-08-16 22:05:04 UTC
Spec URL: https://jjames.fedorapeople.org/gap-pkg-guava/gap-pkg-guava.spec
SRPM URL: https://jjames.fedorapeople.org/gap-pkg-guava/gap-pkg-guava-3.13.1-1.fc26.src.rpm
Fedora Account System Username: jjames
Description: GUAVA is a package that implements coding theory algorithms in GAP.
Codes can be created and manipulated and information about codes can be
calculated.

GUAVA consists of various files written in the GAP language, and an
external program from J. S. Leon for dealing with automorphism groups of
codes and isomorphism testing functions.  Several algorithms that need
the speed are integrated in the GAP kernel.

The functions within GUAVA can be divided into four categories:
- Construction of codes.  GUAVA can construct non-linear, linear and
  cyclic codes over an arbitrary finite field.  Examples are
  HadamardCode, ReedMullerCode, BestKnownLinearCode, QRCode and
  GoppaCode.
- Manipulation of codes.  These functions allow the user to transform
  one code into another or to construct a new code from two codes.
  Examples are PuncturedCode, DualCode, DirectProductCode and UUVCode.
- Computation of information about codes.  This information is stored in
  the code record.  Examples are MinimumDistance, OuterDistribution,
  IsSelfDualCode and AutomorphismGroup.
- Generation of bounds on linear codes.  The table by Brouwer and
  Verhoeff (as it existed in the mid-1990s) is incorporated into GUAVA.
  For example, BoundsMinimumDistance.

Comment 1 Paulo Andrade 2016-08-17 03:10:35 UTC
  It is not required to have:
BuildRequires: gcc

---
  This looks suspecting, as it means it will loop only once:
./src/randschr.c:238:10: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
          if ( hOrder % primeList[i] == 0 )
          ^~
./src/randschr.c:240:13: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
             break;
             ^~~~~
code is:
      for ( i = 0 ; primeList[i] != 0 ; ++i ) {
         if ( hOrder % primeList[i] == 0 )
            raisePermToPower( h, hOrder / primeList[i]);
            break;
      }
and the data is:
guava-3.13.1/src/leon/src/primes.c:Unsigned primeList[] = {2,3,5,7,9,11,13,17,19,23,29,31,37,41,43,47,53,59, ... 0};

---
  || and && have different precedence, so this code is also suspecting:
./src/randschr.c:391:43: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
                   lowestOrder == curOrder && levelLowestOrder > finalLevel) ) {
                   ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
code is:
            if ( nonInvolRejectCount > 0 &&
(...||           (lowestOrder < (curOrder = permOrder(h)) ||
 ...&&...)       lowestOrder == curOrder && levelLowestOrder > finalLevel) ) {

---
  Another case of suspicious code:
./src/ccent.c: In function 'nextBasePointEltCent':
./src/ccent.c:90:24: warning: suggest parentheses around '+' inside '<<' [-Wparentheses]
             priority = 2000000000ul - (unsigned long) MIN(cycleLen[pt],1000) << 20
                                                                                 ~~
                        + cSize;
                        ^~~~~~~

---
  The package appears in good shape, but while I checked all other
compiler warnings, and they appear to just warn about improper
indentation or somewhat uncommon coding practive, the above warnings
may be valid, so, I would like some comment about it.

Comment 2 Jerry James 2016-08-17 04:17:48 UTC
Thanks for the review, Paulo.

(In reply to Paulo Andrade from comment #1)
>   It is not required to have:
> BuildRequires: gcc

Actually, it is now.  See:
https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B#BuildRequires_and_Requires
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/XGXCCAJNEOIEN3KK6TN2657LSIGZGB3N/

>   This looks suspecting, as it means it will loop only once:
> ./src/randschr.c:238:10: warning: this 'if' clause does not guard...
> [-Wmisleading-indentation]

Good catch!  I will add braces to the warning patch to fix that.

>   || and && have different precedence, so this code is also suspecting:
> ./src/randschr.c:391:43: warning: suggest parentheses around '&&' within
> '||' [-Wparentheses]
>                    lowestOrder == curOrder && levelLowestOrder > finalLevel)
> ) {
>                    ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

That one I think is okay.  It appears to me that the && really should be within ||, so it is parsed in the correct order.  But ... it's easy to patch, so I will add parentheses there.

>   Another case of suspicious code:
> ./src/ccent.c: In function 'nextBasePointEltCent':
> ./src/ccent.c:90:24: warning: suggest parentheses around '+' inside '<<'
> [-Wparentheses]
>              priority = 2000000000ul - (unsigned long)
> MIN(cycleLen[pt],1000) << 20
>                                                                             
> ~~
>                         + cSize;
>                         ^~~~~~~

This one worries me.  I'm not sure if the + is supposed to be inside or outside of the <<, so I am reluctant to touch it.  I think I'd better check with upstream before I do anything here, unless you think you can see where the parentheses should go.

Comment 3 Paulo Andrade 2016-08-17 14:39:56 UTC
  I think the last one is likely a bug, because the "+ cSize"
is added in the next line, but (adding parenthesis,) what is
happening is:

priority = (2000000000ul - (unsigned long)MIN(cycleLen[pt],1000)) << (20 + cSize);

  So, it might have two bugs.
  The smaller the value of cycleLen[pt] and cSize, the more
likely the code does what is intended, as it appears to be
some kind of heuristic priority choosing.

Comment 4 Jerry James 2016-08-19 16:05:38 UTC
I added those parentheses.  I also noticed that guava has a software popcount implementation, so I added another patch to use a CPU popcount instruction instead if one is available.  New URLs:

Spec URL: https://jjames.fedorapeople.org/gap-pkg-guava/gap-pkg-guava.spec
SRPM URL: https://jjames.fedorapeople.org/gap-pkg-guava/gap-pkg-guava-3.13.1-2.fc26.src.rpm

Comment 5 Paulo Andrade 2016-08-27 02:00:39 UTC
Looks like the parallel command is broken in rawhide:
$ head -1 /usr/bin/parallel 
##!/usr/bin/perl
note the two '#'.
Fixing it manually and building the package I do not see
any other issue.
You might want to either check what is broken with the
parallel package, or not use it... See the sed command in
http://pkgs.fedoraproject.org/cgit/rpms/parallel.git/commit/?id=b941a86552fa42ca018a58bffc3ba35c428b0d07
(I just fedpkg co'd parallel and it is indeed broken).

  Otherwise, the package is now approved. All the issues
have been fixed!

Comment 6 Gwyn Ciesla 2016-08-29 12:45:39 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/gap-pkg-guava

Comment 7 Fedora Update System 2016-08-30 04:03:46 UTC
gap-pkg-guava-3.13.1-2.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-bcd5fe40a8

Comment 8 Fedora Update System 2016-08-30 04:03:54 UTC
gap-pkg-guava-3.13.1-2.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-2547786f05

Comment 9 Fedora Update System 2016-08-31 03:52:16 UTC
gap-pkg-guava-3.13.1-2.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-bcd5fe40a8

Comment 10 Fedora Update System 2016-08-31 12:57:29 UTC
gap-pkg-guava-3.13.1-2.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-2547786f05

Comment 11 Fedora Update System 2016-09-04 17:39:54 UTC
gap-pkg-guava-3.13.1-2.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 12 Fedora Update System 2016-09-09 16:55:21 UTC
gap-pkg-guava-3.13.1-2.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.


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