python / cpython

The Python programming language
https://www.python.org
Other
62.84k stars 30.09k forks source link

Import from Zip Archive #37552

Closed pfmoore closed 21 years ago

pfmoore commented 21 years ago
BPO 645650
Nosy @gvanrossum, @loewis, @pfmoore
Files
  • python.diff: Patch against current CVS - includes documentation & test changes
  • foo.zip: Test file needed for the new zip import test
  • zip-patch.zip: neal's diff
  • Newpatch-Dec01.zip
  • Newpatch-Dec06.zip
  • 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: ```python assignee = None closed_at = created_at = labels = ['interpreter-core'] title = 'Import from Zip Archive' updated_at = user = 'https://github.com/pfmoore' ``` bugs.python.org fields: ```python activity = actor = 'gvanrossum' assignee = 'nnorwitz' closed = True closed_date = None closer = None components = ['Interpreter Core'] creation = creator = 'paul.moore' dependencies = [] files = ['4751', '4752', '4753', '4754', '4755'] hgrepos = [] issue_num = 645650 keywords = ['patch'] message_count = 16.0 messages = ['41840', '41841', '41842', '41843', '41844', '41845', '41846', '41847', '41848', '41849', '41850', '41851', '41852', '41853', '41854', '41855'] nosy_count = 5.0 nosy_names = ['gvanrossum', 'loewis', 'nnorwitz', 'paul.moore', 'ahlstromjc'] pr_nums = [] priority = 'normal' resolution = 'rejected' stage = None status = 'closed' superseder = None type = None url = 'https://bugs.python.org/issue645650' versions = ['Python 2.3'] ```

    pfmoore commented 21 years ago

    This patch supersedes patch 492105.

    This is an updated version of the patch for imports from a zip file. This version is against current CVS, and includes documentation and tests.

    The patch builds on Windows, and runs OK. I haven't (yet) run any comprehensive tests, as this is the first time I've set up a build environment, and I'm not sure how to run the tests yet :-(

    pfmoore commented 21 years ago

    Logged In: YES user_id=113328

    Uploaded file foo.zip, needed (in lib\test directory) for the changes to test_import.py

    pfmoore commented 21 years ago

    Logged In: YES user_id=113328

    Bad news. I've just hit problems with the patched Python - when I start python.exe, it reports "cannot import site" and seems unable to locate the standard library. (This when I run python from the PCBuild directory).

    I built a clean CVS version, and don't see this problem.

    Can anyone else reproduce this, or help me diagnose it? I don't understand Jim's changes well enough to see what might be the problem (I updated the patch purely mechanically).

    d21744ff-f396-4c71-955e-7dbd2e886779 commented 21 years ago

    Logged In: YES user_id=33168

    Paul, thanks for taking this up. I made very little progress on this I have attached my version of the diffs.

    I had some problems with the original patch which I was never able to correct. For example, I think it was not possible to run python from a local build env't. It appeared that python had to be installed for everything to work. See the problem in bug 586680 (also in site.py). That should probably be addressed and was part of my problem.

    I tried to simplify the code some, so you should see some differences between the patch I attached and the original. These are not as important as making the patch work properly, both when installed and running from a build env't.

    I will try to take a look at your problem later, but it may take a while. Let me know if you make progress.

    pfmoore commented 21 years ago

    Logged In: YES user_id=113328

    My problem turns out to be an interaction with the new "universal newline support". In find_module, within the switch(path_type), case PY_IMP_LISTDIR, the section of code marked / Search the list of suffixes: .pyc, .pyd, ... \/ tries to do fopen(buf, fdp->mode). But .py files are now mode 'U' (universal newline) which is not a valid mode for fopen (). I've made a fix, but I'm not sure how reliable it is.

    On a related note, I'm not 100% sure how robust the code is in the presence of case-insensitive filesystems. I'm way out of my depth in this code, so I don't know if I'll be able to fix it, but I'll have a look (and I'll look at your version of the patch, as well).

    pfmoore commented 21 years ago

    Logged In: YES user_id=113328

    I've now taken Neal's patch and integrated my changes. I'll check it through again, and run a few more tests, and then upload the result (I've had problems with cvs update, so I'm downloading a new copy of CVS python first).

    Most things look OK. There are a couple of issues with case- insensitive filesystems.

    The first is that the cache of os.listdir results is (case- sensitively) keyed on directory name. I don't see this as an issue as (a) I'd expect the case used to remain constant (who's going to change a directory in sys.path at runtime to the same name with a different case???) and (b) it's only an efficiency hit, not an error, in any case.

    The second issue is more problematic. Zip files on sys.path are recognised (see add_zip_names()) by the extension ".zip" (case sensitively). It's arguable that on case-insensitive filesystems, this check should also be case insensitive. But finding that a file is on a case-insensitive filesystem is, I believe, not possible. And using a case-insensitive test all the time isn't right either. I'd say that we should just *define* the algorithm as only treating files ending in ".zip" (case sensitive) as zip files. It doesn't seem to be a problem in practice.

    I'll try to find some way to fit that into the documentation...

    Apart from this issue, I've seen no other problems with the patch.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 21 years ago

    Logged In: YES user_id=21627

    I certainly agree with only treating .zip as indication for zipfiles. On a case insensitive file system, you can put .zip into sys.path and it will still find .ZIP.

    pfmoore commented 21 years ago

    Logged In: YES user_id=113328

    Aargh. It looks like Neal's patch is wrong - it didn't include the changes to the files in the PC directory. Also it has a lot of strange changes, like replacing PyEval_CallObject with PyObject_Call - I don't see how this relates to zipfiles.

    I wasted most of this morning trying to juggle too many patches, to no avail. I think I'm going to have to revert to my patch, get it applied to the CVS version, and upload that. Neal, I agree with a lot of the changes you made, but they'll need redoing - can you apply my patch to a clean checkout and redo your tidying up?

    The patch I'm uploading with this comment builds cleanly for me on Windows 2000, and passes the supplied tests. You need to apply the patch, then put the 2 included zipfiles in the lib/test directory.

    I'd be very grateful if someone could test this on Unix and/or Mac, and update it where necessary. I've not tested these platforms, but I'm assuming that Jim's original code worked there. There are also other areas which seem strange (the big block of deleted code in sysmodule.c worries me - but I see no failures because of it, so I've left it unchanged) and comments would be very helpful, but in the absence of anything better I'm sticking with "it worked when I tried it"...

    d21744ff-f396-4c71-955e-7dbd2e886779 commented 21 years ago

    Logged In: YES user_id=33168

    Hmmm, I thought I used to have changes to PC. Not sure, though. I'll try to take a look at the diffs and test on unix later this evening. I can also add back in any of my changes after you get everything working.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 21 years ago

    Logged In: YES user_id=21627

    Paul, there is sentiment on python-dev that this patch is unacceptable, because it is incomprehensible. I can sympathize with these feelings, so we need to take the possibility into account that this won't go into Python 2.3.

    pfmoore commented 21 years ago

    Logged In: YES user_id=113328

    Understood (I've been watching python-dev). I'm happy to keep working on this while a consensus is reached. Actually, I don't think there's much more I can do, I'm hoping that users of other OS's can test this now....

    pfmoore commented 21 years ago

    Logged In: YES user_id=113328

    Understood (I've been watching python-dev). I'm happy to keep working on this while a consensus is reached. Actually, I don't think there's much more I can do, I'm hoping that users of other OS's can test this now....

    d21744ff-f396-4c71-955e-7dbd2e886779 commented 21 years ago

    Logged In: YES user_id=33168

    Paul, I didn't get a chance to review/test the patch. But as I just said in python-dev, I think it's worthwhile. Guido seems to agree. I'll try to test on Linux soon.

    61337411-43fc-4a9c-b8d5-4060aede66d0 commented 21 years ago

    Logged In: YES user_id=21627

    I've now tested this patch on Unix. Support for compressed files didn't work from the build directory, since the zlib import happens before site.py is run. To work around this issue, I import zlib in site.py if we run from the build directory. I wonder whether the zlib import could be delayed until a compressed zipfile is encountered.

    Regenerated patch, attached as Newpatch-Dec06.zip (there, zipped-uncompressed.zip actually does contain uncompressed files).

    a91270f9-3c6f-42ef-a627-c523b1e6bf04 commented 21 years ago

    Logged In: YES user_id=64929

    Support for compressed files didn't work from the build directory, since the zlib import happens before site.py is run.

    I imported zlib before site.py is imported because site.py may itself be in a compressed zip archive. It looks like your trick of importing zlib in site.py works, and there is no need to limit it to running from the build directory since "import zlib" is cheap. An alternative is to try a zlib import after site.py is imported.

    As noted, this patch may not be accepted anyway, so I will stand by on this and any other problems.

    gvanrossum commented 21 years ago

    Logged In: YES user_id=6380

    I'm going with Just's version. However, some of the code here may still make it into 2.3a2 (the bootstrap related patches).