Bug 751537

Summary: Review Request: gnome-shell-extension-weather - An extension for displaying weather notifications in GNOME Shell
Product: [Fedora] Fedora Reporter: Mattia M. <mattia.meneguzzo+fedora>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: alex, elad, hobbes1069, michal, mishu, notting, package-review, panemade, robatino, ssabchew, theo148
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-11-07 08:01:39 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Mattia M. 2011-11-05 10:45:26 UTC
Spec URL: http://dl.dropbox.com/u/39458594/Fedora%20packages/gnome-shell-extension-weather/gnome-shell-extension-weather.spec

SRPM URL: http://dl.dropbox.com/u/39458594/Fedora%20packages/gnome-shell-extension-weather/gnome-shell-extension-weather-0-0.1.git4681a05.fc16.src.rpm

Description:
gnome-shell-extension-weather is a simple extension for displaying weather notifications in GNOME Shell. Currently, the weather report including forecast for today and tomorrow is fetched from Yahoo! Weather.
This is my first package and I am seeking a sponsor.

Comment 1 Richard Shaw 2011-11-05 13:55:32 UTC
I actually packaged this for my own use back when F15 was released. The only reason I never submitted it was that the weather location had to be determined and set in a rather manual way. Has that been resolved?

If so I'd be willing to take this one but I'm not a sponsor yet.

Richard

