Bug 178263 - Review Request: ncarg - A Fortran and C based software package for scientific visualization
Summary: Review Request: ncarg - A Fortran and C based software package for scientific...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ed Hill
QA Contact: David Lawrence
URL: http://www.cora.nwra.com/~orion/fedora/
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-01-18 22:15 UTC by Orion Poplawski
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-02-06 18:45:33 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Orion Poplawski 2006-01-18 22:15:50 UTC
Spec Name or Url: http://www.cora.nwra.com/~orion/fedora/ncarg.spec
SRPM Name or Url: http://www.cora.nwra.com/~orion/fedora/ncarg-4.4.1-1.src.rpm
Description:
 
NCAR Graphics is a Fortran and C based software package for scientific
visualization.

Comment 1 Orion Poplawski 2006-01-18 23:01:34 UTC
Ed - 

  I posted this just a bit too soon and it doesn't build yet.  Note that this
only builds on devel due to a bug fixed in gfortran-4.1.0 but not it 4.0.1. 
I'll post an update when I've got a working version.

Comment 2 Ed Hill 2006-01-18 23:39:07 UTC
Hi Orion, no worries!  I'll review when you say its ready.

Comment 3 Deji Akingunola 2006-01-19 00:13:15 UTC
Hey orion,

>   I posted this just a bit too soon and it doesn't build yet.  Note that this
> only builds on devel due to a bug fixed in gfortran-4.1.0 but not it 4.0.1. 
> I'll post an update when I've got a working version.

Why not just get it in for devel tree only at this tree. If the build issues are
really due to bugs in the compiler, those gfortran fixes may never get
backported to 4.0.1; unless I don't quite get what you mean.

Comment 4 Chris Chabot 2006-01-19 00:31:28 UTC
A usefull speci file trick is the %exclude tag, with it you can probably
simplify your %files section down to:

%files
%defattr(-,root,root,-)
%doc COPYING Copyright README
%{_sysconfdir}/profile.d/ncarg.*sh
%{_bindir}/*
%{_libdir}/ncarg
%{_mandir}/man*/*.gz
%exclude %{_bindir}/ncargcc
%exclude %{_bindir}/ncargf90
%exclude %{_libdir}/*.a
%exclude %{_libdir}/ncarg/examples
%exclude %{_libdir}/ncarg/tests

%files devel
%{_bindir}/ncargcc
%{_bindir}/ncargf90
%{_includedir}/ncarg
%{_libdir}/*.a
%{_libdir}/ncarg/examples
%{_libdir}/ncarg/tests

Saves a few pages of listing binaries and takes care of all directory ownership
etc properly too


Comment 5 Orion Poplawski 2006-01-19 17:22:40 UTC
Okay, please download again and we'll start from there.  Subsequent versions
will get their revision updated.  Probably still need to shuffle files around.

Comment 6 Ed Hill 2006-01-29 21:12:58 UTC
Hi Orion, I don't have an FC5 system running at the moment to test
whether the ncarg binaries segfault or not.  So I'll have to skip that
part of the review for now.  In any case, here are the parts that I
could do:

good:
 + source matches upstream
 + specfile is legible
 + license is ok and correctly included
 + builds in mock FC5 i386
 + code not content -- although it apparently contains some basic 
     map, font (?), etc. data for creating the graphics
 + dir ownership and permissions looks OK
 + no shared libs and no *.la files

needswork:
 - please add an "unset NCARG" at the end of %build
 - please consider using Chris Chabot's suggestion in comment #4
 - rpmlint emits a few warnings/errors:

     OK to ignore:
     E: ncarg script-without-shellbang /etc/profile.d/ncarg.csh
     E: ncarg script-without-shellbang /etc/profile.d/ncarg.sh
     W: ncarg-devel no-documentation

     Please put all the tutorial bits (/usr/lib/ncarg/tutorial/)
     in the -devel package or perhaps in a -doc package:
     W: ncarg devel-file-in-non-devel-package 
        /usr/lib/ncarg/tutorial/c_colcon.c

     Probably, this should go in the devel package since I don't 
     think it gets used via any run-time (eg. plug-in) methods. 
     Or does it?
     W: ncarg devel-file-in-non-devel-package 
        /usr/lib/ncarg/graphcaps/aed.a

 - please put all the /usr/lib/lib*.a files in /usr/lib/ncarg/
   so that they don't pollute the /usr/lib/ namespace with 
   generic names
 - please have the devel package require the main package:
     "Requires: %{name} = %{version}-%{release}"



Comment 7 Wart 2006-01-29 21:51:29 UTC
(In reply to comment #6)
> Hi Orion, I don't have an FC5 system running at the moment to test
> whether the ncarg binaries segfault or not.

I was able to verify that some of the examples from ncargex in X11 mode (-W 8)
run on FC5 without crashing.

> needswork:
>  - please add an "unset NCARG" at the end of %build

Why bother?  The %build and %install scripts are run as separate shell scripts,
so environment settings don't carry from one to the other AFAIK.

>      Please put all the tutorial bits (/usr/lib/ncarg/tutorial/)
>      in the -devel package or perhaps in a -doc package:

On a related note, it appears that ncargex, in the base package, requires the
example files in the ncarg-devel package.  Perhaps the ncargex package could be
moved to the -devel package, or as Ed suggests, move it all to a -doc package.

Comment 8 Orion Poplawski 2006-02-02 16:32:23 UTC
Thanks for the reviews:

- I prefer to list binaries explicitly - it has alerted me to problems before.
- Moved tutorial to -devel
- Moved .a to %{_libdir}/ncarg/ (and other stuff to %{_libdir}/ncarg/ncarg/
- Added devel Requires.
- Moveg ncargex to -devel
- aed.a is a text graphcad file, not a library.

http://www.cora.nwra.com/~orion/fedora/ncarg-4.4.1-2.src.rpm

Comment 9 Ed Hill 2006-02-03 02:35:30 UTC
Hi Orion, you've addressed all the comments and I just got a clean build 
in mock (fc5, i386) with the 4.4.1-2 SRPM.  Good!  And thanks to Wart 
(#7 above) for checking that the binaries don't segfault.

APPROVED.

Comment 10 Orion Poplawski 2006-02-06 18:45:33 UTC
Checked in and built on devel.


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