Bug 468517 - Review Request: saoimage - Utility for displaying astronomical images
Review Request: saoimage - Utility for displaying astronomical images
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Marek Mahut
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-25 10:31 EDT by Lubomir Rintel
Modified: 2008-11-03 18:13 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-11-03 18:13:31 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mmahut: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)
Comment for Patch2 (1.88 KB, text/plain)
2008-10-25 13:56 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Description Lubomir Rintel 2008-10-25 10:31:37 EDT
SPEC: http://netbsd.sk/~lkundrak/SPECS/saoimage.spec
SRPM: http://netbsd.sk/~lkundrak/SRPMS/saoimage-1.35.1-1.el5.src.rpm

Description:

SAOimage (pronounced S-A-0-image) is a utility for displaying astronomical
images in the X11 window environment. It was written at the Smithsonian
Astrophysical Observatory by Mike Van Hilst in 1990 and is now maintained
by Doug Mink.

Image files can be read directly, or image data may be passed through a
named pipe (Unix) or a mailbox (VMS) from IRAF display tasks. SAOimage
provides a large selection of options for zooming, panning, scaling,
coloring, pixel readback, display blinking, and region specification.
User interactions are generally performed with the mouse. Mouse tracking in
an image's world coordinate system, usually sky coordinates, was added in
1994. You can also plot catalogs over images with WCS information in their
headers.

The SAOimage desktop includes, a main image display window, a button menu
panel, a display magnifier, a pan and zoom reference image, and a color bar.
A color table graph window can be brought up by clicking on the color bar.
Comment 1 Lubomir Rintel 2008-10-25 10:32:04 EDT
Last time I checked it build fine in el5 mock an RPMlint was silent.
Comment 2 Mamoru TASAKA 2008-10-25 13:56:28 EDT
Created attachment 321519 [details]
Comment for Patch2

Some notes for 1.35.1-1:

* License
  - Well as far as I checked the whole code I only find codes
    under
    * license written in "COPYING" (Copyright only)
    * LGPLv2+ (e.g. fitsfile.c)
    So the license tag should be "LGPLv2+".
    Also add some files to %doc which indicates LGPL license.

* Source origin
  - Would you write where you obtained %SOURCE3?

* About saoimage-1.35.1-mkstemp.patch
  - See attached.

* BuildRequires
  - In your srpm "BuildRequires: desktop-file-utils" is missing
    (while the spec file on the URL you posted here has it)

* %install script
  - What is the line below for?
-------------------------------------------------------------
install -d $RPM_BUILD_ROOT/dev
-------------------------------------------------------------

* XML file
  - I guess this should be installed under %_datadir/mime/packages

* Directory ownership issue
  - The directory %_datadir/mime must not be owned by this
    package.


By the way:
[tasaka1@localhost saoimage]$ gdb saoimage
GNU gdb Fedora (6.8-15.fc10)
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i386-redhat-linux-gnu"...
(gdb) run
Starting program: /usr/bin/saoimage 
open error: No such file or directory
ERROR: unable to open /dev/imt1o
Error: No remote input possible.

 Smithsonian Astrophysical Observatory image utility
 SAOimage 1.35.1 1 December 2003, Doug Mink, dmink@cfa.harvard.edu

Reserved 512 x 512 image data buffer.

Program received signal SIGSEGV, Segmentation fault.
0x0807bf89 in init_window_basics (border_pixel=0) at wndwinit.c:201
201       (void)XDestroyImage(ximage);
Missing separate debuginfos, use: debuginfo-install libXau.i386 libXcursor.i386 libXdmcp.i386 libXfixes.i386 libXrender.i386
(gdb) thread apply all bt
(gdb) bt
#0  0x0807bf89 in init_window_basics (border_pixel=0) at wndwinit.c:201
#1  0x0807bfd1 in init_windows2 () at wndwinit.c:97
#2  0x0806bded in init_packages (argc=1, argv=0xbffff3e4) at maininit.c:206
#3  0x0806bf44 in main (argc=1, argv=0xbffff3e4) at maininit.c:118
(gdb) qui
The program is running.  Exit anyway? (y or n) y
[tasaka1@localhost saoimage]$ exit
Comment 3 Mamoru TASAKA 2008-10-25 14:02:37 EDT
By the way it may be good to check implicit function declaration
by adding -Werror-implicit-function-declaration:

http://www.redhat.com/archives/rhl-devel-list/2008-March/msg02036.html
Comment 4 Lubomir Rintel 2008-10-26 14:20:06 EDT
Thanks for the comments Mamoru!

(In reply to comment #2)
> Created an attachment (id=321519) [details]
> Comment for Patch2

Good catch. Will fix.

> Some notes for 1.35.1-1:
> 
> * License
>   - Well as far as I checked the whole code I only find codes
>     under
>     * license written in "COPYING" (Copyright only)
>     * LGPLv2+ (e.g. fitsfile.c)
>     So the license tag should be "LGPLv2+".
>     Also add some files to %doc which indicates LGPL license.

Thanks, you seem to be right. Will be fixed in next revision.

> * Source origin
>   - Would you write where you obtained %SOURCE3?

I stole it :)
Actually -- the logo is present in the source code (being the default picture to display). I grabbed this one from some random site and converted from gif, assuming it's no problem to use it given it's already included in the source code anyways.

> * BuildRequires
>   - In your srpm "BuildRequires: desktop-file-utils" is missing
>     (while the spec file on the URL you posted here has it)

I must have uploaded some older version by mistake. I will ensure it is present in next revision.

> * %install script
>   - What is the line below for?
> -------------------------------------------------------------
> install -d $RPM_BUILD_ROOT/dev

Will remove it. It was used in on of earlier revisions, since the make install creates some links in /dev by default.

