Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problems with import and zipimport in particular #31

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jeff5
Copy link
Member

@jeff5 jeff5 commented Jun 5, 2017

Beginning work on the failure that happens on startup of Jython. First commit is just whitespace and comment. The actual problem is that the constructor has 3 args, but is called (correctly) with just the archive path.

I can fix this by moving some path-wrangling from __new__ into a one-arg constructor. However, shouldn't __new__ be called rather than the constructor by reflection? But that's what's on sys.path_hooks.

@jeff5 jeff5 self-assigned this Jun 5, 2017
@stuaxo
Copy link

stuaxo commented Jun 6, 2017

Not sure the exact bit of code that you are talking about, but sounds like a job for *args ?

This cures the constructor-related exception on attempts at importing
modules, but there are still problems with PyZipImporter. Also improved
some comments.
@jeff5
Copy link
Member Author

jeff5 commented Jun 11, 2017

@stuaxo When I write __new__ here I mean this code: https://github.com/jython/jython3/blob/master/src/org/python/modules/zipimport/PyZipImporter.java#L63.

This stuff is an example of how Jython makes Python objects. It's clever, and not terribly well explained anywhere, but the best explanation I know is here: https://wiki.python.org/jython/PythonTypesInJava.

The problem referred to is that PyZipImporter is being treated as a conventional Java class, and so the Java constructor is called (https://github.com/jython/jython3/blob/master/src/org/python/modules/zipimport/PyZipImporter.java#L55), which expects 3 arguments. And this is corrected in my latest commit here: 8ec8720#diff-990d9685668c8a38f104c60c2dac735dL147

However, this doesn't suddenly make Jython 3 work, so there's more to find.

We use java.nio.Path extensively to achieve platform independence and
to support (and preserve) relative paths exactly as CPython does.
Because Jython does not yet work well enough to run standard unit tests,
we start a thorough Java-based test.
@jeff5
Copy link
Member Author

jeff5 commented Jun 12, 2017

Getting quite nice conformance now with CPython as far as construction of a zipimporter goes:

> dist\bin\jython -S
Jython 3.5.1a1+ (, Jun 12 2017, 22:23:54)
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_131
>>> from zipimport import zipimporter as zi
>>> archive = r'dist\Lib\test\test_importlib\namespace_pkgs\missing_directory.zip'
>>> foo = archive + r'\foo'
>>> za = zi(archive)
>>> za.archive
'dist\\Lib\\test\\test_importlib\\namespace_pkgs\\missing_directory.zip'
>>> za.prefix
''
>>> za._files
{'bar\\': ('dist\\Lib\\test\\test_importlib\\namespace_pkgs\\missing_directory.zip\\bar\\', 0, 0, 0, -1, 1336121151000, 1336121151000, 0), 'foo\\one.py': ('dist\\Lib\\test\\test_importlib\\namespace_pkgs\\missing_directory.zip\\foo\\one.py', 0, 26, 26, -1, 1336121156000, 1336121156000, 680762358), 'bar\\two.py': ('dist\\Lib\\test\\test_importlib\\namespace_pkgs\\missing_directory.zip\\bar\\two.py', 0, 35, 35, -1, 1336121151000, 1336121151000, 3195395652)}
>>> zf = zi(foo)
>>> zf.archive
'dist\\Lib\\test\\test_importlib\\namespace_pkgs\\missing_directory.zip'
>>> zf.prefix
'foo\\'
>>> zf._files is za._files
True
>>>

We copy CPython, which does not have absolute file paths in the _files cache.

Use of relative paths has generally been problematic in Jython, although it's what CPython does extensively. We have our own idea of the current directory, and some complex path manipulation using it, because it seemed Java would not pay attention to the changed value of the current working directory in property user.dir. I remember struggling with this. However, see https://stackoverflow.com/questions/840190/changing-the-current-working-directory-in-java/13981910#13981910 .

The test expresses the behaviour of get_data in CPython 3.5.2, and
fails in every call at present (on Windows), so is disabled in the unit
test.
Also re-named some local members. The objective is (only) to be clear
about the specification of the methods, how it works internally.
Un-expose some methods that do not have CPython counterparts.

All preparatory to systematic tests.
Test against careful observation of equivalent behaviour in CPython,
when a .py file name should be returned and when a .class.
@jeff5
Copy link
Member Author

jeff5 commented Jul 17, 2017

There are many more things I'd like to change in PyZipImporter, but given that it long-since ceased to be what prevents running Jython, it's not the most useful thing to address. Could probably merge this and tackle something else. Prefer not to squash-merge I think, in order to make the steps/reasoning apparent to subsequent contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants