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

I've updated the spec file and srpm. All the pointed issues should be addressed.

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

(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

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 :)

(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

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.

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

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

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.

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.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/purple-telegram