Repository navigation
shutil should not use distutils #64943
Description
| BPO | 20744 |
|---|---|
| Nosy | @akuchling, @doko42, @ronaldoussoren, @pitrou, @merwok |
| Files |
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']
Activity
doko42 commented on Feb 23, 2014
shutil imports distutils in _call_external_zip just for the calling of an external command. This should be done using subprocess these days.
pitrou commented on Feb 23, 2014
Does it pose an actual problem?
ronaldoussoren commented on Feb 25, 2014
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 commented on Mar 3, 2014
Agreed; this looks like a relic. zlib is optional, but zipfile is always in the standard library.
derekchiang93 commented on Mar 4, 2014
New contributor here. I'm submitting a patch; please let me know if I'm doing something wrong :)
merwok commented on Mar 7, 2014
Patch looks good to me.
akuchling commented on Mar 20, 2014
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 commented on Mar 20, 2014
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 commented on Mar 20, 2014
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 commented on Mar 20, 2014
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 commented on Mar 20, 2014
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 commented on Mar 20, 2014
Cool, thanks for doing this!