Bug 483187 - Review Request: kde-plasma-weather - Plasma applet for weather forecasts
Review Request: kde-plasma-weather - Plasma applet for weather forecasts
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-30 00:18 EST by Kevin Kofler
Modified: 2009-02-03 14:11 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-03 14:11:15 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Kevin Kofler 2009-01-30 00:18:54 EST
Spec URL: http://repo.calcforge.org/f10/kde-plasma-weather.spec
SRPM URL: http://repo.calcforge.org/f10/kde-plasma-weather-0.0-0.2.fc9.20090130svn.src.rpm
Description: A plasmoid showing weather reports and forecasts.

Note: You need KDE 4.2 to build and/or test this.

Scratch build for Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=1092688
Comment 1 Orcan Ogetbil 2009-02-01 01:31:14 EST
Here are my notes:

! rpmlint says:
    kde-plasma-weather.x86_64: W: no-documentation
but there is really no documentation, so this can be ignored

? The package must be named according to the Package Naming Guidelines. Upstream webpage says:
   "For those looking for the weather forecast plasmoid (that's the official name of it), it's now..."

So shall we call this package "kde-plasma-weatherforecast" instead?

* please make use of the %{name} macro.

* According to the guidelines
    http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Pre-Release_packages
the pre-release packages are versioned in a special way. Check the "kismet" example given in the above link. So it would probably better to change
    Version:        0.0
    Release:        0.2%{?dist}.20090130svn
to 
    Version:        0
    Release:        0.2.20090130svn%{?dist}

The right-hand side of the disttag is to be used when there is a fix in an older branch, e.g. F-9

* Isn't plasma-devel provided by kdelibs-devel which is one of the dependencies of kdebase-workspace-devel? So, that BR seems redundant.

* Fedora-specific flag -fexceptions is overriden by -fno-exceptions but I think this is a problem with the compilation of kde itself. Can this be fixed?

* Do we really need call ldconfig on post{,un} ?
Comment 2 Kevin Kofler 2009-02-01 08:52:02 EST
> So shall we call this package "kde-plasma-weatherforecast" instead?

It's under extragear/base/plasma/applets/weather in KDE SVN, that's where the name "weather" comes from.

> * please make use of the %{name} macro.

What for? It's not like the name is going to change frequently. That's just useless use of macros which doesn't help anyone.

>    Version:        0

I thought the recommended version to use for checkouts without a version was 0.0, maybe it changed or maybe I just remembered wrong. (I see they use just 0 in Packaging:NamingGuidelines.) But if I change it from 0.0 to 0 now, this will break the upgrade path for those who already installed the package from kde-redhat testing. 0.0 will upgrade just fine to any version number they'll actually use.

>    Release:        0.2.20090130svn%{?dist}

OK, I can put the %{?dist} at the end, not sure why I put it before, probably just a typo. (Not that the guidelines clearly say that 0.2%{?dist}.20090130svn is invalid, but I agree that 0.2.20090130svn%{?dist} makes more sense.)

> * Isn't plasma-devel provided by kdelibs-devel which is one of the dependencies
> of kdebase-workspace-devel? So, that BR seems redundant.

What if we make a separate plasma-devel one day?
But those "redundant" BRs have also been removed from other packages so I can remove it from this one too.

> * Fedora-specific flag -fexceptions is overriden by -fno-exceptions but I think
> this is a problem with the compilation of kde itself. Can this be fixed?

It's a KDE "feature". I can bring this up at the next KDE SIG meeting. But does it really matter for this package? It makes sense for a library

> * Do we really need call ldconfig on post{,un} ?

