Bug 1138980 - Review Request: perl-Gtk2-AppIndicator - Perl extension for libappindicator
Review Request: perl-Gtk2-AppIndicator - Perl extension for libappindicator
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Emmanuel Seyman
Fedora Extras Quality Assurance
:
Depends On:
Blocks: ARM64/F-ExcludeArch-aarch64
  Show dependency treegraph
 
Reported: 2014-09-07 01:28 EDT by Remi Collet
Modified: 2014-10-03 23:23 EDT (History)
5 users (show)

See Also:
Fixed In Version: perl-Gtk2-AppIndicator-0.15-4.fc19
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-09-27 06:11:07 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
emmanuel: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Remi Collet 2014-09-07 01:28:21 EDT
Spec URL: https://raw.githubusercontent.com/remicollet/remirepo/789f248bf6e91acfb023bad05d14e8bd3b12e840/perl-Gtk2-AppIndicator/perl-Gtk2-AppIndicator.spec
SRPM URL: http://rpms.famillecollet.com/SRPMS/perl-Gtk2-AppIndicator-0.15-1.remi.src.rpm
Description: 
Perl extension for libappindicator.

Fedora Account System Username: remi

Target: fedora only (epel don't have libappindicator)

This is a new optional dependency of gmusicbrowser 1.1.13.
Comment 1 Remi Collet 2014-09-07 01:45:11 EDT
For test, gmusicbrowser 1.1.13 is in rawhide,
just need to fix with_appindicator macro.
Comment 2 Ralf Corsepius 2014-09-07 02:08:20 EDT
Some remarks:

These should be added as "BuildRequires:"
perl(AutoLoader)
perl(Carp)
perl(Exporter)
perl(Gtk2)
perl(XSLoader)
perl(strict)  
perl(warnings)


The package must own %{perl_vendorarch}/Gtk2:
Please change %{perl_vendorarch}/Gtk2/* in %files into %{perl_vendorarch}/Gtk2
Perl-modules are supposed to own all directories they are using below %{perl_vendorarch}.


Please check if this line is needed:
find %{buildroot} -depth -type d -exec rmdir {} 2>/dev/null \; -print

This is a work around to a defect which had existed in older Perls, but in most cases isn't needed anymore.
Comment 3 Remi Collet 2014-09-07 02:23:36 EDT
> Perl-modules are supposed to own all directories ...
I will never understand why this exception to common Guidelines still exists... It have sense when %{perl_vendorarch} was versionned... but now ?

Changes: https://github.com/remicollet/remirepo/commit/9c56237ee002b591b28e16557ffbaa524da1d5c7

Spec: https://raw.githubusercontent.com/remicollet/remirepo/9c56237ee002b591b28e16557ffbaa524da1d5c7/perl-Gtk2-AppIndicator/perl-Gtk2-AppIndicator.spec
Srpm: http://rpms.famillecollet.com/SRPMS/perl-Gtk2-AppIndicator-0.15-2.remi.src.rpm
Comment 4 Ralf Corsepius 2014-09-07 02:45:23 EDT
(In reply to Remi Collet from comment #3)
> > Perl-modules are supposed to own all directories ...
> I will never understand why this exception to common Guidelines still
> exists... It have sense when %{perl_vendorarch} was versionned... but now ?

And I will never understand why Fedora has not adopted this on a wider scale, because the need to track parent packages is the origin of numerous installation/uninstallation issues in Fedora.


Anyway, the answer is quite simply: KISS.

Most perl modules no not have strict dependencies on parent packages, which means there also are no strict deps on parent directories. To avoid getting lost in dir-ownership problems, when perl-modules change, we once agreed to generalize for reasons of simplicity and treat all perl-modules as "plugins".

Admitted, in case of this particuliar package is bit stretching this thought, but this approach has helped kept packaging simple and avoiding many issues.
Comment 5 Emmanuel Seyman 2014-09-08 05:49:51 EDT
Taking.
Comment 6 Emmanuel Seyman 2014-09-14 15:45:46 EDT
=== KEY ===

 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===

 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format
%{name}.spec.
 [x] Package meets the Packaging Guidelines including the Perl specific items
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on: http://koji.fedoraproject.org/koji/taskinfo?taskID=7576575

 [x] Rpmlint output: 
perl-Gtk2-AppIndicator.src: W: spelling-error Summary(en_US) libappindicator -> applicator
perl-Gtk2-AppIndicator.src: W: spelling-error %description -l en_US libappindicator -> applicator
perl-Gtk2-AppIndicator.src: E: unknown-key GPG#00f97f56
1 packages and 1 specfiles checked; 1 errors, 2 warnings.

 [x] Package is not relocatable.
 [x] Buildroot is correct
None specified, default used

 [x] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging Guidelines.
 [!] License field in the package spec file matches the actual license.
     License type: Artistic clarified

From the README file in the distribution:

"COPYRIGHT AND LICENCE

Artistic

Copyright (C) 2012 by Hans Oesterholt

This library is free software; you can redistribute it and/or modify it under
the same terms as Perl itself, either Perl version 5.12.3 or, at your option,
any later version of Perl 5 you may have available."

From the module documentation:

"This program is free software; you can redistribute it and/or modify it under
the terms of the Artistic License, which comes with Perl."

The license file contains text of the clarified Artistic license.

Remi, I'ld rather you change the license to "Artistic clarified" 

 [x] 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 is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
a90db45394d50d4b2656cfb292710265  Gtk2-AppIndicator-0.15.tar.gz

 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that
are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [x] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===

 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: fedora-rawhide-x86_64
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested on: http://koji.fedoraproject.org/koji/taskinfo?taskID=7576575
 [?] Package functions as described.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.
 [x] %check is present and the tests pass
'make test disabled, requires a display'

Remi, fix the License tag and you're good.
Comment 7 Remi Collet 2014-09-15 01:33:57 EDT
(In reply to Emmanuel Seyman from comment #6)
> Remi, fix the License tag and you're good.

I was confused by the comment in README "This library is free software; you can redistribute it and/or modify it under the same terms as Perl itself, either Perl version 5.12.3 or, at your option, any later version of Perl 5 you may have available."

Fixed in: https://github.com/remicollet/remirepo/commit/671321f9511f429eacefefe59349a053f7d02ffd

Spec: https://raw.githubusercontent.com/remicollet/remirepo/671321f9511f429eacefefe59349a053f7d02ffd/perl-Gtk2-AppIndicator/perl-Gtk2-AppIndicator.spec
Srpm: http://rpms.famillecollet.com/SRPMS/perl-Gtk2-AppIndicator-0.15-3.remi.src.rpm
Comment 8 Emmanuel Seyman 2014-09-15 06:38:01 EDT
APPROVED.
Comment 9 Remi Collet 2014-09-15 07:24:14 EDT
Thanks for the review!

New Package SCM Request
=======================
Package Name: perl-Gtk2-AppIndicator
Short Description: Perl extension for libappindicator
Upstream URL: http://search.cpan.org/dist/Gtk2-AppIndicator/
Owners: remi
Branches: f19 f20 f21
InitialCC:  perl-sig
Comment 10 Gwyn Ciesla 2014-09-15 08:20:09 EDT
Git done (by process-git-requests).
Comment 11 Petr Pisar 2014-09-15 09:45:59 EDT
The declared license (Artistic) is unacceptable for Fedora <https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Bad_Licenses>.

However the wording "under the same terms as Perl itself" means (GPL+ or Artistic) as has been approved by Fedora legal department. (I will find the link later).

Therefore you should change the license declaration from (Artistic) to (GPL+ or Artistic).

Just for your information, the gperl.h reads:

 * This library is free software; you can redistribute it and/or modify it
 * under the terms of the GNU Library General Public License as published by
 * the Free Software Foundation; either version 2.1 of the License, or (at your
 * option) any later version.

The file is included at AppIndicator.xs:8. I hope including LGPL header file into non-LGPL does breach the LGPL.
Comment 12 Petr Pisar 2014-09-15 09:50:48 EDT
(In reply to Petr Pisar from comment #11)
> However the wording "under the same terms as Perl itself" means (GPL+ or
> Artistic) as has been approved by Fedora legal department. (I will find the
> link later).
> 
See <https://bugzilla.redhat.com/show_bug.cgi?id=1120297#c5> and <https://bugzilla.redhat.com/show_bug.cgi?id=1120299#c2>.
Comment 14 Fedora Update System 2014-09-15 10:36:38 EDT
perl-Gtk2-AppIndicator-0.15-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/perl-Gtk2-AppIndicator-0.15-2.fc19
Comment 15 Fedora Update System 2014-09-15 10:36:46 EDT
perl-Gtk2-AppIndicator-0.15-2.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/perl-Gtk2-AppIndicator-0.15-2.fc20
Comment 16 Fedora Update System 2014-09-15 10:36:51 EDT
perl-Gtk2-AppIndicator-0.15-2.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/perl-Gtk2-AppIndicator-0.15-2.fc21
Comment 17 Fedora Update System 2014-09-16 14:43:31 EDT
perl-Gtk2-AppIndicator-0.15-2.fc21 has been pushed to the Fedora 21 testing repository.
Comment 18 Jakub Čajka 2014-09-19 03:08:38 EDT
Hello,

package fails to build on s390 and AArch64 as it is dependant on libappindicator which is exclusive to %{mono_arches}, i.e. package should have ExclusiveArch:  %{mono_arches} due this dependency.

Links to failing builds:
http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=1540554
http://arm.koji.fedoraproject.org/koji/taskinfo?taskID=2675332

Please look in to it,

Jakub
Comment 19 Remi Collet 2014-09-19 04:07:16 EDT
(In reply to Jakub Čajka from comment #18)
> Please look in to it,

Done: http://pkgs.fedoraproject.org/cgit/perl-Gtk2-AppIndicator.git/commit/?id=8c0b4e9765808ca96e28ce482de425fb7cea8df1

Do you need an update in all the branch, or is rawhide enough ?
Comment 20 Petr Pisar 2014-09-19 04:14:50 EDT
You should keep this bug open and make it a blocker for the architecture-specific tracker. See <https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Architecture_Build_Failures>.
Comment 21 Jakub Čajka 2014-09-19 04:42:59 EDT
(In reply to Remi Collet from comment #19)
> (In reply to Jakub Čajka from comment #18)
> > Please look in to it,
> 
> Done:
> http://pkgs.fedoraproject.org/cgit/perl-Gtk2-AppIndicator.git/commit/
> ?id=8c0b4e9765808ca96e28ce482de425fb7cea8df1
> 
> Do you need an update in all the branch, or is rawhide enough ?

In all branches please. Thanks for quick response.
Comment 22 Remi Collet 2014-09-19 05:06:49 EDT
(In reply to Jakub Čajka from comment #21)
> In all branches please. Thanks for quick response.
Done.
Comment 23 Fedora Update System 2014-09-27 06:11:07 EDT
perl-Gtk2-AppIndicator-0.15-4.fc21 has been pushed to the Fedora 21 stable repository.
Comment 24 Fedora Update System 2014-10-03 23:18:54 EDT
perl-Gtk2-AppIndicator-0.15-4.fc20 has been pushed to the Fedora 20 stable repository.
Comment 25 Fedora Update System 2014-10-03 23:23:04 EDT
perl-Gtk2-AppIndicator-0.15-4.fc19 has been pushed to the Fedora 19 stable repository.

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