Bug 218176 - Review Request: gchempaint - A 2D chemical formulae drawing tool
Summary: Review Request: gchempaint - A 2D chemical formulae drawing tool
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Paul F. Johnson
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 218172
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-12-02 17:17 UTC by Julian Sikorski
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-12-22 10:07:35 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Mock build log (249.63 KB, text/plain)
2006-12-06 08:13 UTC, Julian Sikorski
no flags Details

Description Julian Sikorski 2006-12-02 17:17:24 UTC
Spec URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint.spec
SRPM URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint-0.6.6-1.src.rpm
Description: GChemPaint is a 2D chemical structures editor for the Gnome-2 desktop. GChemPaint is a multi document application and is a bonobo server so that some chemistry can be embedded in other Gnome applications.

Package builds fine inside mock, but keep in mind it requires gnome-chemistry-utils, which I have submitted for review separately. The only warnings rpmlint outputs say about no documentation in devel package and non-config file in /etc (gconf schema).

Comment 1 Paul F. Johnson 2006-12-02 18:32:39 UTC
gnome-chemistry-utils? I don't recall seeing them in extras

Comment 2 Parag AN(पराग) 2006-12-02 18:48:00 UTC
Look like you forgot to check that dependent bug 218172 is first to go in FE and
which (may be) under REVIEW now.

Comment 3 Julian Sikorski 2006-12-03 09:17:14 UTC
Spec URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint.spec
SRPM URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint-0.6.6-2.src.rpm

New release:
- Removed packages pulled by gnome-chemistry-utils-devel from BuildRequires
- Install desktop files using desktop-file-install
- Fixed Requires for -devel package
- Preserve timestamps

Comment 4 Julian Sikorski 2006-12-03 19:46:56 UTC
Spec URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint.spec
SRPM URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint-0.6.6-3.src.rpm
New release:
- Removed --add-category X-Fedora from desktop-file-install command
- Added some wildcards to make %files section shorter

Comment 5 Mamoru TASAKA 2006-12-05 01:57:11 UTC
Removing FE-NEEDSPONSOR (bug 218210)

Comment 6 Mamoru TASAKA 2006-12-05 17:44:56 UTC
Some comments from me.

From http://fedoraproject.org/wiki/Packaging/Guidelines :
* Use rpmlint
------------------------------------------------------
W: gchempaint non-conffile-in-etc /etc/gconf/schemas/gchempaint-arrows.schemas
W: gchempaint macro-in-%changelog files
W: gchempaint-devel no-documentation
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
g_printable_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_set_buffer
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_iter_location
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_buffer
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_text_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_ellipse_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_bpath_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_pango_layout
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
g_printable_export_svg
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_iter_at_location
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_group_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rect_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_line_ext_get_type
------------------------------------------------------
- non-conffile-in-etc warning can be ignored. gconf schemas 
  files are not config file.
- When you want to use percent in %changelog, please use %%.
- Well, for undefined non-weak symbols issue:
  Some reviewers say this can be ignorable for Review, while 
  some reviewers say they should be fixed before approving the package.

  For this package, libgchempaint.so is included in -devel package so
  leaving these undefined symbols causes failure for linking to
  libgchempaint.so, so this should be fixed before this package is
  approved.

  Then:
  1. for g_printable_get_type and g_printable_export_svg:
     This is defined in ./libgcpcanvas/gprintable.c, however
     gprintable.o is not used in libgchempaint.so.0.6.6 so this
     is actually wrong (this should cause failure linkage).
  2. for gnome_canvas_... :
     Perhaps linking libgchempaint.so to libgnomecanvas-2.so
     (in libgnomecanvas-devel) will fix these complaint.

* Requires
  pkgconfig (in -devel)
  - It seems that -devel package does not includes pkgconfig .pc file.

* File and Directory Ownership
  - /usr/share/omf/gchempaint/ is not owned by any package.

Comment 7 Julian Sikorski 2006-12-06 08:12:16 UTC
Spec URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint.spec
SRPM URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint-0.6.6-4.src.rpm
New release:
- Fixed %%changelog section
- Fixed directory ownership
- Removed pkgconfig from -devel package Requires

I can't reproduce the undefined-non-weak-symbol problem on fc6. Maybe I should
add libgnomecanvas-devel to BuildRequires? I'll attach my mock build log.

Comment 8 Julian Sikorski 2006-12-06 08:13:02 UTC
Created attachment 142928 [details]
Mock build log

Comment 9 Mamoru TASAKA 2006-12-07 15:38:45 UTC
Oops.. I didn't aware that I have received your mail.

Well, rpmlint complaint about undefined non-weak symbol
can be gained when you execute rpmlint agaist "installed"
rpm (not against rpm package), i.e.
-------------------------------------------------
[tasaka1@localhost ~]$ rpmlint gchempaint
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
g_printable_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_set_buffer
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_iter_location
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_buffer
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_text_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_ellipse_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_bpath_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_pango_layout
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
g_printable_export_svg
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rich_text_ext_get_iter_at_location
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_group_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_rect_ext_get_type
W: gchempaint undefined-non-weak-symbol /usr/lib/libgchempaint.so.0.6.6
gnome_canvas_line_ext_get_type
W: gchempaint non-conffile-in-etc /etc/gconf/schemas/gchempaint-arrows.schemas
-------------------------------------------------
Well, gnome_canvas_???are also defined in sources under libgcpcanvas,
however, those files are not included in libgchempaint.so.

