Bug 176982 - Review Request: RSSOwl - an RSS/RDS/Atom Newsreader
Review Request: RSSOwl - an RSS/RDS/Atom Newsreader
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Chris Chabot
David Lawrence
:
Depends On: 176981
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-01-04 17:50 EST by Anthony Green
Modified: 2007-11-30 17:11 EST (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-01-17 10:00:56 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 Anthony Green 2006-01-04 17:50:07 EST
Spec Name or Url: http://people.redhat.com/green/FE/devel/rssowl.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/rssowl-1.2-3.src.rpm

Description: 
RSSOwl is a RSS / RDF / Atom Newsreader written in Java using SWT as fast graphic library. Read News in a tabfolder, save favorites in categories, Export to PDF / RTF / HTML / OPML, Import Feeds from OPML, perform fulltext-search, use the integrated browser.
Comment 1 Andrew Overholt 2006-01-04 22:07:52 EST
There appear to be some changes in my spec file that I didn't publicize.  I've
put mine here if there's anything you want to grab:

http://overholt.ca/rssowl/rssowl.spec

Are you not going to natively-compile it?
Comment 2 Anthony Green 2006-01-04 22:40:40 EST
(In reply to comment #1)
> There appear to be some changes in my spec file that I didn't publicize.  I've
> put mine here if there's anything you want to grab:
> 
> http://overholt.ca/rssowl/rssowl.spec

Thanks - I'll have a look.
 
> Are you not going to natively-compile it?

I am.  Did I make a mistake?

Comment 3 Anthony Green 2006-01-05 10:13:42 EST
(In reply to comment #1)
> There appear to be some changes in my spec file that I didn't publicize.  I've
> put mine here if there's anything you want to grab:
> 
> http://overholt.ca/rssowl/rssowl.spec

It looks like the main change was to fix the arch reference in the startup script.
I had avoided that whole problem by referring to the swt jar file in
/usr/share/java.

Comment 4 Anthony Green 2006-01-05 22:18:46 EST
I've removed the obsolete MPL license text from the package.  This was only
included for the integrated itext.jar file, which I've packaged separately from
RSSOwl.

Spec Name or Url: http://people.redhat.com/green/FE/devel/rssowl.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/rssowl-1.2-4.src.rpm
Comment 5 Anthony Green 2006-01-13 22:22:53 EST
I've added a patch so RSSOwl now attempts (via java-gnome) to use the user's
default browser instead of always launching "mozilla".  Updated files here...

Spec Name or Url: http://people.redhat.com/green/FE/devel/rssowl.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/rssowl-1.2-5.src.rpm

Comment 6 Anthony Green 2006-01-16 10:12:26 EST
I've added a Requires to fix the libgconf-java runtime dependency (thanks
overholt!).

Spec Name or Url: http://people.redhat.com/green/FE/devel/rssowl.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/rssowl-1.2-6.src.rpm
Comment 7 Andrew Overholt 2006-01-16 10:14:48 EST
I built this RPM and have used it a bit over the past week or so on ppc.  Things
seem to work well and it all looks decent from my POV.
Comment 8 Chris Chabot 2006-01-16 13:05:58 EST
I'll pick this one up for review.

It builds cleanly, however i've not been able to mock build this package since
itext hasn't been included in fedora(-extras) yet, but it looks like it should.

Functionally it works well for me too.

Spec file needs work though, rpmlint becomes mad over a few things:
E: rssowl explicit-lib-dependency libgconf-java
This is an ignorable error, even though we wish it did, rpm doesn't autodepend
this properly, so explicit require should stay. No fix needed

E: rssowl description-line-too-long RSSOwl is a RSS / RDF / Atom Newsreader
written in Java using SWT as fast graphic library. Read News in a tabfolder,
save favorites in categories, Export to PDF / RTF / HTML / OPML, Import Feeds
from OPML, perform fulltext-search, use the integrated browser. Please contact
bpasero@rssowl.org if you have any questions or problems regarding this version.

Description lines should be wraped at 80 (or even 78) characters, so it fits on
any console, please add some wrapping to this text :-)

W: rssowl non-standard-group Productivity/Networking/News
Please pick one of FE's standard groups and stick to it :-) 
http://fedoraproject.org/wiki/RPMGroups

My suggestion would be "Applications/Internet" for this package.

W: rssowl incoherent-version-in-changelog 1.2-6fc 1.2-6
Version entry should be without the 'fc' in the changelog

W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/popups.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_usersets.html
W: rssowl wrong-file-end-of-line-encoding /usr/share/doc/rssowl-1.2/faq.xml
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/styles/rssowldocs.css
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_webfeeds.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_subscrips.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_ampheta.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/index.html
W: rssowl wrong-file-end-of-line-encoding /usr/share/doc/rssowl-1.2/LICENSE.txt
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_proxy.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_offline.html
W: rssowl wrong-file-end-of-line-encoding /usr/share/doc/rssowl-1.2/README.txt
W: rssowl wrong-file-end-of-line-encoding /usr/share/doc/rssowl-1.2/faq.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/menus.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_browser.html
W: rssowl wrong-file-end-of-line-encoding /usr/share/doc/rssowl-1.2/CHANGELOG.txt
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/quickstart.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/resources.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/dialogs.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_reload.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/rssowl_i18n.template
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/howto.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_searchagg.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/tipstricks.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_manage.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/ht_eximprint.html
W: rssowl wrong-file-end-of-line-encoding
/usr/share/doc/rssowl-1.2/tutorial/en/elements.html

Please add dos2unix (as in the package name) to build requires, and run dos2unix
in the spec file on those files; dos2unix does allow * in the files to convert
so you could do:
 dos2unix %{_docdir}/rssowl-1.2/tutorial/en/*.html
 dos2unix %{_docdir}/rssowl-1.2/tutorial/en/styles/*
 dos2unix %{_docdir}/rssowl-1.2/tutorial/en/*.html
 dos2unix %{_docdir}/rssowl-1.2/*.{xml,html,txt,template}}
inside the specfile (as last part of the %install section)

W: rssowl one-line-command-in-%post /usr/bin/rebuild-gcj-db
W: rssowl one-line-command-in-%postun /usr/bin/rebuild-gcj-db
You can fix these by making it:
%post -p                                                                       
                       %{_bindir}/rebuild-gcj-db

E: rssowl standard-dir-owned-by-package /usr/share/icons
(Actually same goes for applications and pixmaps but rpmlint doesn't check these).

Please change to atleast:
%{_datadir}/applications                                                       
                         /*
%{_datadir}/pixmaps/*                                                          
                         
%{_datadir}/icons/*

So the package doesn't end up owning the (fedora/freedesktop standard) directories.

Please also change the summary, shouldn't start with the package name, better
would be: "Free RSS / RDF / Atom Newsreader". Or you could even omit the "Free"
part, because it being included in fedora extra's implicitly implies that it IS
free, there's no pay-for or not open source software in fedora extra's, so this
"free" description applies for every package in FE :-) So even better would be:
"An RSS, RDF and Atom Newsreader"

For the rest it looks good. Please correct the above described errors and
warnings and i'll go thru the complete formal checklist.

Changing blocker bug to FE-REVIEW. Feel free to assign the bug to me (not in
fedorabugs group yet so can't do this for you) 
Comment 9 Chris Chabot 2006-01-16 13:14:09 EST
I see itext has been posted for review, but no one has set it to FE-REVIEW yet.
I presume katzj@redhat.com is still on this or should i take it over to allow
this package to complete too?

Adding depends for itext bug to this one.
Comment 10 Chris Chabot 2006-01-16 13:23:13 EST
Umm in comment #8 the line wrapping went a bit crazy (charset confusion?), few
corrections:
About files section should read:
%{_datadir}/icons/*
%{_datadir}/pixmaps/*
%{_datadir}/applications/*

And about %post/postrun should read:
%post -p %{_bindir}/rebuild-gcj-db
%postrun -p %{_bindir}/rebuild-gcj-db

Additional note i missed in the previous post):
%clean section should read (according to FE standards):
rm -rf $RPM_BUILD_ROOT 
Comment 11 Anthony Green 2006-01-16 15:09:41 EST
(In reply to comment #8)
> I'll pick this one up for review.

Thanks!  I've made all your requested changes and more...

Spec Name or Url: http://people.redhat.com/green/FE/devel/rssowl.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/rssowl-1.2-7.src.rpm

In addition to your suggestions, I've fixed the doc and .jar file installation
locations (from /usr/share/rssowl to /usr/share/doc/rssowl-1.2 and /usr/share/java).
Comment 12 Chris Chabot 2006-01-16 15:44:05 EST
Ok all the items from my checklist have been fixed

However one thing i overlooked unfortunatly because it was hidden in the sources
list is the .desktop file (Source2) is missing the fedora vendor, and the
X-Fedora category. Its supposed to be installed using desktop-file-utils:

desktop-file-install --vendor fedora                            \
        --dir ${RPM_BUILD_ROOT}%{_datadir}/applications         \
        --add-category X-Fedora                                 \
        %{SOURCE2}

And in the %files section remeber the file will now be called:
fedora-rssowl.desktop

And add "BuildRequires: desktop-file-utils" please

More info here:
http://fedoraproject.org/wiki/PackagingGuidelines#desktop

Please fix that one last remaining issue and we are ready for FE-ACCEPT
Comment 13 Anthony Green 2006-01-16 16:01:51 EST
(In reply to comment #12)
> Ok all the items from my checklist have been fixed
> 
> However one thing i overlooked unfortunatly because it was hidden in the sources
> list is the .desktop file (Source2) is missing the fedora vendor, and the
> X-Fedora category. Its supposed to be installed using desktop-file-utils:

Fixed, thanks.

Spec Name or Url: http://people.redhat.com/green/FE/devel/rssowl.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/rssowl-1.2-8.src.rpm

Comment 14 Chris Chabot 2006-01-16 16:47:02 EST
Formal review list:

MUST review items:
- Builds cleanly on FC5 devel.
- Source included matches upsteam source (md5sum)
- Package name meets guidelines
- spec file name is in %{name}.spec format
- Licence is fedora extra's compatible & is included
- Spec file is in (american) english
- Does not list buildrequires that are excepted in the package guidelines
- All build dependencies are listed
- No ldconfig needed
- All files have proper permissions
- Package is not relocatable
- No duplicate files in %files section
- No missing files in %files section
- Has a proper %clean section with rm -rf $RPM_BUILD_ROOT
- Uses macro's described in PackagingGuidelines
- No entries in %doc that are required for standard program operation
- No -devel package needed
- Proper directory-ownerships
- Proper desktop file & install

Should items:
- Includes upstream licence(s) file
- No insane scriplets
- No unnescesarry requires

Only thing i could not verify of the SHOULD list is the mock build, can't build
this in mock without itext being in extras-development, or to much effort
setting up a local repo to verify, but it looks like it will, so:

FE-ACCEPTED

Thanks for the great & quick work!
Comment 15 Anthony Green 2006-01-16 17:05:26 EST
(In reply to comment #9)
> I see itext has been posted for review, but no one has set it to FE-REVIEW yet.
> I presume katzj@redhat.com is still on this or should i take it over to allow
> this package to complete too?

Please take over if you don't mind.  I think the only reason katzj started
reviewing it is because he felt bad for me. :-)

Thanks,

AG

Comment 16 Jeremy Katz 2006-01-16 18:48:23 EST
Chris -- feel free to take it over.  I mostly wanted to start the ball rolling
and had a few upfront questions.  I was hoping to get back to it the end of last
week, but getting test2 out took more effort than I was expecting and I'm at a
conference this week, so doubt I would get back to looking until next week.
Comment 17 Paul Howarth 2006-01-17 03:39:10 EST
(In reply to comment #14)
> Only thing i could not verify of the SHOULD list is the mock build, can't build
> this in mock without itext being in extras-development, or to much effort
> setting up a local repo to verify

It's actually very easy to set this up; all you need is "createrepo".

Just create somewhere to hold your packages (e.g. a mock-built itext package),
let's say /var/lib/mock/local

# mkdir -p /var/lib/mock/local/RPMS
# cp itext*.rpm /var/lib/mock/local/RPMS
# cd /var/lib/mock/local
# createrepo .

Then edit /etc/mock/fedora-development-i386-core.cfg (or whichever config you're
using) and add this to the end:

[localrepo]
name=localrepo
baseurl=file:///var/lib/mock/local

That should be all you need.
Comment 18 Anthony Green 2006-01-17 10:00:56 EST
Thanks - I'm closing this now.

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