> * XML file
>   - I guess this should be installed under %_datadir/mime/packages

Right. Thanks for noticing that.

> * Directory ownership issue
>   - The directory %_datadir/mime must not be owned by this
>     package.

Why? It does not include shared-mime-info or any other package that would require it in its dependency chain.

> By the way:
> open error: No such file or directory
> ERROR: unable to open /dev/imt1o
> Error: No remote input possible.

This is OK since we do not ship IRAF.

> Program received signal SIGSEGV, Segmentation fault.

This does not happen in my el5. Could you please tell me which Fedora version do you use? Thanks!
Comment 5 Lubomir Rintel 2008-10-26 14:45:21 EDT
(In reply to comment #3)
> By the way it may be good to check implicit function declaration
> by adding -Werror-implicit-function-declaration:

This sounds worthwhile. Due to the big amount of functions without prototypes (there's at least one in vast majority of files) this would require substantial effort. For now I included headers for functions covered by FORTIFY_SOURCE where they were missing and I'll add prototypes for the rest if this becomes mandatory part of packaging guidelines. (I encourage you to advocate for that)

New package:

SPEC: http://netbsd.sk/~lkundrak/SPECS/saoimage.spec
SRPM: http://netbsd.sk/~lkundrak/SRPMS/saoimage-1.35.1-2.el5.src.rpm
Comment 6 Mamoru TASAKA 2008-10-26 14:58:10 EDT
(In reply to comment #4)
> Thanks for the comments Mamoru!
> > * Source origin
> >   - Would you write where you obtained %SOURCE3?
> 
> I stole it :)
> Actually -- the logo is present in the source code (being the default picture
> to display). I grabbed this one from some random site and converted from gif,
> assuming it's no problem to use it given it's already included in the source
> code anyways.

Okay, I trust you then.

> > * Directory ownership issue
> >   - The directory %_datadir/mime must not be owned by this
> >     package.
> 
> Why? It does not include shared-mime-info or any other package that would
> require it in its dependency chain.

Actually currently only shared-mime-info package  owns this directory
on my system. The same discussion can also be applied to
%_datadir/icons/hicolor.

> > Program received signal SIGSEGV, Segmentation fault.
> 
> This does not happen in my el5. Could you please tell me which Fedora version
> do you use? Thanks!

I use F-10 i386. segv still occurs with -2 srpm.
Comment 7 Lubomir Rintel 2008-10-26 17:04:02 EDT
(In reply to comment #6)
> (In reply to comment #4)

> > > * Directory ownership issue
> > >   - The directory %_datadir/mime must not be owned by this
> > >     package.
> > 
> > Why? It does not include shared-mime-info or any other package that would
> > require it in its dependency chain.
> 
> Actually currently only shared-mime-info package  owns this directory
> on my system. The same discussion can also be applied to
> %_datadir/icons/hicolor.

I quite don't understand if you still insist on not owning the package. In case yes, I don't see what would we gain here, except for having a slight possibility of the package leaving a stale unowned directory.

> > > Program received signal SIGSEGV, Segmentation fault.
> > 
> > This does not happen in my el5. Could you please tell me which Fedora version
> > do you use? Thanks!
> 
> I use F-10 i386. segv still occurs with -2 srpm.

Thanks. I reproduced it in a mock chroot and think I have fixed it in new package:

SPEC: http://netbsd.sk/~lkundrak/SPECS/saoimage.spec
SRPM: http://netbsd.sk/~lkundrak/SRPMS/saoimage-1.35.1-3.el5.src.rpm
Comment 8 Mamoru TASAKA 2008-10-28 12:08:10 EDT
(In reply to comment #7)
> I quite don't understand if you still insist on not owning the package. In case
> yes, I don't see what would we gain here, except for having a slight
> possibility of the package leaving a stale unowned directory.

* Please be consistent with other packages
  - Currently only 1 (binary) package (shared-mime-info) owns /usr/share/mime
  - While 81 packages have files under /usr/share/mime/packages (including shared-mime-info)

If you want to make it sure that /usr/share/mime is owned by some packages
when shared-mime-info is not installed (current packaging guidelines is against
this), this is for packaging guidelines issue (you can suggest that this 
directory must be in filesystem package or so).

ref:
  - 18 packages (??) own /usr/share/icons/hicolor
  - While 637 packages have files under /usr/share/icons/hicolor/*/apps

* And I don't think that XML files under the directory are useful
  when shared-mime-info is not installed.

> > > > Program received signal SIGSEGV, Segmentation fault.
I don't see this SEGV on -3, thanks.
Comment 9 Marek Mahut 2008-11-01 10:15:36 EDT
Thank you for the package.

The package looks sane, please take into consideration Mamoru's comment and fix small cosmetic issue around desktop file and this package is APPROVED.

  ./saoimage.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated
Comment 10 Lubomir Rintel 2008-11-01 16:22:03 EDT
(In reply to comment #9)
> Thank you for the package.
> 
> The package looks sane, please take into consideration Mamoru's comment and fix
> small cosmetic issue around desktop file and this package is APPROVED.

Will do. Thanks.

>   ./saoimage.desktop: warning: key "Encoding" in group "Desktop Entry" is
> deprecated

Encoding used to be mandatory, and including it is not an error yet. Will exclude it from recent Fedora branches and devel.

New Package CVS Request
=======================
Package Name: saoimage
Short Description: Utility for displaying astronomical images
Owners: lkundrak
Branches: EL-5 F-9 F-10
Comment 11 Dennis Gilmore 2008-11-03 13:44:25 EST
CVS Done
Comment 12 Lubomir Rintel 2008-11-03 18:13:31 EST
Thanks a lot Ma{moru,rek}, Dennis.
Imported and build.

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