Bug 723756 - Review Request: bliss - Compute automorphism groups and canonical labelings of graphs
Summary: Review Request: bliss - Compute automorphism groups and canonical labelings o...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Gwyn Ciesla
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-07-21 03:16 UTC by Jerry James
Modified: 2011-11-25 23:27 UTC (History)
3 users (show)

Fixed In Version: bliss-0.72-2.fc16
Clone Of:
Environment:
Last Closed: 2011-11-25 23:27:52 UTC
Type: ---
Embargoed:
gwync: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Jerry James 2011-07-21 03:16:05 UTC
Spec URL: http://jjames.fedorapeople.org/bliss/bliss.spec
SRPM URL: http://jjames.fedorapeople.org/bliss/bliss-0.72-1.fc15.src.rpm
Description: Bliss is an open source tool for computing automorphism groups and canonical forms of graphs.  It has both a command line user interface as well as C++ and C programming language APIs.

Comment 1 Gwyn Ciesla 2011-07-25 15:05:11 UTC
Good:

- rpmlint checks return:

A few trivial and/or erroneous spelling issues and:

bliss-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libbliss.so.0.72 exit.5
This library package calls exit() or _exit(), probably in a non-fork()
context. Doing so from a library is strongly discouraged - when a library
function calls exit(), it prevents the calling program from handling the
error, reporting it to the user, closing files properly, and cleaning up any
state that the program has. It is preferred for the library to return an
actual error code and let the calling program decide how to handle the
situation.

This would be reaaaally good to fix, or at least nag upstream about.

- package meets naming guidelines
- package meets packaging guidelines
- license ( GPLv3 ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

EXTRA STUFF FOR PACKAGES WITH DEVEL
==========================

- devel package ok
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r 

So, looks good, generally.  I'm running a mock build to double-check BRs, and I'm curious on your views vis-a-vis exit().

Comment 2 Gwyn Ciesla 2011-07-25 15:21:26 UTC
Mock build was good, so the BRs are as well.

Comment 3 Jerry James 2011-07-25 19:59:59 UTC
Thanks for the review, Jon.

(In reply to comment #1)
> So, looks good, generally.  I'm running a mock build to double-check BRs, and
> I'm curious on your views vis-a-vis exit().

I agree.  I hate it when library authors call exit().  I looked in the source code to see why this library does so.  There is one call to exit(), in a function named fatal_error().  That function is called in the following situations:
- a memory allocation failed
- an internal assert()-like function failed
- heap corruption was detected

I'll talk to the upstream author about returning appropriate error codes instead of calling exit() in these situations.

Comment 4 Gwyn Ciesla 2011-07-26 12:37:28 UTC
Excellent.  I don't know how responsive this particular upstream is, would you rather wait or patch?

Comment 5 Jerry James 2011-07-26 14:37:49 UTC
This is C++ code, so all we really need to do is throw an exception instead of calling exit.  Of course, selecting *which* exception to throw may require some thought...

I'll work up a patch sometime today, and run it by upstream.

Comment 6 Gwyn Ciesla 2011-07-26 14:50:50 UTC
Cool, keep me posted.

Comment 7 Jerry James 2011-11-15 20:02:11 UTC
Well, that hardly took any time at all. :-)  Neither upstream nor I was particularly fast about responding to each other, but here we finally have an approach that it looks like upstream may adopt.  Upstream is also enthusiastic about my plan to use bliss as a replacement for the unpackageable nauty.  New URLs:

Spec URL: http://jjames.fedorapeople.org/bliss/bliss.spec
SRPM URL: http://jjames.fedorapeople.org/bliss/bliss-0.72-2.fc16.src.rpm

Comment 8 Gwyn Ciesla 2011-11-15 20:22:50 UTC
Excellent!  All better!

APPROVED.

Comment 9 Jerry James 2011-11-15 20:58:51 UTC
Thanks for the review, Jon.

New Package SCM Request
=======================
Package Name: bliss
Short Description: Compute automorphism groups and canonical labelings of graphs
Owners: jjames
Branches: f16
InitialCC:

Comment 10 Gwyn Ciesla 2011-11-15 21:05:50 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2011-11-15 21:47:13 UTC
bliss-0.72-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/bliss-0.72-2.fc16

Comment 12 Fedora Update System 2011-11-17 23:27:12 UTC
bliss-0.72-2.fc16 has been pushed to the Fedora 16 testing repository.

Comment 13 Fedora Update System 2011-11-25 23:27:52 UTC
bliss-0.72-2.fc16 has been pushed to the Fedora 16 stable repository.


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