archiver should use zipfile before zip

[ooldham]

BPO	639118
Nosy	@loewis, @akuchling
Files	archive_util.py: modified archive_utils.py arch-diff.txt: diff -u archive_util.orig.py archive_util.py archive-patch2: Updated version of the patch against the trunk

^{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/akuchling'
closed_at = <Date 2002-11-21.18:54:04.000>
created_at = <Date 2002-11-15.21:33:50.000>
labels = ['library']
title = 'archiver should use zipfile before zip'
updated_at = <Date 2002-11-21.18:54:04.000>
user = 'https://bugs.python.org/ooldham'

bugs.python.org fields:

activity = <Date 2002-11-21.18:54:04.000>
actor = 'akuchling'
assignee = 'akuchling'
closed = True
closed_date = None
closer = None
components = ['Distutils']
creation = <Date 2002-11-15.21:33:50.000>
creator = 'ooldham'
dependencies = []
files = ['686', '687', '688']
hgrepos = []
issue_num = 639118
keywords = []
message_count = 8.0
messages = ['13298', '13299', '13300', '13301', '13302', '13303', '13304', '13305']
nosy_count = 4.0
nosy_names = ['loewis', 'akuchling', 'nnorwitz', 'ooldham']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue639118'
versions = ['Python 2.3']

[ooldham]

The distutils archiver should attempt zipfile.py usage
before attempting a spawn of an external 'zip'.

The current code in archive_util.py attempts to spawn an
external 'zip' program for zip action, if this fails, an
attempt to import zipfile.py is made...

This bites folks who have 'old' or non-conforming zip
programs on windows platforms...

Have had a conversation about this with thellar, and he
suggested A: a bug report, B: changing code to attempt
import first, then take spawn action if that fails.

Since this is my first bug report, I am attaching a
archive_util.py that I have modified to do this... I also
modified this particular file to not use the '-q' option when
in verbose mode, for the zip spawn. Also modified so
that if in verbose mode, what gets added to zip file
during zipfile usage gets printed to stdio.

I tested the attached file for with and w/o verbose, and
test with and w/o zipfile.py to force the different code
path executions. Also forced error of having no zipfile.py
and no external zip program.

[nnorwitz]

Logged In: YES
user_id=33168

amk, you've been doing a lot of distutils stuff recently.
Do you have any comments?

[loewis]

Logged In: YES
user_id=21627

I have a meta comment: Ollie, thanks for your report. It would
be even more useful if you had attached a context or unified
diff (diff -c/-u) of the modified file, since that would simplify
merging you changes with other changes that the file has
seen (or will see until the patch is accepted).

[ooldham]

Logged In: YES
user_id=649833

attaching diff between archive_util.orig.py and modified
archive_util.py

[akuchling]

Logged In: YES
user_id=11375

Good idea!

Here's an updated patch against the CVS trunk. I've rearranged the code a bit to reduce the depth of indentation, but it still seems to work.

Ollie, can you please scan the patch and see if I introduced any problems with the rearrangement? If you don't spot anything wrong, I'll check it in.

[ooldham]

Logged In: YES
user_id=649833

A.M. Yes. I had considered this type of change as well.
Looks good, however, I have a few comments :)

1. I LIKE the log.info modification you made...

nit pick 1) Consider renaming new variable 'zipfile' to
something like 'haveZipModule'. Just for clarity...

nit pick 2) You left the comment lines below: 'except
DistutilsExecError' intact, only the indentation changed... I
removed this comment, as in my mind it no longer made
sense, since the function now attempts zipfile module first...
If you are going to leave it, at least re-word it...

nit pick 3) I re-worded the raise DistutilsExecError message...
to show the true order of what it could not do - as did the
original message.

[akuchling]

Logged In: YES
user_id=11375

On 1), zipfile is either the module object or None; it's not a Boolean,
so I'd like to keep it named 'zipfile'.

On 2), good point; edited down to
# XXX really should distinguish between "couldn't find
# external 'zip' command" and "zip failed".

On 3), another good point; fixed.

[akuchling]

Logged In: YES
user_id=11375

Checked in to CVS as rev. 1.15 of archive_util.py; thanks for your contribution!