Bug 430070 - Review Request: evolution-rss - Evolution RSS Reader Plugin
Review Request: evolution-rss - Evolution RSS Reader Plugin
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-24 07:10 EST by Lucian Langa
Modified: 2008-03-28 13:06 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-02-19 04:06:51 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Lucian Langa 2008-01-24 07:10:49 EST
Spec URL: http://mips.edu.ms/evolution-rss.spec
SRPM URL: http://mips.edu.ms/evolution-rss-0.0.7-1.fc8.src.rpm
Description: This is an evolution plugin which enables evolution to read rss feeds.

This is my fisrt package and I need a I'm also seeking for a sponsor.
Comment 1 Mamoru TASAKA 2008-01-30 12:59:29 EST
cc-ing Dan. This won't build with libsoup 2.4 (rawhide)
http://koji.fedoraproject.org/koji/taskinfo?taskID=381907

On dist-f8-updates-candidate, it surely builds.
http://koji.fedoraproject.org/koji/taskinfo?taskID=381997

From quick glance at your spec file:
* Would you explain why this package should have
  "Requies: firefox"?
* Perhaps "Requires: dbus, dbus-glib" is redundant (and
  should be removed). rpmbuild checks the dependency for libraries
  and adds the dependency to rebuilt binary rpms, which
  should pull dbus, dbus-glib automatically
* "BuildRequires: gettext" is redundant if "BuildRequires:
  gettext-devel" is really needed.
