Bug 784950

name: ok
summary: ok
license: ok
handling locale files: ok
rpmlint output: only plasmoid spelling warning

Not needed BuildRequires: desktop-file-utils and telepathy-qt4-devel, please rmove them in git.

APPROVED.

Sorry for interrupting here.

nucleo, are you familar with 
https://fedoraproject.org/wiki/Packaging:ReviewGuidelines ? 

I'd expect at least rpmlint output here. If this is to much work for you, you might use fedora-review -b bug-number , to produce a review, run a mock-build and rpmlint, etc.

You didn't note e.g. the following

[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install

I'm just observing, the form of this review is rather short and tends loose sight of probably existing packaging errors. I do not mistrust approving *this* package.

> You didn't note e.g. the following
>
> [!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install

Huh? There is no such requirement!

It is true that rm -rf %{buildroot} is no longer needed in current Fedora because RPM now does it automatically. However, it also cannot hurt and there is no guideline banning it (it is just no longer required in Fedora). It is also still required in EPEL. (This kind of changes to RPM do not get backported to existing RHEL releases.)

(In other words, your MUST is only a SHOULD for a Fedora-only package, and plain wrong if you want to support EPEL with the same specfile.)

Yepp, Kevin's right. MUST comes from fedora-review tool. There's no line stating this in Review Guidelines. 

Still remaining, those 9 lines of 'review' (including one blank and one containing the word 'approved') are rather short for a review and might not meet the Package Review Guidelines.

(In reply to comment #6)
> Still remaining, those 9 lines of 'review' (including one blank and one
> containing the word 'approved') are rather short for a review and might not
> meet the Package Review Guidelines.
Can you provide violated rules other that "short" review?

Review Guidelines list

Things to check on review [1]

In the yellow marked box: 
Items marked as MUST are things that the package (or reviewer) MUST do.

Best way to show, you checked them, is to write down the list and the results. 




[1] https://fedoraproject.org/wiki/Packaging:ReviewGuidelines#Things_To_Check_On_Review

(In reply to comment #8)
> Review Guidelines list
> 
> Things to check on review [1]
> 
> In the yellow marked box: 
> Items marked as MUST are things that the package (or reviewer) MUST do.
> 
> Best way to show, you checked them, is to write down the list and the results. 
> 
> 
> 
> 
> [1]
> https://fedoraproject.org/wiki/Packaging:ReviewGuidelines#Things_To_Check_On_Review

Please provide specific violated items.

rpmlint output:
ktp-contact-applet.i686: W: spelling-error Summary(en_US) plasmoid -> plasma
ktp-contact-applet.i686: W: spelling-error %description -l en_US plasmoid -> plasma
ktp-contact-applet.src: W: spelling-error Summary(en_US) plasmoid -> plasma
ktp-contact-applet.src: W: spelling-error %description -l en_US plasmoid -> plasma
3 packages and 0 specfiles checked; 0 errors, 4 warnings.

I read the list at [1] as follows: The reviewer is required to check each MUST
item. You'll get bonus points, if you also check SHOULD items.


MUST: If (and only if) the source package includes the text of the license(s)
in its own file, then that file, containing the text of the license(s) for the
package must be included in %doc.[4]
did you check tat?  I can't find that in your review

MUST: The spec file must be written in American English. [5]
I can't find that in your review. 

MUST: The spec file for the package MUST be legible. [6]
I can't find that in your review.

MUST: The sources used to build the package must match the upstream source, as
provided in the spec URL. Reviewers should use md5sum for this task. If no
upstream URL can be specified for this package, please see the Source URL
Guidelines for how to deal with this.

I can't find that in your review, that you checked that. 

Did you check macro-handling?  Is this somewhere written down in your review? 

(list intentionally incomplete)


[1]
https://fedoraproject.org/wiki/Packaging:ReviewGuidelines#Things_To_Check_On_Review

I checked all items related to this package.
You still not provided any violated item so I do not see the need to continue the discussion.

You're ignoring my points. I listed some of them. 

You claim, you had checked, but there no way to see, what you really have checked. I also could ask you, what did you check ans ask you, to enlist, what you did control. (And discussion starts again).

I think we agree that we disagree.

(In reply to comment #12)
> I checked all items related to this package.
> You still not provided any violated item so I do not see the need to continue
> the discussion.

$ spectool -g ktp-contact-applet.spec 
Getting http://download.kde.org/unstable/kde-telepathy/0.3.0/src/ktp-contact-applet-0.3.0.tar.bz2 to ./ktp-contact-applet-0.3.0.tar.bz2
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   372  100   372    0     0   5582      0 --:--:-- --:--:-- --:--:-- 23250
6712   372  6712 24971    0     0   161k      0 --:--:-- --:--:-- --:--:--  161k

$ file ktp-contact-applet-0.3.0.tar.bz2
ktp-contact-applet-0.3.0.tar.bz2: HTML document, ASCII text

The URL points to a mirror selecting site, but no direct download site, so scripts that verify, if upstream source and packaged source will fail.
(a better one would be e.g. ftp://ftp.kde.org/pub/kde/unstable/kde-telepathy/0.3.0/src/ktp-contact-applet-0.3.0.tar.bz2)

Did you check if sources match upstream? It's not obvious from the short review above.

Did you read my first message?
rpmlint gives error if URL wrong.
I wrote that rpmlint output have only spelling warning.
Is that not clear enough?

No more answers here.
If someone thinks that I approved package without check then you can ask for banning me.

re: comment #14
I've seen spectool -g fail before, but that's largely exposing bugs in the tool(s) it uses to download.  Put the URL into a browser, and it works.

Personally, I'd much prefer pointing our package URLs at upstreams preferred download methods (which employ mirrors) than rely on a single (and often slower) upstream server.


Now, as to the contentions as to the thoroughness of the review here.  Yes, it was probably a bit too brief.  Let me suggest that instead of pointing out where the review is lacking (and being negative), focus on how to make *this* review better.  For example, if you see something in the proposed package that's missing or needs improvement, please do mention that.

I'd argue any comment here that doesn't directly pertain to improving the quality of the package under review here, is in the least a bit off-topic, and at most, wasted and misdirected energy.

(In reply to comment #17)
> Now, as to the contentions as to the thoroughness of the review here.  Yes, it
> was probably a bit too brief.  Let me suggest that instead of pointing out
> where the review is lacking (and being negative), focus on how to make *this*
> review better.  For example, if you see something in the proposed package
> that's missing or needs improvement, please do mention that.

I think the only way, why the topic went off-topic here is, that Matthias wanted to have a bit more explicit review output, and nucleo considered it as some finer pointing to "ban" him (That would be his sponsor's decision).
A simple "will do next time" would be enough.

> I'd argue any comment here that doesn't directly pertain to improving the
> quality of the package under review here, is in the least a bit off-topic, and
> at most, wasted and misdirected energy.

+0.5
Pointing out what could be made better (in a polite/'be excellent' way) can also help other (future) reviews.


(In reply to comment #17)
> re: comment #14
> I've seen spectool -g fail before, but that's largely exposing bugs in the
> tool(s) it uses to download.  Put the URL into a browser, and it works.

Works == another click needed to finally download the source. ?
Was spectool ever working with such URLs?
I prefer "some URL" that works with wget or something like that, so you can automate it. A slow proper upstream server instead of a mirror would be best in this case.

e.g. A year ago, there was a script running to verify if all packages contain the upstream sources or a modified version of it. I think, there should be a URL that works with such a script.

One thing that leaps out immediately, IMO:

> Summary: Telepathy contact plasmoid
> 
> %description
> %{summary}.

Please really avoid making the description a copy of the summary, which usually doesn't build a full sentence. The description is supposed to expand on the summary.

[...]

https://fedoraproject.org/wiki/Packaging/Guidelines#summary

| The summary should be a short and concise description of the package.
| The description expands upon this.

[...]

With a comprehendible description (and summary) you also want to raise interest among users, who browse through package search results. A good description would even sum up what "Telepathy" is.

Insiders may consider the three words in the summary concise, but even with the insider knowledge what "Telepathy" is and what a "plasmoid" is, one wonders what this package does? What does it do related to Telepathy contacts? Display them or more? What is its primary feature? The description should tell.

Some other distributions mention "KDE" in the summary and avoid using the special term "plasmoid". I think that's smarter:

  Telepathy contact applet for KDE
  KDE Telepathy contact applet
  Plasma applet for displaying contacts

e.g.
KDE applet to handle Telepathy contacts

and the description could mention Plasma, if that is considered essential.

(In reply to comment #18)
> I think the only way, why the topic went off-topic here is, that Matthias
> wanted to have a bit more explicit review output, and nucleo considered it as
> some finer pointing to "ban" him (That would be his sponsor's decision).
> A simple "will do next time" would be enough.

> +0.5
> Pointing out what could be made better (in a polite/'be excellent' way) can
> also help other (future) reviews.

He made me responsible for errors in existing packages without particular examples - nice beginning, true? And all next comments was in the same way - just charges without examples of errors in this package.

> Works == another click needed to finally download the source. ?
> I prefer "some URL" that works with wget

Works means that rpmlint and wget can do all redirects to get source from that URL but spectool can't. And guidelines requires to run rpmlint on all packages - that exactly what I did and result of output wrote.

$ wget http://download.kde.org/unstable/kde-telepathy/0.3.0/src/ktp-contact-applet-0.3.0.tar.bz2
--2012-02-09 16:05:12--  http://download.kde.org/unstable/kde-telepathy/0.3.0/src/ktp-contact-applet-0.3.0.tar.bz2
Resolving download.kde.org... 2a01:4f8:140:7345:beef::1, 46.4.90.84
Connecting to download.kde.org|2a01:4f8:140:7345:beef::1|:80... connected.
HTTP request sent, awaiting response... 302 Found
Location: http://download.kde.org/download.php?url=unstable/kde-telepathy/0.3.0/src/ktp-contact-applet-0.3.0.tar.bz2 [following]
--2012-02-09 16:05:12--  http://download.kde.org/download.php?url=unstable/kde-telepathy/0.3.0/src/ktp-contact-applet-0.3.0.tar.bz2
Reusing existing connection to download.kde.org:80.
HTTP request sent, awaiting response... 302 Found
Location: ftp://ftp.gtlib.cc.gatech.edu/pub/kde/unstable/kde-telepathy/0.3.0/src/ktp-contact-applet-0.3.0.tar.bz2 [following]
--2012-02-09 16:05:12--  ftp://ftp.gtlib.cc.gatech.edu/pub/kde/unstable/kde-telepathy/0.3.0/src/ktp-contact-applet-0.3.0.tar.bz2
           => `ktp-contact-applet-0.3.0.tar.bz2'
Resolving ftp.gtlib.cc.gatech.edu... 2610:148:1f00:6f00:21b:24ff:fe1d:e940, 2610:148:1f00:6f00:216:36ff:fe8e:1308, 2610:148:1f00:6f00:216:36ff:fee9:2178, ...
Connecting to ftp.gtlib.cc.gatech.edu|2610:148:1f00:6f00:21b:24ff:fe1d:e940|:21... connected.
Logging in as anonymous ... Logged in!
==> SYST ... done.    ==> PWD ... done.
==> TYPE I ... done.  ==> CWD (1) /pub/kde/unstable/kde-telepathy/0.3.0/src ... done.
==> SIZE ktp-contact-applet-0.3.0.tar.bz2 ... 35519
==> EPSV ... done.    ==> RETR ktp-contact-applet-0.3.0.tar.bz2 ... done.
Length: 35519 (35K) (unauthoritative)

100%[=============================================================>] 35,519      74.0K/s   in 0.5s    

2012-02-09 16:05:15 (74.0 KB/s) - `ktp-contact-applet-0.3.0.tar.bz2' saved [35519]

(In reply to comment #19)
> One thing that leaps out immediately, IMO:
> 
> > Summary: Telepathy contact plasmoid
> > 
> > %description
> > %{summary}.
> 
> Please really avoid making the description a copy of the summary, which usually
> doesn't build a full sentence. The description is supposed to expand on the
> summary.
> 
> [...]
> 
> https://fedoraproject.org/wiki/Packaging/Guidelines#summary
> 
> | The summary should be a short and concise description of the package.
> | The description expands upon this.
> 
> [...]
> 
> With a comprehendible description (and summary) you also want to raise interest
> among users, who browse through package search results. A good description
> would even sum up what "Telepathy" is.
> 
> Insiders may consider the three words in the summary concise, but even with the
> insider knowledge what "Telepathy" is and what a "plasmoid" is, one wonders
> what this package does? What does it do related to Telepathy contacts? Display
> them or more? What is its primary feature? The description should tell.
> 
> Some other distributions mention "KDE" in the summary and avoid using the
> special term "plasmoid". I think that's smarter:
> 
>   Telepathy contact applet for KDE
>   KDE Telepathy contact applet
>   Plasma applet for displaying contacts
> 
> e.g.
> KDE applet to handle Telepathy contacts
> 
> and the description could mention Plasma, if that is considered essential.

Great, finally error in package found!

Thanks for all the extra feedback, I'll be sure to include it prior to doing any builds

New Package SCM Request
=======================
Package Name: ktp-contact-applet
Short Description: Telepathy contact plasmoid
Owners: jreznik rdieter
Branches: f16

Git done (by process-git-requests).

Added f17.

(In reply to comment #20)
> He made me responsible for errors in existing packages without particular
> examples - nice beginning, true? And all next comments was in the same way -

Plainly wrong. I didn't made you responsible for any error in any package.

I said, you might didn't notice an existing packaging error in a package 
doing such a short review. I can't see from outside, what you exactly did check. 

I must admit, just asking I you're familiar with packaging guidelines is not exactly "being excellent to each other". I promise, I'll try better next time!

I tried to be constructive and to point out, how to make this review better; All (or all -1) contributors here thinking, this special review is just too short.
My intention was, to outline, one can do it better, and how to improve. 
This point wasn't accepted by repeating, there's nothing wrong with the review.

imported into rawhide, and will look into polishing things once the full switchover from telepathy-kde -> ktp stuff is done.

*** Bug 757355 has been marked as a duplicate of this bug. ***