Bug 1392606 - Review Request: perl-Encode-IMAPUTF7 - Process the special UTF-7 variant required by IMAP
Summary: Review Request: perl-Encode-IMAPUTF7 - Process the special UTF-7 variant requ...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jitka Plesnikova
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1394139
TreeView+ depends on / blocked
 
Reported: 2016-11-07 21:38 UTC by Jason Tibbitts
Modified: 2016-11-14 15:33 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-11-11 01:12:25 UTC
Type: ---
Embargoed:
jplesnik: fedora-review+


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 772620 0 medium CLOSED Review Request: perl-Encode-IMAPUTF7 - Perl extension to deal with UTF-7 modification for IMAP 2021-02-22 00:41:40 UTC

Internal Links: 772620

Description Jason Tibbitts 2016-11-07 21:38:03 UTC
Spec URL: https://www.math.uh.edu/~tibbs/review/perl-Encode-IMAPUTF7/perl-Encode-IMAPUTF7.spec
SRPM URL: https://www.math.uh.edu/~tibbs/review/perl-Encode-IMAPUTF7/perl-Encode-IMAPUTF7-1.05-1.fc24.src.rpm
Description:
This module is able to encode and decode IMAP mailbox names using the UTF-7
modification specified in RFC2060 section 5.1.3.

Fedora Account System Username: tibbs

Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=16340823

Fedora-review output (note that any strange-permissions errors are due to rpmlint not being able to cope with my umask):

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[ ]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[ ]: 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 %license.
[ ]: License field in the package spec file matches the actual license.
     Note: There is no build directory. Running licensecheck on vanilla
     upstream sources. No licenses found. Please check the source files for
     licenses manually.
[ ]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[ ]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by:
     /usr/share/perl5/vendor_perl/Encode(perl-Encode-Locale)
[ ]: Package contains no bundled libraries without FPC exception.
[ ]: Changelog in prescribed format.
[ ]: Sources contain only permissible code or content.
[ ]: Package contains desktop file if it is a GUI application.
[ ]: Development files must be in a -devel package
[ ]: Package uses nothing in %doc for runtime.
[ ]: Package consistently uses macros (instead of hard-coded directory
     names).
[ ]: Package is named according to the Package Naming Guidelines.
[ ]: Package does not generate any conflict.
[ ]: Package obeys FHS, except libexecdir and /usr/target.
[ ]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[ ]: Requires correct, justified where necessary.
[ ]: Spec file is legible and written in American English.
[ ]: Package contains systemd file(s) if in need.

ἐπιθυμία:..perl-Encode-IMAPUTF7/review-perl-Encode-IMAPUTF7> cat review.txt

This is a review *template*. Besides handling the [ ]-marked tests you are
also supposed to fix the template before pasting into bugzilla:
- Add issues you find to the list of issues on top. If there isn't such
  a list, create one.
- Add your own remarks to the template checks.
- Add new lines marked [!] or [?] when you discover new things not
  listed by fedora-review.
- Change or remove any text in the template which is plain wrong. In this
  case you could also file a bug against fedora-review
- Remove the "[ ] Manual check required", you will not have any such lines
  in what you paste.
- Remove attachments which you deem not really useful (the rpmlint
  ones are mandatory, though)
- Remove this text



Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[ ]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[ ]: 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 %license.
[ ]: License field in the package spec file matches the actual license.
     Note: There is no build directory. Running licensecheck on vanilla
     upstream sources. No licenses found. Please check the source files for
     licenses manually.
[ ]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[ ]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by:
     /usr/share/perl5/vendor_perl/Encode(perl-Encode-Locale)
[ ]: Package contains no bundled libraries without FPC exception.
[ ]: Changelog in prescribed format.
[ ]: Sources contain only permissible code or content.
[ ]: Package contains desktop file if it is a GUI application.
[ ]: Development files must be in a -devel package
[ ]: Package uses nothing in %doc for runtime.
[ ]: Package consistently uses macros (instead of hard-coded directory
     names).
[ ]: Package is named according to the Package Naming Guidelines.
[ ]: Package does not generate any conflict.
[ ]: Package obeys FHS, except libexecdir and /usr/target.
[ ]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[ ]: Requires correct, justified where necessary.
[ ]: Spec file is legible and written in American English.
[ ]: Package contains systemd file(s) if in need.
[ ]: Package is not known to require an ExcludeArch tag.
[ ]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 2 files.
[ ]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Perl:
[ ]: Package contains the mandatory BuildRequires and Requires:.
[ ]: CPAN urls should be non-versioned.

===== SHOULD items =====

