Bug 666954 - Review Request: cherrytree - Hierarchical note taking application
Summary: Review Request: cherrytree - Hierarchical note taking application
Keywords:
Status: CLOSED DUPLICATE of bug 750961
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Robin Lee
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2011-01-03 20:18 UTC by Christoph Wickert
Modified: 2011-11-03 02:27 UTC (History)
6 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-11-02 08:50:47 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
make cherrytree get a python(abi) requirement properly (907 bytes, patch)
2011-06-23 19:30 UTC, Robin Lee
no flags Details | Diff
make cherrytree get a python(abi) requirement properly (877 bytes, patch)
2011-06-23 19:47 UTC, Robin Lee
no flags Details | Diff

Description Christoph Wickert 2011-01-03 20:18:30 UTC
Spec URL: http://cwickert.fedorapeople.org/review/cherrytree.spec
SRPM URL: http://cwickert.fedorapeople.org/review/cherrytree-0.19-1.fc15.src.rpm
Description: Cherrytree is a hierarchical note taking application, featuring rich text and syntax highlighting, storing all the data (including images) in a single xml file with extension “.ctd”.

Comment 1 Christoph Wickert 2011-01-03 20:21:40 UTC
Thomas, this is python. ;)

Comment 2 Robin Lee 2011-01-04 02:05:49 UTC
/usr/share/cherrytree/glade/cherrytree.glade.h seems not necessary to be installed.

The latest changelog is missed.

And please paste the output of rpmlint run on all the rpms.

Comment 3 Robin Lee 2011-01-04 02:19:54 UTC
ASCII double quote is preferred in description like ".ctd".

