Bug 452413 (BkChem) - Review Request: bkchem - Chemical drawing program
Summary: Review Request: bkchem - Chemical drawing program
Keywords:
Status: CLOSED NEXTRELEASE
Alias: BkChem
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Terje Røsten
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 476374
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-06-22 13:15 UTC by Henrique C. S. Junior
Modified: 2009-05-22 17:00 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2009-01-07 21:51:29 UTC
Type: ---
Embargoed:
terje.rosten: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Henrique C. S. Junior 2008-06-22 13:15:41 UTC
Spec URL: http://lspooky.fedorapeople.org/bkchem.spec
SRPM URL: http://lspooky.fedorapeople.org/bkchem-0.12.2-1.fc9.src.rpm
Description: BkChem is a chemical drawing tool that can be a good alternative to XDrawChem (abandoned since 2005 by the developers).
The alternative Pacage needs some changes to work, like the creation of a .desktop file.
This is my first package

Comment 1 Henrique C. S. Junior 2008-06-22 18:08:36 UTC
(In reply to comment #0)
> Spec URL: http://lspooky.fedorapeople.org/bkchem.spec
> SRPM URL: http://lspooky.fedorapeople.org/bkchem-0.12.2-1.fc9.src.rpm
> Description: BkChem is a chemical drawing tool that can be a good alternative
to XDrawChem (abandoned since 2005 by the developers).
> The alternative Pacage needs some changes to work, like the creation of a
.desktop file.
> This is my first package

The package seems to be working very well and the changes I made will be oticed
to the developer.



Comment 2 Terje Røsten 2008-06-25 19:03:34 UTC
Review is coming up shortly.

Comment 3 Terje Røsten 2008-06-25 19:56:56 UTC
[ x=ok  -=dont't apply  !=please fix ?=may fix ]

MUST
 [!] rpmlint must be run on every package
  a) bkchem.src: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 10)
     remove tab in the Group: line
  b) bkchem.noarch: E: non-executable-script
/usr/lib/python2.5/site-packages/bkchem/oasa/setup.py 0644
     Is this need at all? Remove in %install
 [x] package must be named according to the Package Naming Guidelines
 [x] spec file name must match the base package %{name}
 [x] package must meet the Packaging Guidelines
 [!] [GPLv2] package must be licensed with a Fedora approved license 
  The bkchem/plugins/piddle directory seems to have files with
  a mix of copyrights, can you check with upstream regarding license
  of these files?
 [!] license field in the package spec file must match the actual license
  See over 
 [x] includes the text of the license(s) in its own file: include in %doc
 [x] be written in American English
 [x] spec file for the package be legible
 [!] sources used to build the package must match the upstream source
  da8bceec65cf4e054a19c510633b61f4  bkchem-0.12.2.tar.gz
  fa3fc119f06ad0204c5c046b768cabd9  bkchem-0.12.2.tar.gz.rpm
  Remove bkchem.desktop and bkchem.png from tarball, you have to
  use pristine sources. They are added by source1 and source2,
  rpmbuild will take care of that. Ask if trouble.
  Source2 bkchem.png is already in the tarball as images/bkchem.png?
 [x] compile and build into binary rpms on at least one architecture
  http://koji.fedoraproject.org/koji/taskinfo?taskID=680857
 [-] not successfully compile an architecture: use ExcludeArch
 [x] all build dependencies must be listed in BuildRequires
 [x] spec file MUST handle locales properly
 [-] shared library files not in any default linker paths: ldconfig
 [-] relocatable package: the packager must state this fact
 [x] package must own all directories that it creates
 [x] not any duplicate files in the %files listing
 [x] permissions on files must be set properly
 [x] package must have a %clean section, which contains rm -rf %{buildroot}
 [!] consistently use macros
  spec in url is fixed, however not the spec in the src.rpm
  Bump release when doing updates 13, 19 and 22 has the same release,
  should be on release 3 now.
 [x] must contain code, or permissable content
 [?] large docs should go in a -doc subpackage
  Maybe split off the doc/ dir in a separate bkchem-doc package?
 [x] %doc must not affect the runtime of the application
 [-] header files must be in a -devel package
 [-] static libraries must be in a -static package
 [-] containing pkgconfig(.pc) files must 'Requires: pkgconfig'
 [-] library files that end in .so: go in a -devel package
 [-] devel pkg: require base package using a fully versioned dependency
 [-] no .la libtool archives
 [!] gui app include a %{name}.desktop file
  Set vendor to nothing: --vendor=""
 [x] must not own files or directories already owned by other packages
 [x] %install includes rm -rf %{buildroot}
 [x]  filenames in rpm packages must be valid UTF-8