* "BuildRequires: gcc-c++" is redundant (see the section
   "Exceptions" of
   http://fedoraproject.org/wiki/Packaging/Guidelines )
* "BuildRequires: dbus" is redundant. dbus-glib-devel
   requires dbus.
* Please try to use %configure macro
* GConf scriptlet is not right. Please refer to
  the section "GConf" of
  http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
* %_prefix/bin must be replaced with %_bindir.

By the way, now it seems your spec/srpm link seem 404 (not found)?
Comment 2 Lucian Langa 2008-01-30 14:42:47 EST
Thanks for your reply,


> cc-ing Dan. This won't build with libsoup 2.4 (rawhide)
> http://koji.fedoraproject.org/koji/taskinfo?taskID=381907

At this time libsoup-2.2 is required by configure

PKG_CHECK_MODULES(EVOLUTION_RSS_EPLUGIN,
  [libgnome-2.0 >= $LIBGNOME_REQUIRED dnl
   libgnomeui-2.0 >= $LIBGNOMEUI_REQUIRED dnl
   gtk+-2.0 >= $LIBGTK_REQUIRED dnl
   libsoup-2.2 >= $LIBSOUP_REQUIRED dnl

Building against 2.4 crashes the plugin and somehow related to
soup_message_get_header.


> From quick glance at your spec file:
> * Would you explain why this package should have
>   "Requies: firefox"?

Actually that is firefox-devel

> Requires: firefox-devel

Application requires gtkmozembed that is actually packed in firefox-devel.
Initially I thought I could provide two different packages gtkmozembed and
nogtkmozembed, but finally this could get confusing so I rather pack it using
gtkmozembed and the user can choose the renderer. That's why firefox-devel is
required.


> * Perhaps "Requires: dbus, dbus-glib" is redundant (and
>   should be removed). rpmbuild checks the dependency for libraries
>   and adds the dependency to rebuilt binary rpms, which
>   should pull dbus, dbus-glib automatically

dropped

> * "BuildRequires: gettext" is redundant if "BuildRequires:
>   gettext-devel" is really needed.

dropped gettext-devel as it is not really required
keep gettext for build requires

> * "BuildRequires: gcc-c++" is redundant (see the section
>    "Exceptions" of
>    http://fedoraproject.org/wiki/Packaging/Guidelines )

dropped

> * "BuildRequires: dbus" is redundant. dbus-glib-devel
>    requires dbus.

dropped

> * Please try to use %configure macro

updated

> * GConf scriptlet is not right. Please refer to
>   the section "GConf" of
>   http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

updated

> * %_prefix/bin must be replaced with %_bindir.

corrected

> By the way, now it seems your spec/srpm link seem 404 (not found)?

there are some local problems there with the connection, sorry for the inconvenience

I've also updated spec and srpm.
Comment 3 Dan Winship 2008-01-30 16:04:20 EST
(In reply to comment #1)
> cc-ing Dan. This won't build with libsoup 2.4 (rawhide)
> http://koji.fedoraproject.org/koji/taskinfo?taskID=381907

There is going to be a libsoup 2.2 compat package soon, since there are still
several other non-ported packages. (bug 430756)
Comment 4 Lucian Langa 2008-02-06 09:37:54 EST
> > * Would you explain why this package should have
> >   "Requies: firefox"?
> 
> Actually that is firefox-devel
> 
> > Requires: firefox-devel
> 
> Application requires gtkmozembed that is actually packed in firefox-devel.
> Initially I thought I could provide two different packages gtkmozembed and
> nogtkmozembed, but finally this could get confusing so I rather pack it using
> gtkmozembed and the user can choose the renderer. That's why firefox-devel is
> required.

I moved firefox-devel to: BuildRequires: firefox-devel
rpmbuild will check the dependency for libraries and will add the dependency
which should pull libgtkembedmoz correctly.
Comment 5 Lucian Langa 2008-02-08 09:18:29 EST
Please use the following URLs instead of the original ones:

Spec URL: http://gnome.eu.org/evolution-rss.spec
SRPM URL: http://gnome.eu.org/evolution-rss-0.0.7-1.fc8.src.rpm

I'm sorry for the inconvenience.
Comment 6 Mamoru TASAKA 2008-02-08 09:39:59 EST
Well, I am still waiting for bug 430978 ......
Comment 7 Lucian Langa 2008-02-17 05:00:17 EST
Since bug 430978 is taking forever, I added libsoup-2.4 patch for evoltion-rss.
I can now build it under rawhide.

Another thing is I've dropped firefox-devel (or xulrunner-devel) until xulrunner
is fixed.

The new files are:

SPEC: http://gnome.eu.org/evolution-rss.spec
SRPM: http://gnome.eu.org/evolution-rss-0.0.7-6.fc8.src.rpm
Comment 8 Mamoru TASAKA 2008-02-17 10:56:24 EST
* License issue
----------------------------------------------------
src/evolution-import-rss.c	GPLv2 (strict)
 -> %_bindir/evolution-import-rss is GPLv2

The other parts are GPLv2+
----------------------------------------------------
  Conclusion:
  - Following 
    http://fedoraproject.org/wiki/Packaging/LicensingGuidelines
    You should write as following in spec file:
----------------------------------------------------
..............
Group: Applications/Internet
License: GPLv2 and GPLv2+
..............
%files -f %{name}.lang
%defattr(-,root,root,-)
# Only the following binaries is under GPLv2. Other
# parts are under GPLv2+.
%{_bindir}/evolution-import-rss
%{_sysconfdir}/gconf/schemas/evolution-rss.schemas
..............
----------------------------------------------------


For 0.0.7-6:

* configure option
  - The following options are not needed for %configure.
----------------------------------------------------------------
--prefix="%{_prefix}" --sysconfdir="%{_sysconfdir}"  --libdir=%{_libdir} 
----------------------------------------------------------------
    You can check what %configure actually does by
    $ rpm --eval %configure .

* Timestamps
  - To keep timestamps on installed files, I recommend to use
----------------------------------------------------------------
make install DESTDIR="%{buildroot}" INSTALL="install -p"
----------------------------------------------------------------
    This method usually works for recent autotool-based Makefiles.

* ldconfig
  - Calling /sbin/ldconfig is not needed for %preun.

* rpmlint
  - You can use rpmlint (in rpmlint package) to detect some generic
    mistakes in your rpms.
    Currently:
----------------------------------------------------------------
evolution-rss.i386: W: non-conffile-in-etc /etc/gconf/schemas/evolution-rss.schemas
evolution-rss.i386: W: no-version-in-last-changelog
----------------------------------------------------------------
    - The first one can be ignored.
    - For second one:
      * Please also write the EVR in %changelog (please check
        http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs )
      * Also, I recommend to insert one blank line between each changelog
        entry like:
----------------------------------------------------------------
%changelog
* Sat Feb 16 2008 Lucian Langa <cooly@gnome.eu.org> - 0.0.7-6
- Drop gecko requirements till xulrunner is fixed

* Tue Feb 12 2008 Lucian Langa <lucilanga@gnome.org> - 0.0.7-5
- buildroot fixes
----------------------------------------------------------------
         This is useful when using Fedora CVS system.

* Some misc cleanup
  - For consistency, as you write
----------------------------------------------------------------
%pre
if [ "$1" -gt 1 ]; then
	export GCONF_CONFIG_SOURCE=`gconftool-2 --get-default-source`
	gconftool-2 --makefile-uninstall-rule \
		%{_sysconfdir}/gconf/schemas/%{name}.schemas >/dev/null || :
                                             ^^^^^^^^^^^^^^^
fi
----------------------------------------------------------------
     Please use %{name} in %files entry.
Comment 9 Lucian Langa 2008-02-17 11:46:00 EST
> * License issue
fixed

> * configure option
fixed

> * Timestamps
fixed

> * ldconfig
fixed

> * rpmlint
fixed

> * Some misc cleanup
fixed

updated spec and srpm to reflect changes.

Comment 10 Mamoru TASAKA 2008-02-17 11:53:49 EST
(In reply to comment #9)
> updated spec and srpm to reflect changes.

To where? (please change the release number every time you modify
your spec file to avoid confusion). 

Comment 12 Mamoru TASAKA 2008-02-17 12:50:57 EST
(In reply to comment #8)
> # Only the following binaries is under GPLv2. Other
Actually the following binary" is under GPLv2, sorry...

Well,
- This package itself is okay
- As written in
  http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored
  A person who wants to get sponsord is requested to either submit
  another review request or do a pre-review of other person's review
  request.
  For you it seems that your another review request (bug 431683)
  will be approved with a little more fixes.

---------------------------------------------------------------------------
   This package (evolution-rss) is APPROVED by me
---------------------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor. At the stage, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 7/8, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.
  
Comment 13 Mamoru TASAKA 2008-02-17 13:05:30 EST
Now I should be sponsoring you. Please follow "Join" wiki again.
Comment 14 Lucian Langa 2008-02-17 13:15:49 EST
Thank you.
Comment 15 Lucian Langa 2008-02-17 13:45:43 EST
New Package CVS Request
=======================
Package Name: evolution-rss
Short Description: This is an evolution plugin which enables evolution to read
rss feeds.
Owners: lucilaga
Branches: F-7 F-8
InitialCC: lucilanga
Cvsextras Commits: yes
Comment 16 Mamoru TASAKA 2008-02-17 13:52:24 EST
The item "Short Description" in CVS Request template is actually
"Summary" in the spec file.
Comment 17 Lucian Langa 2008-02-17 13:57:58 EST
.. I also mistyped the owner, in order to change to I need to file a "Package
Change Request" ? ..as cvs admin requests are processed per half an hour ?
Comment 18 Mamoru TASAKA 2008-02-17 21:25:16 EST
New Package CVS Request
=======================
Package Name: evolution-rss
Short Description: Evolution RSS Reader Plugin
Owners: lucilanga
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes
Comment 19 Kevin Fenzi 2008-02-18 12:30:28 EST
cvs done. 

cvsadmin requests are processed as cvs admins have time to do so. 
Comment 20 Lucian Langa 2008-03-27 14:01:13 EDT
Package Change Request
======================
Package Name: evolution-rss
New Branches: F-9
Comment 21 Kevin Fenzi 2008-03-28 13:06:04 EDT
cvs done.

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