Perhaps it is not expected that this problem will be fixed in 0.6.X version
because fixing this requires entire reconstruction of source.

A. It may be possible that this has been already fixed in development
   version. Would you package 0.7.5 version of gchempaint?
   Then I can check if upgrading to 0.7.5 solves this problem
B. If 0.7.5 still has this problem, would you ask upstream?

Comment 10 Julian Sikorski 2006-12-07 20:36:30 UTC
Well, the problem with development version is the following: it requires
development version of gnome-chemistry-utils, which requires goffice >= 0.3
(also an unstable development version, shipped neither by fc6 nor by rawhide).
So no matter if the problem was fixed in unstable branch, shipping it for fedora
would be a bit problematic. I can ask upstream for fix anyway, but IMO it is
very unlikely that someone would want to link to libgchempaint.so

Comment 11 Julian Sikorski 2006-12-07 20:45:13 UTC
Upstream bug report:
https://savannah.nongnu.org/bugs/index.php?18480

Comment 12 Julian Sikorski 2006-12-08 06:50:18 UTC
Comment from Jean Brefort:
Hmm, not easily fixed. libgchempaint was not intended to be used by other
programs (it should be possible with 0.8.0 but this one is not released yet). At
the moment, libgcpcanvas is only used as a static library, so adding -lgcpcanvas
to LIBS for libgchempaint just makes libtool fail. If fixing this bug is a
requirement for you, the solution is to build a shared version of libgcpcanvas.
Tell me if you need it. Also, please apply patch for #18159 before releasing.

So, is fixing this bug a requirement? Al for the patch, I'll add it later today.

Comment 13 Mamoru TASAKA 2006-12-08 08:37:15 UTC
(In reply to comment #12)
> Comment from Jean Brefort:
> Hmm, not easily fixed. 
> 
> So, is fixing this bug a requirement? Al for the patch, I'll add it later today.

Yes, I know fixing this issue is not a easy work. My opinion is:
* If providing *-devel package* is necessary, this issue should be fixed
* If not (i.e. providing -devel package is not neccesary *for now*,
  which means that the library included in gchempaint rpm
  are only needed by binaries in gchempaint rpm),
  just dropping -devel package and leaving this issue as it is
  can be allowed.

Comment 14 Julian Sikorski 2006-12-08 18:48:21 UTC
Spec URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint.spec
SRPM URL: http://www.republika.pl/belegdol/rpmstuff/gchempaint-0.6.6-5.src.rpm
New release:
- Added upstream patch #18159, fixing AMD64 startup crash
- Disabled -devel package until unresolved non-weak symbols issue is fixed
- Added hicolor-icon-theme to Requires

I have decided to leave the commented out sections in the spec, as the issue has
been fixed in the development branch of gchempaint and the devel package will
return once 0.8.0 version is released.

Comment 15 Julian Sikorski 2006-12-08 19:04:03 UTC
Re-uploaded. un-resolved -> un-defined

Comment 16 Mamoru TASAKA 2006-12-10 16:04:06 UTC
Well, this package seems good to me.
I leave this as the judgment of Paul.

Comment 17 Julian Sikorski 2006-12-10 16:14:18 UTC
Finally ;) Paul, what do you think?

Comment 18 Paul F. Johnson 2006-12-11 20:46:53 UTC
I'll crack on with it, but my buildsys is currently broken due to the ongoing
Python problems.

Comment 19 Mamoru TASAKA 2006-12-12 02:33:54 UTC
(In reply to comment #18)
> I'll crack on with it, but my buildsys is currently broken due to the ongoing
> Python problems.

Ah... actually I also use rawhide and I had to rebuild
several packages against python 2.5.
* If you have trouble with mock, please see:
  https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=219029
* Additional info: If you have trouble with plague, please see:
  https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=214687

Comment 20 Paul F. Johnson 2006-12-17 10:54:52 UTC
rpmlint is not quiet
gchempaint.i386.rpm

W: gchempaint non-conffile-in-etc /etc/gconf/schemas/gchempaint-arrows.schemas

builds fine in mock

Once you sort out the conffile, I'll do the full review

Comment 21 Mamoru TASAKA 2006-12-17 11:30:47 UTC
(In reply to comment #20)
> W: gchempaint non-conffile-in-etc /etc/gconf/schemas/gchempaint-arrows.schemas

This should be ignored as gconf schemas file is not a config
file while this file is usually installed under /etc/gconf.

This is rather rpmlint bug.

Comment 22 Paul F. Johnson 2006-12-17 12:12:49 UTC
rpmlint, so much fun in a package so small ;-p

Review

spec file in US English, UTF-8
macros consistent
MD5s check
Includes docs
Actually works
Can't see any missing deps
-devel comments noted

APPROVED


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