Bug 701801 - Review Request: ast - A Library for Handling World Coordinate Systems in Astronomy
Summary: Review Request: ast - A Library for Handling World Coordinate Systems in Astr...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Sergio Pascual
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-05-03 22:00 UTC by Orion Poplawski
Modified: 2011-12-05 21:47 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-12-05 21:47:31 UTC
Type: ---
Embargoed:
sergio.pasra: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Orion Poplawski 2011-05-03 22:00:44 UTC
Spec URL: http://www.cora.nwra.com/~orion/fedora/ast.spec
SRPM URL: http://www.cora.nwra.com/~orion/fedora/
Description:The AST library provides a comprehensive range of facilities for attaching
world coordinate systems to astronomical data, for retrieving and interpreting
that information and for generating graphical output based on it. It's main
selling points are:

* Ease of use.
* Facilities for generating plots of generalized non-linear, potentially
  discontinuous 2-D or 3-D coordinate systems, with detailed control of the
  appearance of the plot.
* Facilities for converting transparently between different coordinate
  systems, including a wide range of celestial, spectral and time coordinate
  systems.
* Facilities for searching a general collection of connected coordinate
  systems for a coordinate system with any given set of characteristics.
* Allows code for handling WCS information to be written in a general way
  without regard to the specific nature of the coordinate systems being
  handled (i.e. whether they represent sky positions, spectral positions,
  focal plane positions, pixel positions, etc).
* Flexible system for saving and retrieving WCS information, including (but
  not limited to) a range of different popular FITS descriptions.
* Written in C but has interfaces for C, Fortran, Java (via JNI), Perl, and
  UNIX shell.
* Extensive documentation. 

AST is different to other popular FITS-based WCS packages in that it takes a
very high level, generalized "object oriented" view of the problems of
describing, using and storing WCS information. For this reason, it is very
easy to use - it wraps up all the complications of dealing with the specifics
of different classes of coordinate systems (celestial, spectral, pixel, etc).
Another reason it is easy to use is that it hides completely the sometimes
elaborate details of FITS WCS handling, and uses a completely FITS-independent
and much cleaner interface more closely associated with the general nature of
world coordinate information. However, it is possible to go both ways between
AST and FITS (albeit the AST representations are much richer than the FITS and
so some information may be lost in going from AST to FITS).

All coordinate systems are described by "Frame" objects which encapsulate
information about the nature of the coordinate system - axis labels, units,
how to convert axis values to and from text strings, how to "measure distance"
within the Frame (the Frame metric), etc, etc. There are then subclasses of
Frame which "know" about more specific coordinate systems - at the moment the
three main subclasses are SkyFrame (which describes celestial positions on the
sky), SpecFrame (which describes positions within a spectrum) and TimeFrame
(which describes moments in time). All these classes can describe positions
within their respective domains using all the common coordinate systems
(various equatorial, ecliptic, galactic, etc for sky positions, wavelength
frequency, various velocities, etc, for spectral, different timescales, etc,
for time). They also know how to transform axis values between these systems.
There is also a "compound Frame" class which allows any two Frames (of any
class) to be joined together (for instance, you could compound a SkyFrame and
a SpecFrame to describe a spectral cube). These various forms of Frame can be
connected together using "Mappings". A Mapping is a mathematical recipe for
transforming a position within one coordinate system into another. There are
subclasses of Mapping which implement a wide range of different transformation.
These include:

* all the celestial projections and spectral algorithms described in FITS-WCS
  papers II & III (currently excluding the "-TAB" algorithm),
* classes which allows the caller to define their own transformations, either
  by providing a routine written in C or Fortran, or by providing a general
  algebraic FORTRAN-like expression supplied in a text string.
* classes for scaling, shifting, pin cushion distortion, spherical to
  Cartesian transformation, axis permutations, look-up table transformations,
  etc, etc.
* a class which allows any two other Mappings to be combined together either
  in series or in parallel, to form a "compound Mapping". This allows Mappings
  of arbitrary complexity to be constructed using the other Mappings as
  components. 

A "FrameSet" is a collection of Frames connected together by Mappings. A
FrameSet can be created from a FITS header (for instance) using a single call.
The FrameSet can then be used to transform positions between nominated
coordinate systems, to convert axis values to and from text strings using
formats automatically chosen to be appropriate to the class of Frame, to plot
coordinate grids (using any graphics system you like), etc. FrameSets can be
searched for Frames with particular characteristics.


There are a large number of rpmlint warnings like:
ast.i686: W: undefined-non-weak-symbol /usr/lib/libast.so.0.0.0 astAZPfwd

This is caused by a rather strange library configuration.  However, this is expected and is worked around by using the ast_link command.

Comment 1 Sergio Pascual 2011-05-11 22:19:13 UTC
Hi, some comments:

 * The description is too long. I haven't found a guideline about this but IMHO with less than 10 lines is enough. Consider that 
$ rpm -qi ast 
outputs almost two screens full of text, hiding the rpm information.

 * The upstream version of the package is 5.6-0. What do you thonk of translating this to 5.6.0-1 instead of 5.6-1? If upstream releases 5.6-3 you will have to edit the Source macro to get the correct source. (Weird versioning, by the way)

 * Source should contain a full URL

 * Everything inside /usr/share/ast is documentation and is not needed to work with the libraries. As such, I think the contents should go to %docs
 
 * Furthermore, the docs are about 40 M in size. Removing the .tex files reduces the size to around 25 M. These files are good candidates to go to a ast-doc package. If you don't want to make a separate doc package, the library documentation should be in -devel subpackage.

