Bug 220967 - Review Request: libscigraphica - A library of gtk+ widgets for SciGraphica
Review Request: libscigraphica - A library of gtk+ widgets for SciGraphica
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul F. Johnson
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 220968
  Show dependency treegraph
 
Reported: 2006-12-29 13:40 EST by Deji Akingunola
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-02-02 11:57:33 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Deji Akingunola 2006-12-29 13:40:29 EST
Spec URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica-2.1.1-1.src.rpm
Description: A library of gtk+ widgets for SciGraphica. SciGraphica is a free (GPL)
scientific application for data analysis and technical graphics.
Comment 1 Paul F. Johnson 2006-12-29 13:43:36 EST
I'll give this a shot tonight
Comment 2 Paul F. Johnson 2006-12-29 13:49:03 EST
Requires:	%name = %{version}

needs to be 

Requires: %{name} = %{version}-%{release} pkgconfig

BuildRequires:	gettext perl(XML::Parser) pkgconfig

should really be

BuildRequires: gettext-devel perl(XML::Parser)

you don't need pkgconfig as a main package BR as it's not used, but is in the
devel package.

Comment 3 Paul F. Johnson 2006-12-29 13:49:31 EST
gtk+extras-devel doesn't seem to be in FE (rawhide). Is it correctly named?
Comment 4 Deji Akingunola 2006-12-29 13:57:36 EST
It is, I've built it (libscigraphica) in mock.

In response to the other comment, I believe pkgconfig is used in configure process.
I'll put the corrected files up in a moment at 
Spec URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica-2.1.1-2.src.rpm

