Bug 1542646

Summary: Review Request: patool - Portable command line archive file manager
Product: [Fedora] Fedora Reporter: Robert-André Mauchin 🐧 <zebob.m>
Component: Package ReviewAssignee: Zbigniew Jędrzejewski-Szmek <zbyszek>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: package-review, zbyszek
Target Milestone: ---Flags: zbyszek: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2018-02-20 16:38:31 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
pytest-2 output when reqs are installed none

Description Robert-André Mauchin 🐧 2018-02-06 17:49:34 UTC
Spec URL: https://raw.githubusercontent.com/eclipseo/packaging/e632a7b/patool.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/eclipseo/patool/fedora-rawhide-x86_64/00708379-patool/patool-1.12-1.fc28.src.rpm

Description:
Patool is an archive file manager.

Various archive formats can be created, extracted, tested, listed, searched, 
repacked and compared with patool. The advantage of patool is its simplicity 
in handling archive files without having to remember a myriad of programs 
and options. 

The archive format is determined by the file(1) program and as a fallback 
by the archive file extension. 

patool supports 7z (.7z, .cb7), ACE (.ace, .cba), ADF (.adf), ALZIP (.alz), 
APE (.ape), AR (.a), ARC (.arc), ARJ (.arj), BZIP2 (.bz2), CAB (.cab), 
COMPRESS (.Z), CPIO (.cpio), DEB (.deb), DMS (.dms), FLAC (.flac), GZIP (.gz), 
ISO (.iso), LRZIP (.lrz), LZH (.lha, .lzh), LZIP (.lz), LZMA (.lzma), 
LZOP (.lzo), RPM (.rpm), RAR (.rar, .cbr), RZIP (.rz), SHN (.shn), 
TAR (.tar, .cbt), XZ (.xz), ZIP (.zip, .jar, .cbz) and ZOO (.zoo) 
archive formats. It relies on helper applications to handle those archive 
formats (for example bzip2 for BZIP2 archives).

The archive formats TAR, ZIP, BZIP2 and GZIP are supported natively and do 
not require helper applications to be installed.

Fedora Account System Username: eclipseo

Comment 1 Zbigniew Jędrzejewski-Szmek 2018-02-06 21:37:35 UTC
> %{_datadir}/bash-completion/completions/patool.bash-completion
This should be called just 'patool', without the extension.

> %{python2_sitelib}/_patool_configdata.*
This is really an upstream issue, but a package should never ever install private modules into the top-level namespace. Maybe you can talk with upstream to move it underneath patoollib/ ?

Shouldn't this package have Recommends on various tools for all the non-natively-supported formats?

Comment 2 Robert-André Mauchin 🐧 2018-02-06 23:43:55 UTC
> %{python2_sitelib}/_patool_configdata.*
This is really an upstream issue, but a package should never ever install private modules into the top-level namespace. Maybe you can talk with upstream to move it underneath patoollib/ ?

Wouldn't work currently:

File "/usr/lib/python3.6/site-packages/patoolib/configuration.py", line 5, in <module>
    import _patool_configdata as configdata

I doubt I'll manage to get upstream to change this as the GIT repo and issue tracker haven't bulge for 2 years now.

There's also a handful of top-level packages that already exist:

