classification
Title: Misc fixes and cleanups in archiving code in shutil and test_shutil
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: eric.araujo, georg.brandl, martin.panter, nadeem.vawda, python-dev, serhiy.storchaka, tarek
Priority: normal Keywords: patch

Created on 2012-02-20 02:31 by eric.araujo, last changed 2016-12-16 17:23 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
shutil_make_archive_misc.patch serhiy.storchaka, 2015-11-12 17:38 review
shutil_make_archive_misc_2.patch serhiy.storchaka, 2016-06-23 07:24 review
shutil_make_archive_misc_3.patch serhiy.storchaka, 2016-11-12 22:53 review
Messages (10)
msg153760 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-20 02:31
shutil.make_archive suffers from these issues:

- It always supports the 'zip' and 'gztar' formats but if Python is built without zlib then they won’t work.  Even if this is probably rare, the code should be robust.  Likewise, the tests detect the availability of bz2 but not zlib/gzip.

- If zipfile is not found, it tries to call a zip program.  I think this approach (inherited from distutils, which did that before zipfile and tarfile were in the stdlib) should be dropped.  I don’t like shutil using distutils.spawn, I don’t like a Python module calling an external program, and I think nearly everyone has a Python compiled with zlib now.

I want to work on a patch to fix this.  The first point would not change the behavior for most users, it would only modify the list of formats ”guaranteed” by the docs, so I think it’s okay for stable branches; the second point is probably inappropriate for stable branches.
msg154028 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-23 01:18
Another bug: the bztar unpacker will be called for files ending in '.bz2', but it should really check for '.tar.bz2' (just like the gztar unpacker is called for '.tar.gz' or '.tgz').
msg154121 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-24 07:48
I intend to commit this simple fix for all branches:

-    _UNPACK_FORMATS['bztar'] = (['.bz2'], _unpack_tarfile, [],
+    _UNPACK_FORMATS['bztar'] = (['.tar.bz2'], _unpack_tarfile, [],

I could write a unit test, but it would be coupled to the exact text of the exception* (i.e. with assertRaisesRegex) and IMO not very useful.  I tested manually: I compressed a text file with bzip2, passed the filename to unpack_archive, and it tried to unpack it with tarfile.  With my fix, it does not try to unpack it.


* unpack_archive raises ValueError when you give an unsupported value to its format
  parameter, but when it’s in autodetect mode and can’t find an unpacker for the file
  extension, then it raises a ReadError.  Is it okay to change that to ValueError?
msg154134 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2012-02-24 11:22
I don't think there's any harm in testing that the exception message for
a .bz2 file contains the string "unknown archive format". It's unlikely
that we'll want to completely change the error message in future, and if
we do, it will be pretty easy and obvious to fix.


> * unpack_archive raises ValueError when you give an unsupported value to its format
>  parameter, but when it’s in autodetect mode and can’t find an unpacker for the file
>  extension, then it raises a ReadError.  Is it okay to change that to ValueError?

Sounds fine by me.
msg154604 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-29 09:06
test_shutil contains partial tests for the archiving functionality.  I plan to rewrite some tests so that for example you can see a skip message when bz2 is not available, instead of silent non-testing.  Also, the internal functions _make_tarfile and _make_zipfile are directly tested, but IMO it’d be better to only exercise make_archive.
msg254537 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-11-12 17:38
Most of issues are already fixed. Proposed patch fixes remnants:

* You see a skip message when bz2 or lzma are not available, instead of silent non-testing.

* Removed a code that handles zipfile import failure. Importing zipfile now is always successful.

* The gztar and zip formats now are registered and supported only when zlib is available. This is documented now.

* Broken circular dependency between shutil and tarfile.

* Removed outdated sentences about the InfoZIP "zip" utility.
msg254568 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-11-12 22:43
Remember that you probably should not mention “xztar” in the 3.4 or 2.7 docs (even though “lzma” exists in 3.4).

I am not too familiar with using shutil or zipfile, but it seems like extraction of some zip files is always possible, even if zlib (for Deflate) isn’t available (e.g. for zip files that are not compressed, or use bzip2 or LZMA). Maybe you should only make zip file _creation_ conditional.
msg254594 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-11-13 09:49
Yes, I were going to left this for other issue, but now I agree that it is better to left zip in unarhiving registry unconditionally.

In 2.7 the support of bztar is not conditional.
msg283418 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-16 17:07
New changeset 929c2d076a85 by Serhiy Storchaka in branch '3.5':
Issue #14061: Misc fixes and cleanups in archiving code in shutil.
https://hg.python.org/cpython/rev/929c2d076a85

New changeset 268d3763bb07 by Serhiy Storchaka in branch '3.6':
Issue #14061: Misc fixes and cleanups in archiving code in shutil.
https://hg.python.org/cpython/rev/268d3763bb07

New changeset 5fd772a20f25 by Serhiy Storchaka in branch 'default':
Issue #14061: Misc fixes and cleanups in archiving code in shutil.
https://hg.python.org/cpython/rev/5fd772a20f25

New changeset eb02db65e148 by Serhiy Storchaka in branch '2.7':
Issue #14061: Misc fixes and cleanups in archiving code in shutil.
https://hg.python.org/cpython/rev/eb02db65e148
msg283421 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-16 17:23
Thank you Martin for your review.
History
Date User Action Args
2016-12-16 17:23:31serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg283421

stage: patch review -> resolved
2016-12-16 17:07:16python-devsetnosy: + python-dev
messages: + msg283418
2016-11-12 22:53:06serhiy.storchakasetfiles: + shutil_make_archive_misc_3.patch
2016-10-25 18:43:51serhiy.storchakasetassignee: serhiy.storchaka
versions: + Python 3.7
2016-06-23 07:24:38serhiy.storchakasetfiles: + shutil_make_archive_misc_2.patch
versions: - Python 3.4
2015-11-13 09:49:56serhiy.storchakasetmessages: + msg254594
2015-11-12 22:43:19martin.pantersetnosy: + martin.panter
messages: + msg254568
2015-11-12 17:38:19serhiy.storchakasetfiles: + shutil_make_archive_misc.patch
versions: + Python 3.6
messages: + msg254537

keywords: + patch
stage: patch review
2014-08-06 16:01:22serhiy.storchakasetnosy: + serhiy.storchaka

type: behavior
versions: + Python 3.4, Python 3.5, - Python 3.2, Python 3.3
2014-08-06 15:44:46serhiy.storchakaunlinkissue5411 dependencies
2012-02-29 09:06:51eric.araujosetmessages: + msg154604
title: Clean up archiving code in shutil -> Misc fixes and cleanups in archiving code in shutil and test_shutil
2012-02-24 11:22:27nadeem.vawdasetmessages: + msg154134
2012-02-24 07:48:12eric.araujosetmessages: + msg154121
2012-02-23 01:18:34eric.araujosetmessages: + msg154028
2012-02-20 02:35:56eric.araujolinkissue5411 dependencies
2012-02-20 02:31:05eric.araujocreate