Bug 568053 (4ti2) - Review Request: 4ti2 - A software package for problems on linear spaces
Summary: Review Request: 4ti2 - A software package for problems on linear spaces
Keywords:
Status: CLOSED NEXTRELEASE
Alias: 4ti2
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Terje Røsten
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-02-24 17:12 UTC by Mark Chappell
Modified: 2010-03-05 12:43 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-03-05 12:43:42 UTC
terjeros: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Mark Chappell 2010-02-24 17:12:07 UTC
Spec URL: http://tremble.fedorapeople.org/packages/4ti2.spec
SRPM URL: http://tremble.fedorapeople.org/packages/4ti2-1.3.2-1.fc12.src.rpm
Description: 
A software package for algebraic, geometric and combinatorial
problems on linear spaces.

Comment 1 Terje Røsten 2010-02-25 15:38:45 UTC
I am a bit puzzled here, did you start with --enable-shared and ended up
with --disable-shared? 

The are no files in devel. Is this software used as shared libs by other projects?

Include some docs, e.g. http://www.4ti2.de/4ti2_manual.pdf

Fix mixing in macro style.

Comment 2 Mark Chappell 2010-02-25 15:43:22 UTC
(In reply to comment #1)
> I am a bit puzzled here, did you start with --enable-shared and ended up
> with --disable-shared? 
Yes, however the "libraries" it creates, are rather blatantly not for use by any other project: calls to exit, etc.

> The are no files in devel. Is this software used as shared libs by other
> projects?
No, I forgot to remove the devel package after removing them.

> Include some docs, e.g. http://www.4ti2.de/4ti2_manual.pdf

Will do.
 
> Fix mixing in macro style.    

Will do.


Thank you for your time.

Comment 3 Mark Chappell 2010-02-25 16:22:40 UTC
SRPM URL: http://tremble.fedorapeople.org/packages/4ti2-1.3.2-2.fc12.src.rpm

- Tidy up inconsistent use of macros
- Remove empty devel package
- Include 4ti2 manual

Comment 4 Terje Røsten 2010-02-25 17:04:39 UTC
%setup -q

cp %{SOURCE1} .

Add -p to preserve timestamps and remove empty line.

# Compile
make %{?_smp_mflags}

%install

rm -rf %{buildroot}

Remove compile comment and empty line between %install and rm ...


rm -rf %{buildroot}%{_libdir}/*.la
rm -rf %{buildroot}%{_libdir}/*.a

Is these needed?

Remove these:

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig


%doc COPYING TODO
%doc 4ti2_manual.pdf

all docs on one line?

%{_bindir}/*

Please be more explicit here, much simpler to detect changes.

Update linked spec file when posting changes.

Comment 5 Mark Chappell 2010-02-25 17:32:55 UTC
SRPM URL: http://tremble.fedorapeople.org/packages/4ti2-1.3.2-3.fc12.src.rpm
SPEC URL: http://tremble.fedorapeople.org/packages/4ti2.spec



> Add -p to preserve timestamps and remove empty line.

Done


> Remove compile comment and empty line between %install and rm ...

Done


> rm -rf %{buildroot}%{_libdir}/*.la
> rm -rf %{buildroot}%{_libdir}/*.a
> 
> Is these needed?

Unfortunately, yes.  It builds static libraries, then links its self against them.  As it neither distributes the static libraries, nor links staticly against any system libraries I believe this is still acceptable.
 
> Remove these:

Done

> all docs on one line?

Done
 
> %{_bindir}/*
> Please be more explicit here, much simpler to detect changes.

Done
 
> Update linked spec file when posting changes.    

Apologies, done this time

Comment 6 Terje Røsten 2010-02-25 18:53:01 UTC
 ok rpmlint
 ok naming
 ok spec
 -  license, looks like GPLv2+ to me, why are you using LGPLv2?
 ok lang
 ok sha1sum
   bb0bddb3a9de6f6ceeee7e296b5315b8b7e59ea5  4ti2-1.3.2.tar.gz
   bb0bddb3a9de6f6ceeee7e296b5315b8b7e59ea5  4ti2-1.3.2.tar.gz.1
   62d93d38f7f95c8fb6711ca5c33597815f55cf45  4ti2_manual.pdf
   62d93d38f7f95c8fb6711ca5c33597815f55cf45  4ti2_manual.pdf.1
 ok dirs
 ok perms

pedantic stuff:

 - empty lines in %build and %install

Over all with changed license this should be ok.

However, there a serious issue about the generic names:
  
/usr/bin/circuits
/usr/bin/genmodel
/usr/bin/gensymm
/usr/bin/graver
/usr/bin/groebner
/usr/bin/hilbert
/usr/bin/markov
/usr/bin/minimize
/usr/bin/normalform
/usr/bin/qsolve
/usr/bin/rays
/usr/bin/walk

Can prefix all with 4ti2, but then the documentation would be wrong.

(See the environment-modules package)

Or maybe it makes sense to use the modules systems to manage this?

$ module load 4ti2

(See the environment-modules package)

Would bring in all the names, but only for the user wanting this.

Is there any policy on modules usage?

Comment 7 Mark Chappell 2010-03-02 10:18:50 UTC
(In reply to comment #6)
>  ok spec
>  -  license, looks like GPLv2+ to me, why are you using LGPLv2?

Copy and paste error.

>  - empty lines in %build and %install

Gone

> However, there a serious issue about the generic names:
>
> Or maybe it makes sense to use the modules systems to manage this?
> 
> $ module load 4ti2

A much better way of avoiding name space pollution.

In line with openmpi and mpich2, I've moved the binaries into %{_libdir}/%{name}/bin and created a module file


Mark

Comment 9 Terje Røsten 2010-03-02 16:25:11 UTC
Good, 

at least include information in %desc or in a README.fedora or similar
how to "load" the binaries.

You could also include a 4ti2-load command in /usr/bin to do the module load stuff (if possible).

Comment 10 Mark Chappell 2010-03-02 16:41:54 UTC
> at least include information in %desc or in a README.fedora or similar
> how to "load" the binaries.

How's:

This package uses Environment Modules, to load the binaries onto
your PATH you will need to run module load %{name}-%{_arch}

> You could also include a 4ti2-load command in /usr/bin to do the module load
> stuff (if possible).    

It's possible, however would be out of step with the usual use of modules.  I will however provide one should you feel this is required.

Comment 11 Terje Røsten 2010-03-02 17:06:34 UTC
> How's:
> 
> This package uses Environment Modules, to load the binaries onto
> your PATH you will need to run module load %{name}-%{_arch}


Nice, let's do that.

Comment 13 Terje Røsten 2010-03-02 18:11:55 UTC
Thanks, can't find anything more to worry about.


 The package 4ti2 is APPROVED.

Comment 14 Mark Chappell 2010-03-02 19:47:06 UTC
New Package CVS Request
=======================
Package Name: 4ti2
Short Description: A software package for problems on linear spaces
Owners: tremble
Branches: F-12 F-13 EL-5

Comment 15 Jason Tibbitts 2010-03-02 22:52:57 UTC
CVS done (by process-cvs-requests.py).


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