classification
Title: shutil should not use distutils
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, akuchling, derekchiang93, doko, eric.araujo, pitrou, python-dev, ronaldoussoren, tshepang
Priority: normal Keywords: patch

Created on 2014-02-23 17:15 by doko, last changed 2014-03-20 21:54 by derekchiang93. This issue is now closed.

Files
File name Uploaded Description Edit
patch-20744.diff derekchiang93, 2014-03-04 08:51 review
Messages (12)
msg212007 - (view) Author: Matthias Klose (doko) * (Python committer) Date: 2014-02-23 17:15
shutil imports distutils in _call_external_zip just for the calling of an external command.  This should be done using subprocess these days.
msg212010 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-23 17:20
Does it pose an actual problem?
msg212209 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2014-02-25 20:11
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.
msg212660 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-03-03 19:39
Agreed; this looks like a relic.  zlib is optional, but zipfile is always in the standard library.
msg212696 - (view) Author: Derek Chiang (derekchiang93) * Date: 2014-03-04 08:51
New contributor here.  I'm submitting a patch; please let me know if I'm doing something wrong :)
msg212870 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-03-07 10:31
Patch looks good to me.
msg214167 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2014-03-20 02:06
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 )
msg214168 - (view) Author: Derek Chiang (derekchiang93) * Date: 2014-03-20 02:11
I didn't because the patch seemed trivial.  I'm sure there are automated tests that will be run before the patch gets merged?
msg214169 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2014-03-20 02:16
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).
msg214266 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2014-03-20 19:42
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!
msg214270 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-03-20 20:11
New changeset 681e20f8b717 by Andrew Kuchling in branch 'default':
#20744: don't try running an external 'zip' in shutil.make_archive()
http://hg.python.org/cpython/rev/681e20f8b717
msg214288 - (view) Author: Derek Chiang (derekchiang93) * Date: 2014-03-20 21:54
Cool, thanks for doing this!
History
Date User Action Args
2014-03-20 21:54:49derekchiang93setmessages: + msg214288
2014-03-20 20:14:51akuchlingsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2014-03-20 20:11:32python-devsetnosy: + python-dev
messages: + msg214270
2014-03-20 19:42:14akuchlingsetmessages: + msg214266
2014-03-20 18:46:35Arfreversetnosy: + Arfrever
2014-03-20 02:16:53eric.araujosetmessages: + msg214169
2014-03-20 02:11:57derekchiang93setmessages: + msg214168
2014-03-20 02:06:29akuchlingsetnosy: + akuchling
messages: + msg214167
2014-03-07 10:31:07eric.araujosetstage: needs patch -> patch review
messages: + msg212870
versions: + Python 3.5, - Python 3.3, Python 3.4
2014-03-04 08:51:40derekchiang93setfiles: + patch-20744.diff

nosy: + derekchiang93
messages: + msg212696

keywords: + patch
2014-03-03 19:39:33eric.araujosetnosy: + eric.araujo
messages: + msg212660
2014-03-03 17:54:17tshepangsetnosy: + tshepang
2014-02-25 20:11:47ronaldoussorensetnosy: + ronaldoussoren
messages: + msg212209
2014-02-23 17:20:55pitrousetnosy: + pitrou
messages: + msg212010
2014-02-23 17:15:43dokocreate