This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 218176 - Review Request: gchempaint - A 2D chemical formulae drawing tool
Review Request: gchempaint - A 2D chemical formulae drawing tool
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: 218172
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-12-02 12:17 EST by Julian Sikorski
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-12-22 05:07:35 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


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

  None (edit)
Description Julian Sikorski 2006-12-02 12:17:24 EST
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 13:32:39 EST
gnome-chemistry-utils? I don't recall seeing them in extras
Comment 2 Parag AN(पराग) 2006-12-02 13:48:00 EST
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 04:17:14 EST
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 14:46:56 EST
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-04 20:57:11 EST
Removing FE-NEEDSPONSOR (bug 218210)
Comment 6 Mamoru TASAKA 2006-12-05 12:44:56 EST
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 03:12:16 EST
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 03:13:02 EST
Created attachment 142928 [details]
Mock build log
Comment 9 Mamoru TASAKA 2006-12-07 10:38:45 EST
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 15:36:30 EST
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 15:45:13 EST
Upstream bug report:
https://savannah.nongnu.org/bugs/index.php?18480
Comment 12 Julian Sikorski 2006-12-08 01:50:18 EST
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 03:37:15 EST
(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 13:48:21 EST
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 14:04:03 EST
Re-uploaded. un-resolved -> un-defined
Comment 16 Mamoru TASAKA 2006-12-10 11:04:06 EST
Well, this package seems good to me.
I leave this as the judgment of Paul.
Comment 17 Julian Sikorski 2006-12-10 11:14:18 EST
Finally ;) Paul, what do you think?
Comment 18 Paul F. Johnson 2006-12-11 15:46:53 EST
I'll crack on with it, but my buildsys is currently broken due to the ongoing
Python problems.
Comment 19 Mamoru TASAKA 2006-12-11 21:33:54 EST
(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 05:54:52 EST
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 06:30:47 EST
(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 07:12:49 EST
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.