Bug 1291060 - Review Request: purple-telegram - adds support for Telegram to Pidgin
Review Request: purple-telegram - adds support for Telegram to Pidgin
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Jan Grulich
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2015-12-13 05:18 EST by Jiri Eischmann
Modified: 2016-08-23 01:44 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-08-14 11:41:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jgrulich: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Jiri Eischmann 2015-12-13 05:18:08 EST
Spec URL: https://dl.dropboxusercontent.com/u/1309518/purple-telegram.spec
SRPM URL: https://dl.dropboxusercontent.com/u/1309518/purple-telegram-1.2.2-1.fc23.src.rpm

Description:

Hi,
I've been maintaining purple-telegram in Copr for several months and now I'd like to ask for a review to get it to Fedora official repositories.

purple-telegram is a plugin that adds support for popular IM service Telegram to purple-based clients such as Pidgin.

Fedora Account System Username: eischmann
Comment 1 Michael Schwendt 2015-12-13 17:32:04 EST
A brief look:

Consider pointing the fedora-review tool at this ticket. It will download the latest package from the "Spec URL:" and "SRPM URL:" lines, perform a local Mock build and run lots of checks on the packages, which are of interest to all packagers not only during official package review.


> BuildRequires:	gettext,libgcrypt-devel,pkgconfig(zlib),pkgconfig(purple),pkgconfig(libwebp)

Note that while this line of BuildRequires works, it is considered much more readable and maintainable to split individual build requirements to separate lines, especially if there were many more BR, and also if you ever needed to delete one, doing that with individual lines is more convenient:

  BuildRequires: gettext
  BuildRequires: libgcrypt-devel
  BuildRequires: pkgconfig(zlib)
  BuildRequires: pkgconfig(purple)
  BuildRequires: pkgconfig(libwebp)


> %find_lang telegram-purple

You run %find_lang here without using its output file "telegram-purple.lang". You need to pass that file to the %files section:

  %files -f telegram-purple.lang

https://fedoraproject.org/wiki/Packaging:Guidelines#Why_do_we_need_to_use_.25find_lang.3F


> %{_libdir}/purple-2/telegram-purple.so

That directory is provided by libpurple, but then this plugin needs to do something about ownership of the "pidgin" icon directories further below:


> %config %{_sysconfdir}/telegram-purple/*

Directory  /etc/telegram-purple  is not included yet:
https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership


> #Icons
> %{_datadir}/pixmaps/pidgin/protocols/16/telegram.png

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


> #Translations
> %{_datadir}/locale/de/LC_MESSAGES/telegram-purple.mo

Due to not using %find_lang correctly.


> #AppStream metadata
> %{_datadir}/appdata/telegram-purple.metainfo.xml

https://fedoraproject.org/wiki/Packaging:Guidelines#AppData_files
https://fedoraproject.org/wiki/Packaging:AppData
Comment 2 Jiri Eischmann 2015-12-15 08:58:00 EST
I've updated the spec file and srpm. All the pointed issues should be addressed.
Comment 3 Michael Schwendt 2015-12-15 17:20:21 EST
> #Icons
> %dir %{_datadir}/pixmaps/pidgin/protocols/*/
> %{_datadir}/pixmaps/pidgin/protocols/*/telegram.png

The single %dir statement won't be enough as it doesn't cover the two dirs  %{_datadir}/pixmaps/pidgin and  %{_datadir}/pixmaps/pidgin/protocols.

You could include the full tree with this single line,

  %{_datadir}/pixmaps/pidgin/

which would also include any telegram.png files in there. Or be more specific with as many %dir lines as necessary.
Comment 4 Jiri Eischmann 2015-12-16 10:43:03 EST
(In reply to Michael Schwendt from comment #3)
> > #Icons
> > %dir %{_datadir}/pixmaps/pidgin/protocols/*/
> > %{_datadir}/pixmaps/pidgin/protocols/*/telegram.png
> 
> The single %dir statement won't be enough as it doesn't cover the two dirs 
> %{_datadir}/pixmaps/pidgin and  %{_datadir}/pixmaps/pidgin/protocols.
> 
> You could include the full tree with this single line,
> 
>   %{_datadir}/pixmaps/pidgin/

Ok, I went for this option and updated the spec file. The new SRPM is here:
https://dl.dropboxusercontent.com/u/1309518/purple-telegram-1.2.2-3.fc23.src.rpm
Comment 5 Matthias Jentsch 2015-12-18 10:05:39 EST
Hey, theres two little things I've noticed in your RPM:

- libgcrypt version dependency is >= 1.60, it will not work with a version lower than that.
- The package description "Purple plugin for Telegram" is kind of confusing IMO. This somehow indicates that its a plugin for Telegram, It should say "Libpurple protocol plugin for Telegram support" instead. This may be just my opinion though :)
Comment 6 Jiri Eischmann 2015-12-21 08:12:39 EST
(In reply to Matthias Jentsch from comment #5)
> Hey, theres two little things I've noticed in your RPM:
> 
> - libgcrypt version dependency is >= 1.60, it will not work with a version
> lower than that.

Fixed. This might be the reason why it doesn't build on RHEL 7 any more in Copr. It shouldn't be a problem for any supported release of Fedora.

> - The package description "Purple plugin for Telegram" is kind of confusing
> IMO. This somehow indicates that its a plugin for Telegram, It should say
> "Libpurple protocol plugin for Telegram support" instead. This may be just
> my opinion though :)

Also fixed, I don't have a problem with the proposed summary.

The linked spec file has been updated and new SRPM is here: https://dl.dropboxusercontent.com/u/1309518/purple-telegram-1.2.2-4.fc23.src.rpm
Comment 7 Jan Grulich 2015-12-21 08:37:47 EST
1) Group tag is obsoleted and not necessary
2) If that's a git snapshot or some pre-release you should name it according to https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages.
Comment 8 Jiri Eischmann 2015-12-21 09:05:33 EST
(In reply to Jan Grulich from comment #7)
> 1) Group tag is obsoleted and not necessary

Yeah, I know. On the other hand, I'd like the spec to be as useful for RHEL builds as possible. So as long as it's not a forbidden item, I'd like to keep it there.
 
> 2) If that's a git snapshot or some pre-release you should name it according
> to
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages.

It's not a git snapshot per se, I cloned the repo with the release tag. The releases are marked as betas, but it doesn't mean it's a beta of 1.2.2 release, they simply increment the release numbers and mark the releases beta until they're probably satisfied enough with it. Maybe Matthias Jentsch can clarify the numbering since he's the upstream developer.
Comment 9 Jiri Eischmann 2015-12-21 09:07:25 EST
The upstream spec also has it as 1.2.2 release without any pre-release marks: https://github.com/majn/telegram-purple/blob/master/rpm/purple-telegram.spec
Comment 10 Jan Grulich 2015-12-21 13:26:47 EST
Matthias, can you please clarify the versioning? Otherwise I think it's good to go as I don't see any blocker for this review.
Comment 11 Matthias Jentsch 2015-12-21 14:42:13 EST
Those tags were merely an "addition" to clarify that its a beta and which one it is, they didn't follow semantic versioning, 1.X.Y-betaZ are the actual versions.
Comment 12 Gwyn Ciesla 2015-12-23 04:55:41 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/purple-telegram

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