Misc fixes and cleanups in archiving code in shutil and test_shutil

[merwok]

BPO	14061
Nosy	@birkenfeld, @tarekziade, @merwok, @vadmium, @serhiy-storchaka
Files	shutil_make_archive_misc.patch shutil_make_archive_misc_2.patch shutil_make_archive_misc_3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-12-16.17:23:31.902>
created_at = <Date 2012-02-20.02:31:05.378>
labels = ['3.7', 'type-bug', 'library']
title = 'Misc fixes and cleanups in archiving code in shutil and test_shutil'
updated_at = <Date 2016-12-16.17:23:31.900>
user = 'https://github.com/merwok'

bugs.python.org fields:

activity = <Date 2016-12-16.17:23:31.900>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-12-16.17:23:31.902>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2012-02-20.02:31:05.378>
creator = 'eric.araujo'
dependencies = []
files = ['41023', '43517', '45466']
hgrepos = []
issue_num = 14061
keywords = ['patch']
message_count = 10.0
messages = ['153760', '154028', '154121', '154134', '154604', '254537', '254568', '254594', '283418', '283421']
nosy_count = 7.0
nosy_names = ['georg.brandl', 'nadeem.vawda', 'tarek', 'eric.araujo', 'python-dev', 'martin.panter', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14061'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[merwok]

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.

[merwok]

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').

[merwok]

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?

[nadeemvawda]

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.

[merwok]

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.

[serhiy-storchaka]

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.

[vadmium]

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.

[serhiy-storchaka]

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.

[python-dev]

New changeset 929c2d076a85 by Serhiy Storchaka in branch '3.5':
Issue bpo-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 bpo-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 bpo-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 bpo-14061: Misc fixes and cleanups in archiving code in shutil.
https://hg.python.org/cpython/rev/eb02db65e148

[serhiy-storchaka]

Thank you Martin for your review.