Generic:
[ ]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[ ]: Final provides and requires are sane (see attachments).
[ ]: Package functions as described.
[ ]: Latest version is packaged.
[ ]: 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.
[ ]: Package should compile and build into binary rpms on all supported
     architectures.
[ ]: %check is present and all tests pass.
[ ]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: perl-Encode-IMAPUTF7-1.05-1.fc24.noarch.rpm
          perl-Encode-IMAPUTF7-1.05-1.fc24.src.rpm
perl-Encode-IMAPUTF7.src: W: strange-permission Encode-IMAPUTF7-1.05.tar.gz 660
perl-Encode-IMAPUTF7.src: W: strange-permission perl-Encode-IMAPUTF7.spec 660
2 packages and 0 specfiles checked; 0 errors, 2 warnings.




Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
1 packages and 0 specfiles checked; 0 errors, 0 warnings.



Requires
--------
perl-Encode-IMAPUTF7 (rpmlib, GLIBC filtered):
    perl(:MODULE_COMPAT_5.22.2)
    perl(Encode)
    perl(Encode::Encoding)
    perl(MIME::Base64)
    perl(base)
    perl(strict)



Provides
--------
perl-Encode-IMAPUTF7:
    perl(Encode::IMAPUTF7)
    perl-Encode-IMAPUTF7



Source checksums
----------------
http://www.cpan.org/authors/id/P/PM/PMAKHOLM/Encode-IMAPUTF7-1.05.tar.gz :
  CHECKSUM(SHA256) this package     : 470305ddc37483cfe8d3c16d13770a28011f600bf557acb8c3e07739997c37e1
  CHECKSUM(SHA256) upstream package : 470305ddc37483cfe8d3c16d13770a28011f600bf557acb8c3e07739997c37e1


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -v -n perl-Encode-IMAPUTF7
Buildroot used: fedora-24-x86_64
Active plugins: Generic, Shell-api, Perl
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 1 Jitka Plesnikova 2016-11-08 15:37:38 UTC
Source file is ok
Summary is ok
License is ok
Description is ok
URL and Source0 are ok
All tests passed

$ rpm -qp --requires perl-Encode-IMAPUTF7-1.05-1.fc26.noarch.rpm | sort | uniq -c
      1 perl(:MODULE_COMPAT_5.24.0)
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsXz) <= 5.2-1
FIX: Most of binary requires are missing, because perl-generators was not installed.

$ rpm -qp --provides perl-Encode-IMAPUTF7-1.05-1.fc26.noarch.rpm | sort | uniq -c
      1 perl-Encode-IMAPUTF7 = 1.05-1.fc26
FIX: Provides 'erl(Encode::IMAPUTF7) = 1.05 is not listed due to missing perl-generators

$ rpmlint ./perl-Encode-IMAPUTF7*
perl-Encode-IMAPUTF7.src: W: strange-permission Encode-IMAPUTF7-1.05.tar.gz 660
perl-Encode-IMAPUTF7.src: W: strange-permission perl-Encode-IMAPUTF7.spec 660
2 packages and 1 specfiles checked; 0 errors, 2 warnings.

Rpmlint is ok

BuildRequires:
FIX: Please add following build-requires:
    coreutils - because 'tr', 'mv', 'touch',... are used in spec file
    findutils - 'find' is used in spec file
    perl - it is used in spec file
    perl-generators - is necessary for generating of package provides and
                      requires. The package is not part of default
		      buildroot
    Encode-IMAPUTF7-1.05/Makefile.PL
    perl(base) - lib/Encode/IMAPUTF7.pm:7
    perl(Encode::Encoding) - lib/Encode/IMAPUTF7.pm:7
    perl(File::Basename) - t/0-test.t:12
    perl(File::Spec) - t/0-test.t:13
    perl(MIME::Base64) - lib/Encode/IMAPUTF7.pm:10
    perl(strict) - lib/Encode/IMAPUTF7.pm:5
    perl(Test::More) - t/0-test.t:12
    perl(warnings) - lib/Encode/IMAPUTF7.pm:7

TODO: Please replace PERL_INSTALL_ROOT with more common DESTDIR.

If you want to add the package to EPEL, please ignore these two TODO 
TODO: The easier way to remove .packlist is used NO_PACKLIST option,
      which is part of perl(ExtUtils::MakeMaker) >= 6.76. It can be 
      used in all Fedoras. The command is 
      %{__perl} Makefile.PL INSTALLDIRS=vendor NO_PACKLIST=1

TODO: Remove the deleting empty directories in %install section. This is
      default behavior for Fedoras.