Thanks.
Comment 5 Deji Akingunola 2006-12-29 14:27:00 EST
(In reply to comment #2)
> Requires:	%name = %{version}
> 
> needs to be 
> 
> Requires: %{name} = %{version}-%{release} pkgconfig
> 
> BuildRequires:	gettext perl(XML::Parser) pkgconfig
> 
> should really be
> 
> BuildRequires: gettext-devel perl(XML::Parser)
> 
> you don't need pkgconfig as a main package BR as it's not used, but is in the
The build actually only relies on binaries in gettext itself, it doesn't compile
against gettext libs or use anything fron gettext-devel. Also, as mentioned
earlier, libscigraphica checks for pkgconfig when running the configure script.

> devel package.
> 
I've added it. Thanks
 

Comment 6 Paul F. Johnson 2006-12-31 08:33:41 EST
Builds cleanly in mock
rpmlint is quiet (warning on the devel package, but it can be ignored)

Review

Bad:
Release:	2 - must have the .%{?dist} tag
Requires:	%name = %{version}-%{release} - inconsistent naming - should be %{name}
%{_libdir}/scigraphica/ - this takes the ownership of the directory which should
be owned by scigraphical

Good:
UTF-8, American English, contains documentation for the main package
pkgconfig in R for the -devel package
MD5SUM matches upstream
No .la or .a files packaged

Unsure:
Does the -devel need ldconfig to be run?
Comment 7 Deji Akingunola 2006-12-31 18:22:20 EST
(In reply to comment #6)

> Bad:
> Release:	2 - must have the .%{?dist} tag
> Requires:	%name = %{version}-%{release} - inconsistent naming - should be %{name}

Fixed

> %{_libdir}/scigraphica/ - this takes the ownership of the directory which should
> be owned by scigraphical
> 
Maybe not. It just seems to design mistake on the part of upstream to name that
directory scigraphica, its a superset of things scigraphica itself installs
under it. I've modified scigraphica not to own that directory, but just install
it own stuff under it.

> Unsure:
> Does the -devel need ldconfig to be run?
I believe the package guildline says it has to be done for the package and its
subpackage that contains a *.so.

Corrected spec and srpms below, thanks.

Spec URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica-2.1.1-3.src.rpm

 
Comment 8 Paul F. Johnson 2006-12-31 18:25:19 EST
The scigraphica directory must belong to the application and not the library.
Comment 9 Mamoru TASAKA 2007-01-01 10:08:00 EST
Well,

(In reply to comment #7)
> (In reply to comment #6)
> > Unsure:
> > Does the -devel need ldconfig to be run?
> I believe the package guildline says it has to be done 
> for the package and its
> subpackage that contains a *.so.
It is not necessary. As when you try "ldconfig -v" and it returns like:
-------------------------------------------------
        libGL.so.1 -> libGL.so.1.2
-------------------------------------------------
ldconfig checks libraries with somajor if the library has 
somajor/sominor. and .so files are not (necessary to be) checked.

(In reply to comment #8)
> The scigraphica directory must belong to the 
> application and not the library.
Umm.. Ownership distribution between libraries <-> applications is
of second importance or less.

First of all, "all directories created during installation must
belong to some package". You can install libscigraphica without
any applications which may use this library. %{_libdir}/scigraphica/
_must_ be owned by this package.

There are other issues.
----------------------------------
/usr/share/aclocal/libscigraphica-2.0.m4
----------------------------------
This should be in -devel package and owning aclocal m4 files means
-devel package should need "Requires: automake"

libscigraphica-2.0.pc says:
-----------------------------------
Requires: glib-2.0 gtk+-2.0 gtkextra-2.0
-----------------------------------
This means that -devel package should require ????-devel 
as well as pkgconfig (Please check "Requires" section of
http://fedoraproject.org/wiki/Packaging/Guidelines)

And.. this package contains many documentations (xml/header files,
etc) and image files and keeping timestamps on these files 
are strongly recommended. Please keep timestamps on these files.
(I have not tried, however for many cases
-----------------------------------------------------
make DESTDIR=%{buildroot} INSTALL="%{__install} -c -p" install
------------------------------------------------------
works.
Comment 10 Mamoru TASAKA 2007-01-01 13:18:42 EST
Another issue.
--------------------------------------------------------------
+ make
make  all-recursive
make[1]: Entering directory `/builddir/build/BUILD/libscigraphica-2.1.1'
Making all in pixmaps
make[2]: Entering directory `/builddir/build/BUILD/libscigraphica-2.1.1/pixmaps'
make[2]: Nothing to be done for `all'.
make[2]: Leaving directory `/builddir/build/BUILD/libscigraphica-2.1.1/pixmaps'
Making all in scigraphica
make[2]: Entering directory `/builddir/build/BUILD/libscigraphica-2.1.1/scigraphica'
Making all in widgets
make[3]: Entering directory
`/builddir/build/BUILD/libscigraphica-2.1.1/scigraphica/widgets'
if /bin/sh ../../libtool --mode=compile gcc -DHAVE_CONFIG_H -I. -I. -I../.. -I.
-I../../pixmaps -I../../scigraphica -I../../scigraphica/dialogs
-I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0
-I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0
-I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng12  
-I/usr/include/gtkextra-2.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include
-I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0
-I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/freetype2
-I/usr/include/libpng12    -I/usr/include/python2.5 -I/usr/include/python2.
5/Numeric/ -DWITH_NUMERIC_PYTHON    -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
-fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386
-mtune=generic -fasynchronous-unwind-tables -g -O -W -Wall -DWITH_WARNINGS -MT
gtkplotart.lo -MD -MP -MF ".deps/gtkplotart.Tpo" \
.........

    This means that gcc optimizes this code finally by "-O", not
    "-O2"? If so, this should be fixed.
Comment 11 Deji Akingunola 2007-01-01 13:21:28 EST
(In reply to comment #9)

> 
> (In reply to comment #8)
> > The scigraphica directory must belong to the 
> > application and not the library.
> Umm.. Ownership distribution between libraries <-> applications is
> of second importance or less.
> 
> First of all, "all directories created during installation must
> belong to some package". You can install libscigraphica without
> any applications which may use this library. %{_libdir}/scigraphica/
> _must_ be owned by this package.
>
So Paul what do think about this? I actually know people that use libscigraphica
without needing scigraphica.
 
> There are other issues.
> ----------------------------------
All fixed.
Happy new year to one and all.

Spec URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica-2.1.1-4.src.rpm
Comment 12 Deji Akingunola 2007-01-01 21:39:32 EST
(In reply to comment #10)
> Another issue.

> 5/Numeric/ -DWITH_NUMERIC_PYTHON    -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2
> -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386
> -mtune=generic -fasynchronous-unwind-tables -g -O -W -Wall -DWITH_WARNINGS -MT
> gtkplotart.lo -MD -MP -MF ".deps/gtkplotart.Tpo" \
> .........
> 
>     This means that gcc optimizes this code finally by "-O", not
>     "-O2"? If so, this should be fixed.
This is caused by the configure scripts assuming it building a CVS checkout (the
developers have enabled some extra debugging options for CVS builds, and
mistakingly shipped a configure script generated in a CVS checkout).
Regenerating the configure script should have worked, but it failed because some
expected file are not shipped in the released package.
I've fixed it and some other issues I noticed while looking at this.

Spec URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica-2.1.1-5.src.rpm
Comment 13 Mamoru TASAKA 2007-01-01 22:41:29 EST
Well, some minor things and issues.

Minor:
* BuildRequies
  pkgconfig, gtk2-devel is not necessary. These are (finally) required
  by gtk+extra-devel

? Group: Development/Libraries (main)
  Usually "Development" is used for -devel package and the
  group of this type is "System Environment/Libraries".

Should be fixed:
* %{_datadir}/pixmaps/%{name}/
  is not owned by any package.
Comment 14 Deji Akingunola 2007-01-02 16:39:06 EST
(In reply to comment #13)
> Well, some minor things and issues.
> 
Fixed.

Spec URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/scigraphica/libscigraphica-2.1.1-6.src.rpm
Comment 15 Mamoru TASAKA 2007-01-03 05:55:12 EST
Seemingly okay. I leave this package as the judgment by Paul.

One question.
What is the .c souce codes included in -devel package?
--------------------------------------------------
/usr/include/scigraphica-2.0/scigraphica/algorithms/Axb_core.c
/usr/include/scigraphica-2.0/scigraphica/algorithms/lm_core.c
--------------------------------------------------
Comment 16 Mamoru TASAKA 2007-01-17 10:12:55 EST
Well, Paul, are you reviewing this?
Comment 17 Mamoru TASAKA 2007-01-27 10:31:46 EST
Umm, again ping, Paul?
Comment 18 Paul F. Johnson 2007-01-28 07:31:53 EST
Builds fine in mock, rpmlint is clean (devel gives a warning about no-docs which
can be ignored)

Review

Good

Consistent use of macros
No permission conflicts / incorrect ownership
Docs included
No static libs included
devel file uses pkgconfig correctly
library - doesn't need desktop icon
rpm requirements sane

APPROVED
Comment 19 Deji Akingunola 2007-02-02 11:57:33 EST
Imported into cvs and built. Thanks

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