$ ll /usr/lib/python3.6/site-packages/*.py
-rw-r--r--. 1 root root  24K août  31  2014 /usr/lib/python3.6/site-packages/augeas.py
-rw-r--r--. 1 root root  16K févr. 16  2016 /usr/lib/python3.6/site-packages/cycler.py
-rw-r--r--. 1 root root  16K janv. 15  2017 /usr/lib/python3.6/site-packages/decorator.py
-rw-r--r--. 1 root root  39K déc.  24 17:29 /usr/lib/python3.6/site-packages/distro.py
-rw-r--r--. 1 root root  126 nov.  28 16:47 /usr/lib/python3.6/site-packages/easy_install.py
-rw-r--r--. 1 root root  19K juil. 26  2017 /usr/lib/python3.6/site-packages/git_archive_all.py
-rw-r--r--. 1 root root  59K avril  1  2013 /usr/lib/python3.6/site-packages/IPy.py
-rw-r--r--. 1 root root  75K nov.   9 10:24 /usr/lib/python3.6/site-packages/langtable.py
-rw-r--r--. 1 root root 8,2K avril  4  2017 /usr/lib/python3.6/site-packages/magic.py
-rw-r--r--. 1 root root  14K juil. 27  2015 /usr/lib/python3.6/site-packages/ntplib.py
-rw-r--r--. 1 root root 1,1K janv.  6  2017 /usr/lib/python3.6/site-packages/OleFileIO_PL.py
-rw-r--r--. 1 root root 6,2K déc.  15  2015 /usr/lib/python3.6/site-packages/ordered_set.py
-rw-r--r--. 1 root root  87K juin   4  2015 /usr/lib/python3.6/site-packages/pyinotify.py
-rw-r--r--. 1 root root 225K juil. 28  2017 /usr/lib/python3.6/site-packages/pyparsing.py
-rw-r--r--. 1 root root 4,4K août  24  2015 /usr/lib/python3.6/site-packages/requests_file.py
-rw-r--r--. 1 root root 118K juil. 28  2017 /usr/lib/python3.6/site-packages/sh.py
-rw-r--r--. 1 root root  31K sept. 19 15:47 /usr/lib/python3.6/site-packages/six.py
-rw-r--r--. 1 root root 2,9K mars  28  2015 /usr/lib/python3.6/site-packages/sockshandler.py
-rw-r--r--. 1 root root  32K mars  23  2017 /usr/lib/python3.6/site-packages/socks.py
-rw-r--r--. 1 root root 5,0K janv. 13  2011 /usr/lib/python3.6/site-packages/termcolor.py
-rw-r--r--. 1 root root 9,7K déc.   8 16:02 /usr/lib/python3.6/site-packages/tldr.py


I could patch it I guess.


>> %{_datadir}/bash-completion/completions/patool.bash-completion
>This should be called just 'patool', without the extension.

Done.

> Shouldn't this package have Recommends on various tools for all the non-natively-supported formats?

Yes I forgot about this.


Spec URL: https://raw.githubusercontent.com/eclipseo/packaging/7bf94ea/patool.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/eclipseo/patool/fedora-rawhide-x86_64/00711233-patool/patool-1.12-1.fc28.src.rpm

SPEC diff: https://github.com/eclipseo/packaging/commit/7bf94eab8915ea4021e01df12059e4aa4e1da804#diff-978fea9b8642e77c470ed419b5f34bfd

Comment 3 Zbigniew Jędrzejewski-Szmek 2018-02-07 08:48:38 UTC
> File "/usr/lib/python3.6/site-packages/patoolib/configuration.py", line 5, in <module>
>    import _patool_configdata as configdata

Yeah, that'd require changing the code…

Other packages you quote don't installl *private* modules. Top-level single-file modules are OK, it's just the ones that start with "_" that bother me. Anyway, that's an RFE, not something that matters for package review.

> %{__install}
There's no benefit over using plain "install".

pytest-{2,3} should be called -v so that the tests are listed.

I'm not sure about the Recommends: some of those binaries are not available in Fedora. Not sure what dnf does in that case, but it'll at least warn. You might want to comment those out until they are available.

If all those binaries are available in the build environment, tests fail, see attached file. You should add whatever as much as possible to BR and then fix the tests or filter them out.

Comment 4 Zbigniew Jędrzejewski-Szmek 2018-02-07 08:49:48 UTC
Created attachment 1392508 [details]
pytest-2 output when reqs are installed

Comment 5 Robert-André Mauchin 🐧 2018-02-07 15:47:07 UTC
Spec URL: https://github.com/eclipseo/packaging/blob/7f99098/patool.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/eclipseo/patool/fedora-rawhide-x86_64/00711629-patool/patool-1.12-1.fc28.src.rpm

SPEC diff: https://github.com/eclipseo/packaging/commit/7f99098c0c1efbfbc8ab5cba59927288038caa1b#diff-978fea9b8642e77c470ed419b5f34bfd


I patched the test of Zopfli: it was testing decompression which the zopfli binary doesn't do. It now tests the compression.

The other errors were related to Star: the program assumed the arguments were the same as "tar" but in fact they are slightly different for verbosity and compress-type. I rewrote this portion and now tests are successful.

Thanks again for the review.

Comment 6 Zbigniew Jędrzejewski-Szmek 2018-02-07 16:03:49 UTC
+ package name is OK
+ license is acceptable for Fedora (GPLv3+)
+ license is specified correctly
+ builds and installs OK
+ Requires/BuildRequires/Provides/Recommends look good
+ fedora-review finds no important issues
+ tests are present and pass

Package is APPROVED.

Comment 7 Gwyn Ciesla 2018-02-07 19:27:43 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/patool. You may commit to the branch "f27" in about 10 minutes.

Comment 8 Fedora Update System 2018-02-09 22:36:20 UTC
patool-1.12-1.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2018-3a91b279e2

Comment 9 Fedora Update System 2018-02-09 22:51:46 UTC
patool-1.12-1.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2018-aabb305a90

Comment 10 Fedora Update System 2018-02-12 19:10:09 UTC
patool-1.12-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-aabb305a90

Comment 11 Fedora Update System 2018-02-13 07:58:55 UTC
patool-1.12-1.fc27 has been pushed to the Fedora 27 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-3a91b279e2

Comment 12 Fedora Update System 2018-02-20 16:38:31 UTC
patool-1.12-1.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

Comment 13 Fedora Update System 2018-02-27 17:17:59 UTC
patool-1.12-1.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.