Please correct all 'FIX' issues and consider fixing 'TODO' items and
provide new spec file.

The package is not approved.

Comment 2 Jason Tibbitts 2016-11-10 01:19:27 UTC
Dang it, I swear one version of the package I made had perl-generators in there.  Unfortunately I started with cpanspec, and it really needs to be fixed.  I fixed a lot of its output but it would have been simpler to start from scratch, I think.  In fact, I realize it's sufficiently useless that I went ahead and remove mention of it from the Perl guidelines entirely.

Anyway, cpanspec seems to be poor enough now that I just removed reference to it from the Perl guidelines.

I didn't even know about the NO_PACKLIST, and the deletion of empty directories by default.  That's great.  I sure wish cpanspec knew about it, too.

Is %_fixperms still required?  And is it still required to call "make pure_install"?  If "make install" worked we could just recommend %make_install.

Here are new files:
Updated spec: https://www.math.uh.edu/~tibbs/review/perl-Encode-IMAPUTF7/perl-Encode-IMAPUTF7.spec
Updated SRPM: https://www.math.uh.edu/~tibbs/review/perl-Encode-IMAPUTF7/perl-Encode-IMAPUTF7-1.05-2.fc25.src.rpm
(f25 this time so that perl-generators isn't there by default)

But, a note, from the person who wrote the packaging guidelines relating to build dependencies:

I did not add the dependencies on coreutils and findutils.  If you're asking others to add them, I would kindly ask that you please stop doing so as it only causes confusion among the packagers.  The guidelines guarantee that RPM can run basic shell scripts and actually build packages.  You get mv and find and tar and sed and patch.  (I did add a dep on make, which elicits a complaint from fedora-review.  Sometimes you just can't win.)

The intent was never to give anyone the impression that those were required.  I went ahead and altered the Perl guidelines to use wording closer to that of the main guidelines to try and minimize any confusion.  But if you have suggestions for any other clarifications I might add (short of just including a complete list, which is what we want to avoid) then please let me know.

Comment 3 Jitka Plesnikova 2016-11-10 08:59:12 UTC
> FIX: Most of binary requires are missing, because perl-generators was not
> installed.
$ rpm -qp --requires perl-Encode-IMAPUTF7-1.05-2.fc26.noarch.rpm | sort | uniq -c
      1 perl(:MODULE_COMPAT_5.24.0)
      1 perl(Encode)
      1 perl(Encode::Encoding)
      1 perl(MIME::Base64)
      1 perl(base)
      1 perl(strict)
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsXz) <= 5.2-1
Binary requires are ok.
 
> FIX: Provides 'erl(Encode::IMAPUTF7) = 1.05 is not listed due to missing
> perl-generators
$ rpm -qp --provides perl-Encode-IMAPUTF7-1.05-2.fc26.noarch.rpm | sort | uniq -c
      1 perl(Encode::IMAPUTF7) = 1.05
      1 perl-Encode-IMAPUTF7 = 1.05-2.fc26
Binary provides are ok.
 
> BuildRequires:
> FIX: Please add following build-requires:
>     coreutils - because 'tr', 'mv', 'touch',... are used in spec file
It is not necessary to listed. Ok

>     findutils - 'find' is used in spec file
'find' was removed from spec. Ok

>     perl - it is used in spec file
>     perl-generators - is necessary for generating of package provides and
>                       requires. The package is not part of default
> 		      buildroot
>     Encode-IMAPUTF7-1.05/Makefile.PL
>     perl(base) - lib/Encode/IMAPUTF7.pm:7
>     perl(Encode::Encoding) - lib/Encode/IMAPUTF7.pm:7
>     perl(File::Basename) - t/0-test.t:12
>     perl(File::Spec) - t/0-test.t:13
>     perl(MIME::Base64) - lib/Encode/IMAPUTF7.pm:10
>     perl(strict) - lib/Encode/IMAPUTF7.pm:5
>     perl(Test::More) - t/0-test.t:12
>     perl(warnings) - lib/Encode/IMAPUTF7.pm:7
-BuildRequires:  perl(Encode)
-BuildRequires:  perl(ExtUtils::MakeMaker)
-BuildRequires:  perl(Test::NoWarnings)
+
+BuildRequires:  make perl perl-generators
+BuildRequires:  perl(base) perl(Encode) perl(Encode::Encoding)
+BuildRequires:  perl(ExtUtils::MakeMaker) perl(File::Basename) perl(File::Spec)
+BuildRequires:  perl(MIME::Base64) perl(strict) perl(Test::More)
+BuildRequires:  perl(Test::NoWarnings) perl(warnings)
Ok.
 