Comment 4 Christoph Wickert 2011-01-04 08:52:04 UTC
(In reply to comment #2)
> /usr/share/cherrytree/glade/cherrytree.glade.h seems not necessary to be
> installed.
> 
> The latest changelog is missed.

Both fixed, thanks

> And please paste the output of rpmlint run on all the rpms.

I always though this is part of the review when the reviewer tests is the package builds in mock, otherwise a requester can post 'faked' rpmlint output here. But here you are:

rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/cherrytree-0.19-2.fc15.*
cherrytree.noarch: W: spelling-error %description -l en_US xml -> XML, cml, ml
cherrytree.noarch: W: spelling-error %description -l en_US ctd -> cts, ct, cd
cherrytree.noarch: W: no-manual-page-for-binary cherrytree
cherrytree.src: W: spelling-error %description -l en_US xml -> XML, cml, ml
cherrytree.src: W: spelling-error %description -l en_US ctd -> cts, ct, cd
2 packages and 0 specfiles checked; 0 errors, 5 warnings.

(In reply to comment #3)
> ASCII double quote is preferred in description like ".ctd".

Fixed, was a left over from copy & paste from the website.

SPEC: http://cwickert.fedorapeople.org/review/cherrytree.spec
SRPM: http://cwickert.fedorapeople.org/review/cherrytree-0.19-2.fc15.src.rpm

Comment 5 Robin Lee 2011-01-04 10:54:52 UTC
With respect to deb/control and modules/core.py, this package depends on 7za, so please add /usr/bin/7za to requirement.

Comment 6 Christoph Wickert 2011-01-04 11:06:30 UTC
Will do. Would you mind doing a full review instead of step by step?

Comment 7 Robin Lee 2011-01-04 12:59:00 UTC
* TODO: python2-devel is now preferred to python-devel.
https://fedoraproject.org/wiki/Packaging:Python#BuildRequires

* Source tarball matches upstream:
$ md5sum cherrytree-0.19.tar.gz 
f194c5f3b3016e2d1c9807e435bbf625  cherrytree-0.19.tar.gz

* I hope you can convince upstream to rid of gtk requirement from setup.py. It should be an easy job.


This package will be approved when the two last issues fixed.

Comment 8 Thomas Spura 2011-01-04 22:03:15 UTC
(In reply to comment #1)
> Thomas, this is python. ;)

Hehe, nice ;-)

(In reply to comment #7)
> * I hope you can convince upstream to rid of gtk requirement from setup.py. It
> should be an easy job.

Not that easy...
gtk is not required in setup.py. They import the cons module to get the version from there. So it would be best, if they would introduce a new *.py file for the gtk variables and import it from there.
(patch below)

But I would suggest to not install into %{python_sitelib}, because upstream (and so othere distributions don't do that too, but your choice ;-)

It's starting up, but it would be better to ask upstream, if the patch is ok...

SPEC: http://tomspur.fedorapeople.org/review/cherrytree.spec
PATCH:http://tomspur.fedorapeople.org/review/cherrytree-gtk-dep.patch

Comment 9 Robin Lee 2011-01-05 07:03:11 UTC
This package uses glade mechanism, you'd better add pygtk2-libglade to requirement.

Comment 10 Christoph Wickert 2011-01-06 21:55:49 UTC
(In reply to comment #7)
> * TODO: python2-devel is now preferred to python-devel.
> https://fedoraproject.org/wiki/Packaging:Python#BuildRequires

AFAIK python-devel is a virtual provides that always defaults to the perferred version of python. IMHO we should not hardcode a specific version.

> * I hope you can convince upstream to rid of gtk requirement from setup.py. It
> should be an easy job.
> 
> 
> This package will be approved when the two last issues fixed.

Are you blocking this review for an issue with the upstream code that can be easy worked around? I have contacted upstream and sent him the patch, but if he doesn't accept it I will not use it because of our commitment to upstream. Nevertheless this should not block a review. It's a package review and not a code review and as long as there are no security issues or severe bugs, this should not be considered a blocker I think.

(In reply to comment #9)
> This package uses glade mechanism, you'd better add pygtk2-libglade to
> requirement.

You don't need to have pygtk2-libglade installed to run cherrytree.

(In reply to comment #8)
> But I would suggest to not install into %{python_sitelib}, because upstream
> (and so othere distributions don't do that too, but your choice ;-)

Fine with me.

Comment 11 Robin Lee 2011-01-07 07:03:24 UTC
(In reply to comment #10)
> (In reply to comment #7)
> > * TODO: python2-devel is now preferred to python-devel.
> > https://fedoraproject.org/wiki/Packaging:Python#BuildRequires
> 
> AFAIK python-devel is a virtual provides that always defaults to the perferred
> version of python. IMHO we should not hardcode a specific version.
'python-devel' may change to be provided by 'python3-devel' at some time before cherrytree gets prepared for Python 3. So it's better to hardcode the exact version of Python required by cherrytree.


> (In reply to comment #9)
> > This package uses glade mechanism, you'd better add pygtk2-libglade to
> > requirement.
> 
> You don't need to have pygtk2-libglade installed to run cherrytree.
OK.

> 
> (In reply to comment #8)
> > But I would suggest to not install into %{python_sitelib}, because upstream
> > (and so othere distributions don't do that too, but your choice ;-)
> 
> Fine with me.
And which path world you choose?
I also prefer not to patch much the original source.
And please post your final specfile for review.

Comment 12 Christoph Wickert 2011-01-11 14:20:52 UTC
cherrytree 0.19.1 was just released. It contains more or less the same patch as Tomas proposed, see
http://code.google.com/p/giuspen-cherrytree/source/detail?r=2d2e8c770f15c86b2e3a12300380557526c1c29f

Upstream also confirmed that pygtk2-libglade is not necessary because he only uses gtkuilder and not libglade itself.

However I have a problem/question now: When using setup.py, a egg is created. What to do with it? Is it worth packaing when I install to /usr/share/cherrytree instead of %{python_sitelib}?

Changes:
* Tue Dec 11 2011 Christoph Wickert <cwickert> - 0.19.1-1
- Update to 0.19.1
- Use setup.py instead of manual installation
- BR python2-devel instead of python-devel

SPEC: http://cwickert.fedorapeople.org/review/cherrytree.spec
I will post a final package one I know what do do with the egg.

Comment 13 Robin Lee 2011-01-11 15:32:13 UTC
About eggs, refer to http://fedoraproject.org/wiki/Packaging:Python#Packaging_eggs_and_setuptools_concerns .

In the case of cherrytree, it is the second case of the above definitions of eggs.

Including the egg is a 'Should' but not a 'Must', and for cherrytree, the egg-info file is useless.

But if nothing was installed to %{python_sitelib}, the 'python(abi)' requirement will not be auto-generated.

After all, just include the egg.


New rpmlint result:
$ rpmlint ./cherrytree-0.19.1-1.fc14.noarch.rpm 
cherrytree.noarch: W: spelling-error %description -l en_US xml -> XML, cml, ml
cherrytree.noarch: W: spelling-error %description -l en_US ctd -> cts, ct, cd
cherrytree.noarch: W: devel-file-in-non-devel-package /usr/share/cherrytree/glade/cherrytree.glade.h
cherrytree.noarch: E: script-without-shebang /usr/share/applications/cherrytree.desktop
cherrytree.noarch: W: no-manual-page-for-binary cherrytree
1 packages and 0 specfiles checked; 1 errors, 4 warnings.

* TODO: The header file is included again. 
* TODO: Add 'chmod -x linux/cherrytree.desktop' to %prep and notify upstream.
And you may notify upstream of these issues.

Comment 14 Christoph Wickert 2011-01-17 11:22:17 UTC
Guiseppe was so kind as to fix all remaining issues in cherrytree 0.19.3:
- Python modules are installed into %{python_sitelib} by default
- Desktop file is not executable
- cherrytree.glade.h was dropped.

$ rpmlint ../RPMS/noarch/cherrytree-0.19.3-1.fc14.noarch.rpm 
cherrytree.noarch: W: spelling-error %description -l en_US xml -> XML, cml, ml
cherrytree.noarch: W: spelling-error %description -l en_US ctd -> cts, ct, cd
cherrytree.noarch: W: no-manual-page-for-binary cherrytree
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
$ rpmlint ../SRPMS/cherrytree-0.19.3-1.fc14.src.rpm 
cherrytree.src: W: spelling-error %description -l en_US xml -> XML, cml, ml
cherrytree.src: W: spelling-error %description -l en_US ctd -> cts, ct, cd
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

SPEC: http://cwickert.fedorapeople.org/review/cherrytree.spec
SRPM: http://cwickert.fedorapeople.org/review/cherrytree-0.19.3-1.fc14.src.rpm

Comment 15 Robin Lee 2011-01-17 12:54:31 UTC
Python modules are not actually installed into %{python_sitelib} in your RPM.

TODO: No need to explicitly set the python(abi) requirement.
TODO: Please clean up the whitespaces at the end of lines.
TODO: I think Group better to be 'Applications/Editors'.
TODO: Useless comment should be deleted.

Comment 16 Christoph Wickert 2011-01-17 13:51:29 UTC
(In reply to comment #15)
> Python modules are not actually installed into %{python_sitelib} in your RPM.

My bad, I looked at a wrong version of the 

> TODO: No need to explicitly set the python(abi) requirement.

Will do once I'm home.

> TODO: Please clean up the whitespaces at the end of lines.

Which ones?

> TODO: I think Group better to be 'Applications/Editors'.

Similar applications like gjots2, kjots and notecase are in Appications/Productivity, too.

> TODO: Useless comment should be deleted.

There is not a single comment in the spec. Are you sure you didn't look at a cached version?

Comment 17 Robin Lee 2011-01-17 14:00:20 UTC
(In reply to comment #16)
> (In reply to comment #15)

> > TODO: Please clean up the whitespaces at the end of lines.
> 
> Which ones?
The first two lines of the description.

> 
> > TODO: I think Group better to be 'Applications/Editors'.
> 
> Similar applications like gjots2, kjots and notecase are in
> Appications/Productivity, too.
OK.

> 
> > TODO: Useless comment should be deleted.
> 
> There is not a single comment in the spec. Are you sure you didn't look at a
> cached version?
#chmod -x linux/cherrytree.desktop

Comment 18 Thomas Spura 2011-01-19 07:36:11 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > TODO: Useless comment should be deleted.
> > 
> > There is not a single comment in the spec. Are you sure you didn't look at a
> > cached version?
> #chmod -x linux/cherrytree.desktop

$ whatis chmod
chmod                (1)  - change file mode bits

Are "file mode bits" related to "a cached version" or "a useless comment"?

Comment 19 Christoph Wickert 2011-01-19 08:03:10 UTC
Robin was right, there was indeed a comment because spec and srpm are not in sync. I will update the package later, but now I'm rather busy.

Comment 20 Christoph Wickert 2011-01-21 17:24:07 UTC
(In reply to comment #15)
> TODO: Please clean up the whitespaces at the end of lines.

These were there for a reason, but I removed them to make you feel better.

I also removed the useless egg. It's not even a SHOULD because the python files of this package cannot be used by something else. I added the python(abi) requirement manually.

SRPM: http://cwickert.fedorapeople.org/review/cherrytree-0.19.3-2.fc14.src.rpm
SPEC: http://cwickert.fedorapeople.org/review/cherrytree.spec

Comment 21 Robin Lee 2011-01-21 19:56:37 UTC
(In reply to comment #20)
> (In reply to comment #15)
> I also removed the useless egg. It's not even a SHOULD because the python files
> of this package cannot be used by something else. I added the python(abi)
> requirement manually.
OK.

TODO: With respect to deb/control and modules/core.py, this package depends on 7za, so please add /usr/bin/7za to requirement. I said this earlier.

TODO: %{_datadir}/application-registry/%{name}.applications and %{_datadir}/mime-info/%{name}.* are redundant and should be excluded. Just %{_datadir}/mime/packages/%{name}.xml and %{_datadir}/applications/%{name}.desktop are useful with the current xdg mimetype mechanism.

Refer to: http://standards.freedesktop.org/shared-mime-info-spec/shared-mime-info-spec-latest.html

TODO: Typos: 'unclude', 'manually ad'.

Comment 22 Christoph Wickert 2011-01-21 22:17:18 UTC
(In reply to comment #21)
> TODO: With respect to deb/control and modules/core.py, this package depends on
> 7za, so please add /usr/bin/7za to requirement. I said this earlier.

Indeed, you said this earlier, but I missed it due to the high number of comments. I don't want to  be impolite or sound ungrateful, but I would rather prefer a *full* review instead of new pitfalls with every comment. I think this would be easier and less time consuming for both me and you.

> TODO: %{_datadir}/application-registry/%{name}.applications and
> %{_datadir}/mime-info/%{name}.* are redundant and should be excluded. Just
> %{_datadir}/mime/packages/%{name}.xml and
> %{_datadir}/applications/%{name}.desktop are useful with the current xdg
> mimetype mechanism.

I agree that %{_datadir}/application-registry/ seems to be dead, but can we really be sure all applications in Fedora are using shared-mime-info?

Comment 23 Robin Lee 2011-01-22 05:32:22 UTC
(In reply to comment #22)
> (In reply to comment #21)
> > TODO: With respect to deb/control and modules/core.py, this package depends on
> > 7za, so please add /usr/bin/7za to requirement. I said this earlier.
> 
> Indeed, you said this earlier, but I missed it due to the high number of
> comments. I don't want to  be impolite or sound ungrateful, but I would rather
> prefer a *full* review instead of new pitfalls with every comment. I think this
> would be easier and less time consuming for both me and you.
> 
I am sorry. I will do better next time.

> > TODO: %{_datadir}/application-registry/%{name}.applications and
> > %{_datadir}/mime-info/%{name}.* are redundant and should be excluded. Just
> > %{_datadir}/mime/packages/%{name}.xml and
> > %{_datadir}/applications/%{name}.desktop are useful with the current xdg
> > mimetype mechanism.
> 
> I agree that %{_datadir}/application-registry/ seems to be dead, but can we
> really be sure all applications in Fedora are using shared-mime-info?
Most of DE's and FM's should have been using this mechanism.
Refer to: http://www.freedesktop.org/wiki/Specifications/shared-mime-info-spec?action=show&redirect=Standards%2Fshared-mime-info-spec#Currentstatus

The main point is that if %{_datadir}/application-registry/%{name}.applications is included, then this package must require gnome-mime-data, which provides the directory %{_datadir}/application-registry/ .

Comment 24 veron 2011-06-23 14:46:55 UTC
Anyone succeeded since January? I really badly need this program on Fedora 15 KDE. I tried to install from alien conversion but I failed. Even author himself 
http://www.giuspen.com/topic/rpm-for-fedora/
http://www.giuspen.com/cherrytree/

Comment 25 Robin Lee 2011-06-23 15:19:59 UTC
I am the reviewer but not the requester of this package.

But I just made a binary rpm based on the requester's specfile:
RPM: http://cheeselee.fedorapeople.org/cherrytree-0.22.1-1.fc15.noarch.rpm
SRPM: http://cheeselee.fedorapeople.org/cherrytree-0.22.1-1.fc15.src.rpm

Comment 26 veron 2011-06-23 15:40:35 UTC
Million of thanks. I'm a new user of Fedora. Forgive me my ignorance, but what can we do to make this program get into the official Fedora repository or at least RPM fusion. It is developed continuously and and new versions are released frequently.
On other distributions wishes are reported on their bugzilla sites. On Fedora there is 
https://fedoraproject.org/wiki/Package_maintainers_wishlist
but it looks look packages wait quite long to be made, besides they request real name to register.
Generally I wish I could have every new update of cherry tree available in repository for easy install. Is is possible at all in Fedora world?

Comment 27 Robin Lee 2011-06-23 15:56:48 UTC
This is the very request for getting cherrytree into Fedora repository. But the requester, or who wants to maintain cherrytree in Fedora, has not paid attention to this request for a while.

Or you can just join the Fedora packagers[1] if interested. And you can take over this request if the requester keeps unresponsive[2].

[1] https://fedoraproject.org/wiki/Join_the_package_collection_maintainers
[2] https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews#Submitter_not_responding

Comment 28 veron 2011-06-23 16:14:01 UTC
I'm not a programmer, just a new Fedora user. But I think this is a great and useful program and many other Fedora users will be satisfied if they only have it available.
I've been using it for a few months on Ubuntu and I must tell that I haven't found any better alternative. So, I may only confirm following the original requester that keeping it in repositories is a good idea.
I will write to the author on his forum and ask if he can engage and keep an eye on it.
cherrytree is also present here:
http://gtk-apps.org/content/show.php/CherryTree?content=113350

I don't understand this:
"And you can take over this request if the requester keeps unresponsive[2]."
What doesn't mean to "take over" and to review? To test it?

Comment 29 Robin Lee 2011-06-23 16:27:17 UTC
(In reply to comment #28)
> What doesn't mean to "take over" and to review? To test it?
Just mean that you have a way to take the maintainership of this package.

Comment 30 Christoph Wickert 2011-06-23 17:38:02 UTC
Sorry, I am extremely busy and I lost interest in this package. Nevertheless I updated it and fixed some more rpmlint warnings. Here is what I got:

SPEC: http://cwickert.fedorapeople.org/review/cherrytree.spec
SRPM: http://cwickert.fedorapeople.org/review/cherrytree-0.22.1-1.fc16.src.rpm

$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/cherrytree-0.22.1-1.fc16.*
cherrytree.noarch: W: spelling-error %description -l en_US ctd -> ct, std, cad
cherrytree.noarch: W: no-manual-page-for-binary cherrytree
cherrytree.src: W: spelling-error %description -l en_US ctd -> ct, std, cad
2 packages and 0 specfiles checked; 0 errors, 3 warnings.

I think the quickest is to finish this review but in long term I'd like to have somebody else (co-)maintain this package because I don't really use it.

(In reply to comment #23)
> The main point is that if %{_datadir}/application-registry/%{name}.applications
> is included, then this package must require gnome-mime-data, which provides the
> directory %{_datadir}/application-registry/ .

For the record: This is not correct, the file and directory ownership guidelines were revisited already in March 2010 and do allow multiple directory ownership to avoid unnecessary deps. Nevertheless I dropped %{_datadir}/{application-registry,mime-info} because nothing is using it anymore these days. It was last used in GNOME 2.18.

Comment 31 Robin Lee 2011-06-23 18:01:27 UTC
It seems you removed the python(abi) Requires which has been added in #20.

Comment 32 Christoph Wickert 2011-06-23 18:39:31 UTC
I thought rpm would add it automatically when I drop the egg into %{python_sitelib}. This didn't work but the manually added requirement doesn't work in mock anymore because get_python_version() returns nothing and rpm will fail building the srpm with 

Error: line 22: Version required: Requires:       python(abi) =

I guess python is no longer in the default build root, at least for the srpm. Any ideas how to get the dep versioned?

Comment 33 Robin Lee 2011-06-23 19:30:59 UTC
Created attachment 509599 [details]
make cherrytree get a python(abi) requirement properly

I met a similar situation before, and a method suggested by Toshio Kuratomi solved that problem[1].

But this one now seems worse, and we may need a trickier solution.


[1] https://bugzilla.redhat.com/show_bug.cgi?id=567348#c21

Comment 34 Robin Lee 2011-06-23 19:47:00 UTC
Created attachment 509600 [details]
make cherrytree get a python(abi) requirement properly

Shorter version

Comment 35 Christoph Wickert 2011-06-23 21:19:31 UTC
Thanks for these. I figured out that it was not the macro I used before but that is was placed within the %if clause. So I switched back and move the global 2 lines down.

SPEC: http://cwickert.fedorapeople.org/review/cherrytree.spec
SRPM: http://cwickert.fedorapeople.org/review/cherrytree-0.22.1-2.fc16.src.rpm

Comment 36 Robin Lee 2011-06-24 03:50:54 UTC
(In reply to comment #35)
> Thanks for these. I figured out that it was not the macro I used before but
> that is was placed within the %if clause. So I switched back and move the
> global 2 lines down.
> 
> SPEC: http://cwickert.fedorapeople.org/review/cherrytree.spec
> SRPM: http://cwickert.fedorapeople.org/review/cherrytree-0.22.1-2.fc16.src.rpm

That doesn't solve the issue, since Python isn't in the basic build root of F15 and Rawhide(F16).
Mock build failed: http://koji.fedoraproject.org/koji/taskinfo?taskID=3157539

Comment 37 Germán Racca 2011-09-30 13:56:15 UTC
I'd like to see this package in Fedora repos also...any progress?

Comment 38 Robin Lee 2011-11-02 06:17:20 UTC
If you intended to give up this review, can I take over this package? We have already worked a lot on it.

Comment 39 Christoph Wickert 2011-11-02 08:50:47 UTC
I have no intentions of maintaining this package, I don't use it myself. If you are willing to do it, go ahead.

Once you have opened a new review request mark this one a duplicate please.

Comment 40 Robin Lee 2011-11-03 02:27:44 UTC

*** This bug has been marked as a duplicate of bug 750961 ***


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