Bug 483187

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} ?

> 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.

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.

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.

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?

(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.

> 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.

(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.

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.

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?

> 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. :-(

* Sun Feb 01 2009 Kevin Kofler <Kevin.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

(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
-----------------------------------------------------

New Package CVS Request
=======================
Package Name: kde-plasma-weather
Short Description: Plasma applet for weather forecasts
Owners: kkofler spstarr
Branches: 
InitialCC: tuxbrewr

Oops, forgot to request branches.

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

cvs done.

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.

This is now in the package conveyor belt for rawhide