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

Does it pose an actual problem?

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.

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

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

Patch looks good to me.

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 )

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

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

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!

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

Cool, thanks for doing this!

nosy: + derekchiang93
messages: + msg212696

keywords: + patch