Comment 2 Mattia M. 2011-11-05 15:53:09 UTC
(In reply to comment #1)
> I actually packaged this for my own use back when F15 was released. The only
> reason I never submitted it was that the weather location had to be determined
> and set in a rather manual way. Has that been resolved?
> 
> If so I'd be willing to take this one but I'm not a sponsor yet.
> 
> Richard

The settings (location, temperature units etc.) still have to be set through command line, as explained on the page https://github.com/simon04/gnome-shell-extension-weather under "Configuration".
But in my opinion this is not an issue at all.

Comment 3 Richard Shaw 2011-11-05 17:10:09 UTC
Ok, unofficial review since I can't sponsor you.

1. Always run rpmlint on the SRPM (and RPMs if you build them).

I got the following output:

$ rpmlint rpmbuild/gnome-shell-extension-weather/RPMS/noarch/gnome-shell-extension-weather-0-0.1.git4681a05.fc15.noarch.rpm 
gnome-shell-extension-weather.noarch: E: description-line-too-long C gnome-shell-extension-weather is a simple extension for displaying weather notifications in GNOME Shell.
gnome-shell-extension-weather.noarch: E: description-line-too-long C Currently, the weather report including forecast for today and tomorrow is fetched from Yahoo! Weather.

Basically you need to break your lines at 80 characters in the %description.

2. The following are not needed in your spec file and can be removed. (I would normally say, unless you going to build for EL5 or older versions of Fedora, but since this is for F16+, it's a given):

rm -rf %{buildroot} in %install
%defattr(-,root,root,-) in %files

3. There is a basic configuration gui in the tarball that's not installed by default. The files in the 3.0 branch are bad so I pulled them for Master for me but since it appears you're only planning to build for F16+ you should be able to use the ones in the tarball. I tried it out and it seemed to work. You still have to look up the WOEID manually but it has radio buttons for the other settings.

In %install I added:

install -D -pm 0755 weather-extension-configurator.py \
        %{buildroot}%{_bindir}/weather-extension-configurator.py

desktop-file-install \
        --dir=%{buildroot}%{_datadir}/applications \
        weather-extension-configurator.desktop

In %files I added:

%{_bindir}/weather-extension-configurator.py
%{_datadir}/applications/weather-extension-configurator.desktop


Richard

Comment 4 Mattia M. 2011-11-06 09:47:17 UTC
(In reply to comment #3)

Thanks for your review and precious suggestions.


> Ok, unofficial review since I can't sponsor you.
> 
> 1. Always run rpmlint on the SRPM (and RPMs if you build them).


Ops, I forgot that.


> I got the following output:
> 
> $ rpmlint
> rpmbuild/gnome-shell-extension-weather/RPMS/noarch/gnome-shell-extension-weather-0-0.1.git4681a05.fc15.noarch.rpm 
> gnome-shell-extension-weather.noarch: E: description-line-too-long C
> gnome-shell-extension-weather is a simple extension for displaying weather
> notifications in GNOME Shell.
> gnome-shell-extension-weather.noarch: E: description-line-too-long C Currently,
> the weather report including forecast for today and tomorrow is fetched from
> Yahoo! Weather.
> 
> Basically you need to break your lines at 80 characters in the %description.


Done.

 
> 2. The following are not needed in your spec file and can be removed. (I would
> normally say, unless you going to build for EL5 or older versions of Fedora,
> but since this is for F16+, it's a given):
> 
> rm -rf %{buildroot} in %install
> %defattr(-,root,root,-) in %files


Removed.

 
> 3. There is a basic configuration gui in the tarball that's not installed by
> default. The files in the 3.0 branch are bad so I pulled them for Master for me
> but since it appears you're only planning to build for F16+ you should be able
> to use the ones in the tarball. I tried it out and it seemed to work. You still
> have to look up the WOEID manually but it has radio buttons for the other
> settings.
> 
> In %install I added:
> 
> install -D -pm 0755 weather-extension-configurator.py \
>         %{buildroot}%{_bindir}/weather-extension-configurator.py
> 
> desktop-file-install \
>         --dir=%{buildroot}%{_datadir}/applications \
>         weather-extension-configurator.desktop
> 
> In %files I added:
> 
> %{_bindir}/weather-extension-configurator.py
> %{_datadir}/applications/weather-extension-configurator.desktop


Added.
I also added "python" to the "Requires:" section and the commands needed to compile the GSettings schemas (as suggested on the Fedora Wiki page http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#GSettings_Schema ).

Now rpmlint, executed on the .spec, the .src.rpm and the .rpm, returns:

gnome-shell-extension-weather.noarch: W: no-manual-page-for-binary weather-extension-configurator.py
2 packages and 1 specfiles checked; 0 errors, 1 warnings.


Here are the links to the new SPEC and SRPM files:

Spec URL: http://dl.dropbox.com/u/39458594/Fedora%20packages/gnome-shell-extension-weather_2/gnome-shell-extension-weather.spec

SRPM URL: http://dl.dropbox.com/u/39458594/Fedora%20packages/gnome-shell-extension-weather_2/gnome-shell-extension-weather-0-0.1.git1bb2dd6.fc16.src.rpm

Comment 5 Richard Shaw 2011-11-06 16:49:23 UTC
Yup, I realized it needed "Requires: python" after my last post, good catch.

I think you're in pretty good shape. Now you just need to get sponsored. 

A couple of ways to do that. You can package more software, especially stuff off the wishlist[1]. You can also perform unofficial reviews other review requests[2]. Be sure to state that it is in fact an unofficial review and that you're seeking sponsorship.

Richard

[1] http://fedoraproject.org/wiki/Package_maintainers_wishlist
[2] http://fedoraproject.org/PackageReviewStatus/NEW.html

Comment 6 Parag AN(पराग) 2011-11-07 05:07:11 UTC
I saw this review request on devel list and remembered about following
https://bugzilla.redhat.com/show_bug.cgi?id=718242#c1

Comment 7 Elad Alfassa 2011-11-07 08:01:39 UTC
Sorry, this package can not be included in Fedora.
from my response to bug #718242:

Sorry, but we asked fedora legal long ago, and, well, this extension can't be
included in fedora due to the license terms of the yahoo api.
see http://lists.fedoraproject.org/pipermail/legal/2011-May/001633.html for
more information.




-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 8 Elad Alfassa 2011-11-07 08:03:56 UTC
Perhaps someone should try getting this into rpmfusion-free...

Comment 9 Mattia M. 2011-11-07 08:34:45 UTC
(In reply to comment #8)
> Perhaps someone should try getting this into rpmfusion-free...

OK, I'll try to ask for its inclusion in RPMFusion.

Comment 10 Mattia M. 2011-11-07 15:31:04 UTC
Here is the review request for the inclusion of the package in RPMFusion: https://bugzilla.rpmfusion.org/show_bug.cgi?id=2017

Comment 11 Michal Jaegermann 2011-12-12 19:59:27 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > Perhaps someone should try getting this into rpmfusion-free...
> 
> OK, I'll try to ask for its inclusion in RPMFusion.

Regardless of an admisibility this package to Fedora it should be noted that an included 'weather-extension-configurator.py' is broken as it does not allow to change what it calls "woeid".  Regardless of what you type in a corresponding form field it has no effect.  Moreover despite of consistently calling it that way it expects no WOEID, like returned for example by http://woeid.rosselliot.co.nz/, but some other location code one has to divine somehow to change from a deafault Insbruck or to adjust to a new location.  Currently possible using 'gsettings' once you found such code.

Comment 12 Elad Alfassa 2011-12-12 20:02:36 UTC
(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Perhaps someone should try getting this into rpmfusion-free...
> > 
> > OK, I'll try to ask for its inclusion in RPMFusion.
> 
> Regardless of an admisibility this package to Fedora it should be noted that an
> included 'weather-extension-configurator.py' is broken as it does not allow to
> change what it calls "woeid".  Regardless of what you type in a corresponding
> form field it has no effect.  Moreover despite of consistently calling it that
> way it expects no WOEID, like returned for example by
> http://woeid.rosselliot.co.nz/, but some other location code one has to divine
> somehow to change from a deafault Insbruck or to adjust to a new location. 
> Currently possible using 'gsettings' once you found such code.

This is not the place to report this problem. Report it to the upstream maintainer.




-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 13 Jason Tibbitts 2013-04-24 18:35:04 UTC
*** Bug 822466 has been marked as a duplicate of this bug. ***