SHOULD
 [!] ping upstream about missing license text
    see above, the piddle subdir.
 [?] translations if description and summary sections
 [x] test that the package builds in mock
  http://koji.fedoraproject.org/koji/taskinfo?taskID=680857
 [x] compile and build into binary rpms on all archs
  http://koji.fedoraproject.org/koji/taskinfo?taskID=680857
 [x] package functions as described
 [!] those scriptlets are sane
   Just remove the %post/%postun scripts (sorry).
 [-] subpackages require the base package fully versioned dep
 [-] pkgconfig(.pc) in devel
 [-] no explicit file dep outside /etc, /bin/, /sbin, /usr/{sbin,bin}


Comment 4 Henrique C. S. Junior 2008-09-14 19:28:46 UTC
The package is almost done now. Only a few changes in the .spec file to make some changes in the upstream tarball are needed.

Comment 5 Susi Lehtola 2008-09-29 14:52:54 UTC
Prereview:

[ x=ok  -=dont't apply  !=please fix ?=may fix ]

MUST
 [!] rpmlint must be run on every package

SPECS/bkchem.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 11)
bkchem.noarch: E: non-executable-script /usr/lib/python2.5/site-packages/bkchem/oasa/setup.py 0644
1 packages and 1 specfiles checked; 1 errors, 1 warnings.

Fix changelog in SPEC file, you have 0.12.2-1 twice. (Why is this not picked up by rpmlint?)

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!SRPM spec does not match the one on the website!
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

- Remove the installation script setup.py from the package, it is not needed.

 [x] package must be named according to the Package Naming Guidelines
 [x] spec file name must match the base package %{name}
 [x] package must meet the Packaging Guidelines
 [x] Package must be licensed with a Fedora approved license

- no plugin anymore

 [!] license field in the package spec file must match the actual license

- The license is GPLv2+ not GPLv2. (lines 298-299 in gpl.txt) 

 [x] includes the text of the license(s) in its own file: include in %doc
 [x] be written in American English
 [x] spec file for the package be legible
 [!] sources used to build the package must match the upstream source

da8bceec65cf4e054a19c510633b61f4  bkchem-0.12.2.tar.gz
fa3fc119f06ad0204c5c046b768cabd9  SOURCES/bkchem-0.12.2.tar.gz

- Still need to use original source package and use source1, source2 etc and patches for any modifications.

 [x] compile and build into binary rpms on at least one architecture

- OK on F9 x86_64.

 [?] not successfully compile an architecture: use ExcludeArch

- Not checked

 [?] all build dependencies must be listed in BuildRequires

- Works for me, mock may be stricter.

 [x] spec file MUST handle locales properly
 [-] shared library files not in any default linker paths: ldconfig
 [-] relocatable package: the packager must state this fact
 [x] package must own all directories that it creates
 [x] not any duplicate files in the %files listing
 [x] permissions on files must be set properly
 [x] package must have a %clean section, which contains rm -rf %{buildroot}
 [x] consistently use macros
 [x] must contain code, or permissable content
 [!] large docs should go in a -doc subpackage

