Bug 466655 - Review Request: libfplll - LLL-reduces euclidian lattices
Summary: Review Request: libfplll - LLL-reduces euclidian lattices
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-10-12 09:39 UTC by Conrad Meyer
Modified: 2008-10-23 06:54 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-10-23 06:54:36 UTC
Type: ---
Embargoed:
j: fedora-review+
huzaifas: fedora-cvs+


Attachments (Terms of Use)

Description Conrad Meyer 2008-10-12 09:39:33 UTC
Spec URL: http://konradm.fedorapeople.org/fedora/SPECS/libfplll.spec
SRPM URL: http://konradm.fedorapeople.org/fedora/SRPMS/libfplll-3.0.9-1.fc9.src.rpm
Description:
fpLLL-3.0 contains several algorithms on lattices that rely on
floating-point computations. This includes implementations of the
floating-point LLL reduction algorithm, offering different
speed/guarantees ratios. It contains a 'wrapper' choosing the
estimated best sequence of variants in order to provide a guaranteed
output as fast as possible. In the case of the wrapper, the
succession of variants is oblivious to the user. It also includes
a rigorous floating-point implementation of the Kannan-Fincke-Pohst
algorithm that finds a shortest non-zero lattice vector.

Comment 1 Conrad Meyer 2008-10-12 09:44:25 UTC
Builds in koji [0]; rpmlint is silent.

[0]: http://koji.fedoraproject.org/koji/taskinfo?taskID=875176

Comment 2 Jason Tibbitts 2008-10-17 21:14:43 UTC
Indeed, this builds fine.  I get one rpmlint complaint:
  libfplll-devel.x86_64: W: no-documentation
which is indeed correct but also not an issue.

Normally I'd complain about obscure acronyms which aren't explained in the package description, but I'm not sure that "Lenstra-Lenstra-Lovascz" is any clearer than "LLL".  I will point out that the use of "oblivious" in the description doesn't really make any sense because a "succession of variants" can't have conscious awareness in the first place.  I'm not sure what they really mean.  Maybe "immaterial".  But that's just nitpicking.

There's a test suite; you should call it (with "make check" in a %check section).  I'm not sure how to interpret the results but a quick read of the code indicates that calls to llldiff should produce no output if there are no problems, and that seems to be the case in my tests.

The "generate" command is incredibly generic; I don't think this package can be approved with a binary of that name.  fplll-generate would make sense.

The header files install directory into /usr/include with very generic names (/usr/include/defs.h, for example).  These will need to be either renamed or moved into a subdirectory.

* source files match upstream:
   04f630a4d939f4fc1c721c57c921a2e940efb8b315adca6f994192220326aeb7  
   libfplll-3.0.9.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
  libfplll-3.0.9-1.fc10.x86_64.rpm
   libfplll.so.0()(64bit)
   libfplll = 3.0.9-1.fc10
   libfplll(x86-64) = 3.0.9-1.fc10
  =
   /sbin/ldconfig
   libfplll.so.0()(64bit)
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libgcc_s.so.1(GCC_4.0.0)(64bit)
   libgmp.so.3()(64bit)
   libmpfr.so.1()(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)
   libstdc++.so.6(GLIBCXX_3.4.9)(64bit)

  libfplll-devel-3.0.9-1.fc10.x86_64.rpm
   libfplll-devel = 3.0.9-1.fc10
   libfplll-devel(x86-64) = 3.0.9-1.fc10
  =
   libfplll = 3.0.9-1.fc10
   libfplll.so.0()(64bit)

X %check is not present, but should be.
* shared libraries installed; ldconfig called properly.
* unversioned .so files are in the -devel package.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
X generically named files.
* scriptlets OK (ldconfig).
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.

The package review process needs reviewers!  If you haven't done any package
reviews recently, please consider doing one.

Comment 3 Conrad Meyer 2008-10-17 21:44:12 UTC
(In reply to comment #2)
> ...
>
> The package review process needs reviewers!  If you haven't done any package
> reviews recently, please consider doing one.

I think maybe I'm doing something wrong w.r.t. reviews -- it seems to take quite a long time (a substantial portion of an hour for the first pass through the list of requirements) and I miss basic things that aren't mentioned in the packaging guidelines. I eagerly look forward to the review-o-matic helper mentioned on fedora-devel-list.

All the same I very much appreciate your feedback! New URLs:
http://konradm.fedorapeople.org/fedora/SPECS/libfplll.spec
http://konradm.fedorapeople.org/fedora/SRPMS/libfplll-3.0.9-2.fc9.src.rpm

Comment 4 Jason Tibbitts 2008-10-21 02:56:53 UTC
Looks good; %check section is present and passes; generically-named binary and headers have been renamed or moved into their own directory.  The only minor thing I can point out is that it looks perhaps a bit odd to have fplll_micro and fplll_verbose, but fplll-generate (underscores versus a dash).  It's minor, so feel free to import what you like but of course it would be tough to change after import.

If there are basic things not mentioned in the packaging guidelines, please feel free to point them out so that the packaging committee can address them.  Unfortunately the review-o-matic isn't going to be able to catch anything more than trivial issues, and you could get the same just by doing scratch builds and running rpmlint on the result.

Anyway, this package looks fine to me.

APPROVED

Comment 5 Conrad Meyer 2008-10-21 04:02:42 UTC
I agree with fixing the discrepancy between the underscores and the hyphen. Thanks!

New Package CVS Request
=======================
Package Name: libfplll
Short Description: LLL-reduces euclidian lattices
Owners: konradm
Branches: F-10 F-9
InitialCC:

Comment 6 Huzaifa S. Sidhpurwala 2008-10-22 10:33:11 UTC
cvs done

Comment 7 Conrad Meyer 2008-10-23 06:54:36 UTC
Built in rawhide, closing.


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