When I have more time, I will see if the Makefile can be patched to "remove the unresolved symbol" warnings.

Comment 2 Orion Poplawski 2011-10-14 18:04:34 UTC
* Fri Oct 14 2011 Orion Poplawski <orion.com> 5.7.2-1
- Update to 5.7-2
- Truncate description
- Move documentation to subpackage

I'm not sure how to get a usable URL.  I put it in as a comment in the spec.

http://www.cora.nwra.com/~orion/fedora/ast.spec
http://www.cora.nwra.com/~orion/fedora/ast-5.7.2-1.fc15.src.rpm

Comment 3 Orion Poplawski 2011-11-22 20:58:21 UTC
Sergio - Are you still up for doing this review, or should I put it back in the queue?

Comment 4 Sergio Pascual 2011-11-24 12:15:44 UTC
I would like to do the review this weekend... I've been very busy these months

Comment 5 Sergio Pascual 2011-11-28 10:02:23 UTC
These are blockers, after fixing them the package should be ok.

License
------- 
* There are some files proj.c proj.h wcsmath.h wcstrig.c wcstrig.h (from an old version of wcslib) under LGPLv2+. So the license tag should be: GPLv2+ and LGPLv2+
* The FSF address is wrong. You should report it upstream 

Libraries
---------
* In the installed package there are some pgplot related libraries. As pgplot is not free and can't be distributed by Fedora, I suggest to remove the libraries

/usr/lib64/libast_pgplot3d.so
/usr/lib64/libast_pgplot3d.so.0
/usr/lib64/libast_pgplot3d.so.0.0.0
/usr/lib64/libast_pgplot.so
/usr/lib64/libast_pgplot.so.0
/usr/lib64/libast_pgplot.so.0.0.0

Documentation
------------- 
* rpmlint complains about hidden directories and empty files. They should be removed.
   
ast-doc.x86_64: W: hidden-file-or-dir /usr/share/doc/ast/sun210.htx/.star2html-init
ast-doc.x86_64: E: zero-length /usr/share/doc/ast/sun210.htx/star2html.sty
ast-doc.x86_64: E: zero-length /usr/share/doc/ast/sun211.htx/star2html.sty
ast-doc.x86_64: W: hidden-file-or-dir /usr/share/doc/ast/sun211.htx/.star2html-init

* Tex files are included. They should be removed unless they are needed for something.

* In the spec, you create a /usr/share/doc/ast directory and move there the documentation installed. But the name of the directory is incorrect. Everything in docdir is named "package-version", in this case ast-doc-5.7.2

I would let rpm do its job and grab the docs from the source dir

%files doc
%doc sun210.htx sun210.ps sun211.htx sun211.ps

Other recommendations, not blockers

* The following macros are not needed anymore, so you can remove them safely from the specfile:
BuildRoot, %clean and %defattr

* If you are willing and have time, you can try to educate upstream about:
 - its weird versioning scheme
 - linking to wcstools instead of pasting wcstools files in the code
 - using pkgconfig instead of home-made solutions
 - creating pgplot libraries only if pgplot is present

Comment 6 Orion Poplawski 2011-11-28 23:20:28 UTC
http://www.cora.nwra.com/~orion/fedora/ast.spec
http://www.cora.nwra.com/~orion/fedora/ast-6.0.1-1.fc16.src.rpm

* Mon Nov 28 2011 Orion Poplawski <orion.com> 6.0.1-1
- Update to 6.0-1
- Fixup some lib linkages
- Fix license tag
- Fix FSF license
- Fixup doc install
- Drop BuildRoot, clean, defattr

I've sent an email upstream about the various issues.

Comment 7 Sergio Pascual 2011-12-03 16:19:51 UTC
Rpmlint output:
ast.src:12: W: macro-in-comment %{srcver}
ast.src: W: invalid-url Source0: ast-6.0-1.tar.gz
ast-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/ast-6.0-1/loader.h
ast-devel.x86_64: W: spelling-error %description -l en_US prog -> prig, prof, pro
ast-devel.x86_64: W: no-documentation
ast-devel.x86_64: W: no-manual-page-for-binary ast_link

Package and spec are named according the guidelines

License is GPLv2+ and  LGPLv2+

Specfile legible
Source matches upstream, source follows the convention for Troublesome URLs (see http://fedoraproject.org/wiki/Packaging/SourceURL)
Package builds, No ExcludeArch needed
BuildRequires listed
ldconfig called properly
No bundled libraries (the files from wcslib have been modified by ast developers, so I don't consider it to be the same library.)

Owns directories it creates
Macros are consistent
Large docs are in -doc subpackage
Headers and .so are in -devel subpackage
-devel requires base package
No .la files

BuildRoot is not needed
%clean is not needed


Package is APPROVED

By the way, you are not required to fix the incorrect FSF address. But if you do, please fix also loader.h before uploading

Comment 8 Orion Poplawski 2011-12-05 21:11:24 UTC
Thanks for the review.

New Package SCM Request
=======================
Package Name: ast
Short Description: A Library for Handling World Coordinate Systems in Astronomy
Owners: orion
Branches: f15 f16 el6
InitialCC:

Comment 9 Gwyn Ciesla 2011-12-05 21:19:05 UTC
Git done (by process-git-requests).

Comment 10 Orion Poplawski 2011-12-05 21:47:31 UTC
Checked in and built.  Thanks Jon.


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