- Doc dir is almost half the size of the whole package, needs to be branched to its own package.

 [x] %doc must not affect the runtime of the application
 [-] header files must be in a -devel package
 [-] static libraries must be in a -static package
 [-] containing pkgconfig(.pc) files must 'Requires: pkgconfig'
 [-] library files that end in .so: go in a -devel package
 [-] devel pkg: require base package using a fully versioned dependency
 [-] no .la libtool archives
 [x] gui app include a %{name}.desktop file
 [x] must not own files or directories already owned by other packages
 [x] %install includes rm -rf %{buildroot}
 [x]  filenames in rpm packages must be valid UTF-8

Comment 6 Henrique C. S. Junior 2008-10-05 04:09:06 UTC
In less than 1 week, Beda, the upstream programmer, will be releasing one new version of BKChem with two specific sources. One, specially prepared to avoid some problems with Fedora's criteria, like the issue with Piddle's licence.
I'm only waiting for him.

Comment 7 Terje Røsten 2008-10-05 08:23:59 UTC
Ok, that's great.

Comment 9 Terje Røsten 2008-10-20 07:26:13 UTC
Still tarball issue here:

md5sum bkchem-0.12.3-nopiddle.tar.gz*          
cf44527c4e2ea770dec10dd8330e4182  bkchem-0.12.3-nopiddle.tar.gz
5937df0331fd7fcbfd5ef40b5923d59e  bkchem-0.12.3-nopiddle.tar.gz.rpm

lspooky, have you modified the tarball? 

Please fix this before we continue, then post links to spec and srpm.

Comment 10 Henrique C. S. Junior 2008-10-20 23:25:19 UTC
Hi, Terje
I've fixed some minor errors in the upstream tarbal. Beda have already fixed it in the upstream now, so, here we go again:
[1] - bkchem.spec: http://lspooky.fedorapeople.org/bkchem/0.12.3/bkchem.spec
[2] - bkchem.src.rpm: http://lspooky.fedorapeople.org/bkchem/0.12.3/bkchem-0.12.3-1.fc9.src.rpm

Comment 12 Terje Røsten 2008-11-10 15:18:29 UTC
Ok, everything is fine here now:

 APPROVED

Now it blocks on FE-NEEDSPONSOR, I am not a sponsor and can't help you there.
Anyway I would recommend that you do some reviews, more info here:

 https://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Comment 13 Mamoru TASAKA 2008-11-10 18:41:02 UTC
Henrique, do you have another review request by you or
have any pre-review for other person's review request?

To follow Fedora policy written on HowToGetSponsored wiki, I request
new packages to do either of the two I wrote above.

