shutil should not use distutils

[doko42]

BPO	20744
Nosy	@akuchling, @doko42, @ronaldoussoren, @pitrou, @merwok
Files	patch-20744.diff

^{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 = None
closed_at = <Date 2014-03-20.20:14:51.234>
created_at = <Date 2014-02-23.17:15:43.963>
labels = ['type-bug', 'library']
title = 'shutil should not use distutils'
updated_at = <Date 2014-03-20.21:54:49.203>
user = 'https://github.com/doko42'

bugs.python.org fields:

activity = <Date 2014-03-20.21:54:49.203>
actor = 'derekchiang93'
assignee = 'none'
closed = True
closed_date = <Date 2014-03-20.20:14:51.234>
closer = 'akuchling'
components = ['Library (Lib)']
creation = <Date 2014-02-23.17:15:43.963>
creator = 'doko'
dependencies = []
files = ['34283']
hgrepos = []
issue_num = 20744
keywords = ['patch']
message_count = 12.0
messages = ['212007', '212010', '212209', '212660', '212696', '212870', '214167', '214168', '214169', '214266', '214270', '214288']
nosy_count = 9.0
nosy_names = ['akuchling', 'doko', 'ronaldoussoren', 'pitrou', 'eric.araujo', 'Arfrever', 'tshepang', 'python-dev', 'derekchiang93']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue20744'
versions = ['Python 3.5']

[doko42]

shutil imports distutils in _call_external_zip just for the calling of an external command. This should be done using subprocess these days.

[pitrou]

Does it pose an actual problem?

[ronaldoussoren]

Why is _call_external_zip needed at all? The code says it is used when the zipfile module is not available, but that module is part of the stdlib and should always be available.

[merwok]

Agreed; this looks like a relic. zlib is optional, but zipfile is always in the standard library.

[derekchiang93]

New contributor here. I'm submitting a patch; please let me know if I'm doing something wrong :)

[merwok]

Patch looks good to me.

[akuchling]

Derek: thanks for your patch! However, did you run the test suite for the shutil module to verify that your change is correct?

(The developer guide discusses running tests at http://docs.python.org/devguide/runtests.html )

[derekchiang93]

I didn't because the patch seemed trivial. I'm sure there are automated tests that will be run before the patch gets merged?

[merwok]

Not currently. Tests run automatically after a patch is merged, which is why patch authors should run tests (especially if they’re changing something missing tests, or adding a new feature and tests for it).

[akuchling]

Yes, tests are only run after a change is committed and pushed into
Mercurial; this is done by BuildBot https://www.python.org/dev/buildbot/ .

So it's a good idea to run tests before submitting a patch or committing a change. No matter how trivial a change seems, it should always be tested first. Every programmer has a few stories of "this can't possibly fail" changes that fail, sometimes spectacularly.

(One of mine: I rewrote some C string-handling code for a product
that supported 4 or 5 different Unixes and processor architectures,
tried it on one of them, and concluded it was fine. It segfaulted on
exactly one architecture. Unfortunately this was discovered by a VP
who was demoing to a customer at the time. I got a talking-to about
that one.)

Running the tests finds a simple problem: there's no longer an 'import
zipfile' statement. I'll add the import inside the _make_zipfile()
function. This is against PEP-8, strictly speaking, but it means
importing shutil doesn't always import zipfile; it'll only import the
module if it's actually needed. (I'll probably do the same for the
import of tarfile.)

Derek, thanks for your patch!

[python-dev]

New changeset 681e20f8b717 by Andrew Kuchling in branch 'default':
bpo-20744: don't try running an external 'zip' in shutil.make_archive()
http://hg.python.org/cpython/rev/681e20f8b717

[derekchiang93]

Cool, thanks for doing this!