> TODO: Please replace PERL_INSTALL_ROOT with more common DESTDIR.
-make pure_install PERL_INSTALL_ROOT=%buildroot
+make pure_install DESTDIR=%buildroot
Ok.

> TODO: The easier way to remove .packlist is used NO_PACKLIST option,
>       which is part of perl(ExtUtils::MakeMaker) >= 6.76. It can be 
>       used in all Fedoras. The command is 
>       %{__perl} Makefile.PL INSTALLDIRS=vendor NO_PACKLIST=1
-%{__perl} Makefile.PL INSTALLDIRS=vendor
+%{__perl} Makefile.PL INSTALLDIRS=vendor NO_PACKLIST=1
Ok. 
Please add version restriction for perl(ExtUtils::MakeMaker) to require version 6.76 or higher.

> TODO: Remove the deleting empty directories in %install section. This is
>       default behavior for Fedoras.
-%global remove_packlist find %buildroot -type f -name .packlist -exec rm -f {} \\; ; find %buildroot -depth -type d -exec rmdir {} 2>/dev/null \\;
...
-%remove_packlist
Ok.

Difference between 'make install' and 'make pure_install is described here.
https://fedoraproject.org/wiki/Perl/Tips#ExtUtils::MakeMaker

I know that cpanspec is outdated. There are lots of patches, but the upstream does not accept them yet.

The package looks good.
Approved.

Comment 4 Petr Pisar 2016-11-10 09:04:17 UTC
(In reply to Jason Tibbitts from comment #2)
> The guidelines guarantee that
> RPM can run basic shell scripts and actually build packages.
Yes.

> You get mv and
> find and tar and sed and patch.

Oh, really? Where can I get to know what involves "basic shell scripts" commands? The undefined set is what confuses everybody.

Comment 5 Petr Pisar 2016-11-10 09:11:26 UTC
(In reply to Jason Tibbitts from comment #2)
> I went ahead and altered the Perl guidelines to use wording closer to that
> of the main guidelines to try and minimize any confusion.

I recommend not to clutter Perl or any other language-specific guidelines with these main Fedora guidelines statements.

Comment 6 Jason Tibbitts 2016-11-10 18:00:47 UTC
(In reply to Petr Pisar from comment #5) 
> I recommend not to clutter Perl or any other language-specific guidelines
> with these main Fedora guidelines statements.

Me, too, but... the guideline already had a misstatement of the current guideline when it should have had just a link.  Perl folks wanted that statement in there.  I just made sure it wasn't incorrect.

Comment 7 Jason Tibbitts 2016-11-10 21:46:06 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/perl-Encode-IMAPUTF7

Comment 8 Jason Tibbitts 2016-11-11 01:12:25 UTC
Imported and built in rawhide, and added to Anitya.  Thanks for the review and pointers to the Perl tips; it's been a rather long time since I did any Perl work.  I'm trying to package enough of the stack to run Cassandane, the Cyrus IMAPd test suite: https://github.com/cyrusimap/cassandane
Net up will be perl-Mail-IMAPTalk.

This is all part of a larger effort to trying to get our Cyrus IMAPd package updated.

Comment 9 Petr Pisar 2016-11-11 08:30:36 UTC
I will help you. Please check a package is not submitted for a review before packing it so we do not stomp on our feet.

Comment 10 Petr Pisar 2016-11-11 14:25:26 UTC
I submitted all the dependencies for a review.

Comment 11 Jason Tibbitts 2016-11-11 16:51:11 UTC
Hmm, I didn't see any open reviews for any of the deps and the IMAPTalk one was just submitted so I don't think any work has been duplicated besides the IMAPTalk package which I was about to submit but now don't have to.

Still need to package Mail::JMAPTalk, Net::DAVTalk, Net::CalDAVTalk, Net::CardDAVTalk (which aren't on CPAN and haven't yet had tagged releases)and double check the rest of the dependencies listed at https://github.com/cyrusimap/cassandane/blob/master/doc/README.deps

Comment 12 Petr Pisar 2016-11-14 08:19:50 UTC
They are on the CPAN and the reviews were submitted

Mail::JMAPTalk    bug #1394151
Net::DAVTalk      bug #1394242
Net::CalDAVTalk   bug #1394252
Net::CardDAVTalk  bug #1394262

You have to use Bugzilla to search them. Fedora's wiki hides them if they have opened prerequisites.

Comment 13 Jason Tibbitts 2016-11-14 15:33:44 UTC
Oh, cool, thanks.  The cassandane docs mention that they aren't on cpan so I didn't bother to search CPAN.


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