If you choose to do a pre-review of other person's review request,
- Fedora package collection review requests which are waiting for someone to
  review can be checked on:
  http://fedoraproject.org/PackageReviewStatus/NEW.html
  (NOTE: please don't choose "Merge Review")

- Review guidelines are described mainly on:
  http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
  http://fedoraproject.org/wiki/Packaging/Guidelines
  http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

Comment 14 Mamoru TASAKA 2008-11-10 19:33:13 UTC
By the way

- All codes are under GPLv2+ (or no license) and HTML files
  are under GFDL, so the license tag must be
  "GPLv2+ and GFDL".

- This package uses some files from Python powerwidgets
  "Pmw", which is actually in Fedora as "python-pmw"
  (bug 462250: I reviewed...). 
  Is it possible to make this package use system-wide
  python-pmw?
  (simply creating symlinks will be easier?)

- I guess packaging oasa seperately will be better.
  http://bkchem.zirael.org/oasa_en.html

Comment 15 Henrique C. S. Junior 2008-11-11 17:18:49 UTC
Hi, Mamoru,
I will contact Beda Kosata to inquire about the possibility of creating links. Soon I'll post the answer here



(In reply to comment #14)
> By the way
> 
> - All codes are under GPLv2+ (or no license) and HTML files
>   are under GFDL, so the license tag must be
>   "GPLv2+ and GFDL".
> 
> - This package uses some files from Python powerwidgets
>   "Pmw", which is actually in Fedora as "python-pmw"
>   (bug 462250: I reviewed...). 
>   Is it possible to make this package use system-wide
>   python-pmw?
>   (simply creating symlinks will be easier?)
> 
> - I guess packaging oasa seperately will be better.
>   http://bkchem.zirael.org/oasa_en.html

Comment 16 Mamoru TASAKA 2008-12-10 15:34:03 UTC
Henrique, any news from the upstream?

Comment 17 Henrique C. S. Junior 2008-12-10 17:36:56 UTC
(In reply to comment #16)
> Henrique, any news from the upstream?

Hello, Mamoru, unfortunately I don't have  any feedback from the upstream yet.
How should I proceed in the case of not receive any response?
In any case, I will send another email and see what happens.
I'm not giving up yet.

Comment 18 Mamoru TASAKA 2008-12-11 15:25:32 UTC
At least would you would you submit a seperate review request for
orsa and make BkChem depend on (use) seperated orsa?

Comment 19 Henrique C. S. Junior 2008-12-11 15:55:54 UTC
Good news! I just received a reply from Beda. There follows:
--------------------
> - This package uses some files from Python powerwidgets
>  "Pmw", which is actually in Fedora as "python-pmw"
>  (bug 462250: I reviewed...).
>  Is it possible to make this package use system-wide
>  python-pmw?
>  (simply creating symlinks will be easier?)

I am packaging Pmw together with BKChem because I had to fix a bug or
two there and there was no upstream maintenance of Pmw going on.
However, it is quite possible that the Pmw package in Fedora also has
these fixes, so it might be possible to use the Fedora package.
You can try it by simply deleting all files named Pmw*.py in bkchem and
see if it would work when the Fedora package is installed.
If yes, I could prepare a special package without Pmw.

>
> - I guess packaging oasa seperately will be better. (and packaging it separetely will pe necessari some code changes?)
>

I have started to release OASA just this year because of some interest
in it. It would of course be able to have two separate packages, but
then more effort would have to be put into keeping them in sync - when I
release BKChem I always expect that it is used with the version of OASA
that is current at the time. Failing to match versions of OASA and
BKChem could lead to subtle and hard to find errors.
On the other hand, it is not much of a problem for me to create a
package of BKChem without OASA.

> I know you must be very busy and although I am not an experienced programmer, if you guide me to do the necessary changes, I'll be happy to help.
>

All these things would not require that much time. Just let me know what
I should remove from the package (as I do now with Piddle) and I will do it.
-----------

So, I'll start the tests with python-pmw, reporting to Beda if some changes are necessary.
Also, I will start the process of packaging OASA, and as is customary, I will request a review of the package.

Comment 20 Henrique C. S. Junior 2008-12-13 01:23:11 UTC
After some tests, BKChem seems to work fine with Fedora's python-pmw, so the included pwm in BKChem will be dropped to avoid this obvious redundance.
In the next step, I will start the packaging of OASA.

Comment 21 Henrique C. S. Junior 2008-12-13 19:50:59 UTC
Here we go.
BKChem worked perfectly with python-pmw from fedora. OASA is now on a separate package awaiting to be reviewed (Bug 476374) and all the tests with this new release of BKChem (using pithon-pmw and the new package of OASA) are going OK.

SPEC: http://lspooky.fedorapeople.org/bkchem/0.12.5/bkchem.spec
SRPM: http://lspooky.fedorapeople.org/bkchem/0.12.5/bkchem-0.12.5-1.fc10.src.rpm

Comment 22 Henrique C. S. Junior 2008-12-16 14:12:14 UTC
Untill now, Beda was creating a tarball especially for Fedora, but it could be a substantially large working for him in the future. Following the advice of Terje, I created a patch that works directly on the upstream's "original". I hope I have done it correctly and that it is the best solution.

SPEC: http://lspooky.fedorapeople.org/bkchem/0.12.5/2/bkchem.spec
SRPM: http://lspooky.fedorapeople.org/bkchem/0.12.5/2/bkchem-0.12.5-2.fc10.src.rpm

Comment 23 Terje Røsten 2008-12-16 20:54:47 UTC
Looks good, some minor things:

o rpmlint:

 bkchem.src: W: mixed-use-of-spaces-and-tabs (spaces: line 34, tab: line 3)

o use diff -x option or remove setup.py~  to clean up the patch.

o use wget -N to get correct timestamp on tarball.

o maybe change naming: bkchem.patch -> bkchem-remove-oasa.patch ?

I see you are sponsored, congrats!

Comment 24 Mamoru TASAKA 2008-12-17 18:42:38 UTC
Some notes for 0.12.5-2:

* License
  - As I said above, the license tag should be 
    "GPLv2+ and GFDL"

* About Patch1
  - I guess the size of Patch1 is unneededly large.
    Would you
    - simply remove unneeded files
    - and create a patch for the rest part?
    (It seems actually only setup.py is patched?)

Comment 25 Henrique C. S. Junior 2008-12-17 21:17:36 UTC
Hello, Mamoru, 
The patch 3 performs tasks in BKChem: 
* Remove the piddle plugin (excluding the folder, the files related and the entry in setup.py). Piddle is a plugin that is obsolete and will be removed from BKChem in the future because its functions are performed by pycairo today. Moreover, he had several problems of license that the upstream will not be able to solve).
* Remove OASA's folder and removes the entries in setup.py.
* Exclude Pmw*.py files that are redundant in fedora, since we have a python-pmw package already.

Beda was creating a tarball especially for Fedora untill now, but applying patches to the official tarball seemed a better approach to the upstream and save its time. Is there any other alternative that I can take, Mamoru?
Sorry if this is taking longer than necessary.

Comment 26 Henrique C. S. Junior 2008-12-17 21:33:21 UTC
***The patch performs 3 tasks***

Comment 27 Henrique C. S. Junior 2008-12-18 12:58:25 UTC
Here are the changes:
* The name of the patch was changed to something more self explanatory "bkchem-exclude-oasa-piddle-pmw.patch";
* The license was added as GPLv2+ and GFDL;
* The changelog explains about the adoption of the patch and what the patch does;
* Rpmlint is not showing errors.

In fact, the patch is absurdly large, 1.3 MB. Is it better to simply change the patch to rm commands directly into the file .spec and apply the patch only in the file setup.py?

SPEC: http://lspooky.fedorapeople.org/bkchem/0.12.5/2/bkchem.spec
PATCH: http://lspooky.fedorapeople.org/bkchem/0.12.5/2/bkchem-exclude-oasa-piddle-pmw.patch
SRPM: http://lspooky.fedorapeople.org/bkchem/0.12.5/2/bkchem-0.12.5-2.fc10.src.rpm

Comment 28 Henrique C. S. Junior 2008-12-18 13:50:44 UTC
Here we go with a new proposed .spec[1] and a patch[2] much smaller. Also, of course, the SRPM[3].
[1] - http://lspooky.fedorapeople.org/bkchem/0.12.5/2/proposed/bkchem.spec
[2] - http://lspooky.fedorapeople.org/bkchem/0.12.5/2/proposed/bkchem-setup.patch
[3] - http://lspooky.fedorapeople.org/bkchem/0.12.5/2/proposed/bkchem-0.12.5-2.fc10.src.rpm

Comment 29 Mamoru TASAKA 2008-12-18 14:51:14 UTC
Please change the release number every time you modify
your spec file to avoid confusion.

Comment 31 Mamoru TASAKA 2008-12-18 17:56:21 UTC
Looks good, however I suggest that one line in %changelog must be shorter
than now (note that on %description rpmlint will complain if one line
exceeds 79 characters)

Comment 32 Terje Røsten 2008-12-18 18:07:06 UTC
Yeah, I suggest the following: rm -> %{__rm} or %{__rm} -> rm. 
It's normal to have Patch1 line right below SourceX line.

Comment 33 Henrique C. S. Junior 2008-12-18 18:34:03 UTC
Thanks, Mamoru and Terje,
Here are the changes. :)
spec: http://lspooky.fedorapeople.org/bkchem/0.12.5/4/bkchem.spec
srpm: http://lspooky.fedorapeople.org/bkchem/0.12.5/4/bkchem-0.12.5-4.fc10.src.rpm

Comment 34 Mamoru TASAKA 2008-12-19 17:12:14 UTC
Well, what I wanted to say is that long lines in %changelog
should be divided into more than 2 lines, not saying that
the contents of %changelog should be smaller...

But all other things seems good.

Comment 35 Henrique C. S. Junior 2008-12-23 13:58:12 UTC
Hello, sorry for the delay in responding, but I was working in another city these days and am still without access to a PC. Day 24 I will be back. I've made some changes in the .spec file according to your suggestions and rebuild the SRPM.

spec: http://lspooky.fedorapeople.org/bkchem/0.12.5/4/bkchem.spec
srpm:
http://lspooky.fedorapeople.org/bkchem/0.12.5/4/bkchem-0.12.5-4.fc10.src.rpm

Comment 36 Mamoru TASAKA 2008-12-26 07:52:20 UTC
To Terje:

Would you check the latest srpm by Henrique? It looks
good to me.

Comment 37 Terje Røsten 2008-12-26 09:52:50 UTC
Yes, this is package is in excellent shape. 

Please make CVS request after #476374 is closed.

Comment 38 Henrique C. S. Junior 2008-12-26 11:01:48 UTC
Thank you, guys.
I've made a CVS request for python-oasa following the instructions here: https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure (I hope this is the right procedure).
I'll be waiting for your ok (or not) to bkchem too.

Happy new year.

Comment 39 Mamoru TASAKA 2008-12-26 14:53:01 UTC
By the way for CVS request you don't have to wait for
python-oasa review requst to completely be closed.

Comment 40 Henrique C. S. Junior 2008-12-26 17:11:55 UTC
I've started a CVS request for bkchem too. Am I doing this ahead of time?

Comment 41 Terje Røsten 2008-12-26 17:20:04 UTC
It's ok, continue.

Comment 42 Mamoru TASAKA 2008-12-26 17:23:30 UTC
Please fill in CVS request template.

Comment 43 Henrique C. S. Junior 2008-12-26 17:52:16 UTC
New Package CVS Request
=======================
Package Name: bkchem
Short Description: Chemical drawing tool
Owners: lspooky
Branches: F-9 F-10
InitialCC: terjeros mtasaka

Comment 44 Kevin Fenzi 2008-12-28 19:22:01 UTC
cvs done.

Comment 45 Fedora Update System 2009-01-06 10:32:45 UTC
bkchem-0.12.5-4.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/bkchem-0.12.5-4.fc9

Comment 46 Fedora Update System 2009-01-06 10:34:14 UTC
bkchem-0.12.5-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/bkchem-0.12.5-4.fc10

Comment 47 Fedora Update System 2009-01-07 09:15:26 UTC
bkchem-0.12.5-4.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update bkchem'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-0081

Comment 48 Fedora Update System 2009-01-07 09:31:55 UTC
bkchem-0.12.5-4.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update bkchem'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2009-0204

Comment 49 Fedora Update System 2009-01-07 21:51:24 UTC
bkchem-0.12.5-4.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 50 Fedora Update System 2009-01-07 21:51:56 UTC
bkchem-0.12.5-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.


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