Good question. I think we don't, as there's no public shared library, only a plugin in %{_kde4_libdir}/kde4. I'll remove it.
Comment 3 Kevin Kofler 2009-02-01 08:54:39 EST
Actually, I'll replace Version: 0.0 with a real version number, I just asked the upstream author what to use. I also asked him whether he prefers kde-plasma-weather or kde-plasma-weatherforecast.
Comment 4 Kevin Kofler 2009-02-01 08:59:17 EST
Oh, I didn't finish my sentence: I was saying: it makes sense for a library to require exception-safety (and thus -fexceptions) because users of the library may want to use exceptions, but for an application or a plasmoid, it's pretty much irrelevant. And in any case it's not an issue with this package, but with the general KDE default flags.
Comment 5 Kevin Kofler 2009-02-01 09:06:15 EST
Waiting for feedback from upstream:
<Kevin_Kofler> spstarr_work: What's the official version number of the weather forecast plasmoid?
<Kevin_Kofler> 1.0 prerelease (i.e. 1.0-0.1.2009xxxxsvn)?
<Kevin_Kofler> And should it be called kde-plasma-weather or kde-plasma-weatherforecast?
Comment 6 Orcan Ogetbil 2009-02-01 12:51:53 EST
(In reply to comment #2)
> > * please make use of the %{name} macro.
> 
> What for? It's not like the name is going to change frequently. That's just
> useless use of macros which doesn't help anyone.
> 

I've been told in many reviews that %{name} should be used extensively, except a couple places like the URL. Even now, since there's a possibility that the package name may be changed, it might come handy.

(In reply to comment #3)
> Actually, I'll replace Version: 0.0 with a real version number, I just asked
> the upstream author what to use. I also asked him whether he prefers
> kde-plasma-weather or kde-plasma-weatherforecast.

alright. let's wait.
Comment 7 Rex Dieter 2009-02-01 13:01:45 EST
> I've been told in many reviews that %{name} should be used extensively

Did this advice have basis in fedora's packaging guidelines?  If so, where?

All I'm aware of, is that macros should be used consistently, ie, avoid using %{name} in some places and hard-code it others.
Comment 8 Orcan Ogetbil 2009-02-01 13:20:02 EST
(In reply to comment #7)
> > I've been told in many reviews that %{name} should be used extensively
> 
> Did this advice have basis in fedora's packaging guidelines?  If so, where?
> 

I'm not 100% sure that it is in the guidelines (I never found it). But I have been told this so many times by different reviewers, that I took it as an unwritten convention, or maybe it is so deeply hidden inside the guidelines that nobody remembers where it is... or it is just a legend.

If it is an unwritten convention, sure, you don't have to obey it. But it would be nice if you do.

side note: Another thing that I've been told many times to use in a very specific unique way and that I couldn't find in the guidelines is %defattr(-,root,root,-)

> All I'm aware of, is that macros should be used consistently, ie, avoid using
> %{name} in some places and hard-code it others.

Well, strictly speaking, since %{name} has to be used in the BuildRoot, any other usage will cause an inconsistency.
Comment 9 Shawn Starr 2009-02-01 13:31:22 EST
Well, the plasmoid is "Weather Forecast" but we didn't change the .desktop file to reflect this.

X-KDE-Library=plasma_applet_weather 

You could call the RPM kde-plasma-weatherforecast but the installed plasmoid is 'plasma_applet_weather'

Version is '1.0' for KDE 4.2.
Comment 10 Orcan Ogetbil 2009-02-01 13:46:11 EST
I'm not sure if this is related to packaging. 

The image on the left-hand side of the applet does not look right. No matter how the weather is, it shows me a large black Ø contained in a larger white rectangle.

Is this what this plasmoid supposed to show?
Comment 11 Kevin Kofler 2009-02-01 14:05:22 EST
> side note: Another thing that I've been told many times to use in a very
> specific unique way and that I couldn't find in the guidelines is
> %defattr(-,root,root,-)

That one is because the files must be owned by root. Without this, they can be owned by some other user depending on how the RPM package is built.

> The image on the left-hand side of the applet does not look right. No matter
> how the weather is, it shows me a large black Ø contained in a larger white
> rectangle.

This is because the BBC reports the current weather conditions as N/A for several locations. Apparently they don't get reports of weather conditions, only temperature and wind. So we get a huge N/A icon. :-(
Comment 12 Kevin Kofler 2009-02-01 14:12:58 EST
* Sun Feb 01 2009 Kevin Kofler <Kevin@tigcc.ticalc.org> 1.0-0.1.20090130svn
- Set Version to 1.0
- Fix Release (put alphatag before disttag)
- Remove redundant BR plasma-devel
- Don't call ldconfig, not needed
- Use %%{name} macro

Spec URL: http://repo.calcforge.org/f10/kde-plasma-weather.spec
New SRPM URL: http://repo.calcforge.org/f10/kde-plasma-weather-1.0-0.1.20090130svn.fc9.src.rpm
Comment 13 Orcan Ogetbil 2009-02-01 14:25:17 EST
(In reply to comment #11)
> > side note: Another thing that I've been told many times to use in a very
> > specific unique way and that I couldn't find in the guidelines is
> > %defattr(-,root,root,-)
> 
> That one is because the files must be owned by root. Without this, they can be
> owned by some other user depending on how the RPM package is built.
> 
I was more considered about the "-"s. I don't know why
   %defattr(-,root,root)
or
   %defattr(0644,root,root,0755)
is "bad" (even for RPMs for which one doesn't need to assign exotic permissions)


> > The image on the left-hand side of the applet does not look right. No matter
> > how the weather is, it shows me a large black Ø contained in a larger white
> > rectangle.
> 
> This is because the BBC reports the current weather conditions as N/A for
> several locations. Apparently they don't get reports of weather conditions,
> only temperature and wind. So we get a huge N/A icon. :-(
That's what I suspected. The weather in this s.hole where I reside is so cold that BBC doesn't know how to picture it.


Anyways, the package is good to go now. (One last thing, just an idea to clear possible confusions in the future): you can add to the description that this plasmoid is planned to be included in KDE 4.3.)

-----------------------------------------------------
This package (kde-plasma-weather) is APPROVED by oget
-----------------------------------------------------
Comment 14 Kevin Kofler 2009-02-01 14:46:37 EST
New Package CVS Request
=======================
Package Name: kde-plasma-weather
Short Description: Plasma applet for weather forecasts
Owners: kkofler spstarr
Branches: 
InitialCC: tuxbrewr
Comment 15 Kevin Kofler 2009-02-01 14:48:47 EST
Oops, forgot to request branches.
Comment 16 Kevin Kofler 2009-02-01 14:49:17 EST
New Package CVS Request
=======================
Package Name: kde-plasma-weather
Short Description: Plasma applet for weather forecasts
Owners: kkofler spstarr
Branches: F-9 F-10
InitialCC: tuxbrewr
Comment 17 Kevin Fenzi 2009-02-02 23:10:42 EST
cvs done.
Comment 18 Shawn Starr 2009-02-03 12:46:39 EST
The spec file will be changed today. I am getting an official extragear tarball instead of pulling from SVN. We will use this for 4.2.x series, 4.3 will be using kdeplasma-addons.
Comment 19 Shawn Starr 2009-02-03 14:11:15 EST
This is now in the package conveyor belt for rawhide

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