GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix problems with import and zipimport in particular by jeff5 · Pull Request #31 · jython/jython3

Conversation

@jeff5

@jeff5 jeff5 commented Jun 5, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jeff5
[Trivial] White space, comments and line-wrap only in zipimport module.
8eea4ef
@jeff5 jeff5 self-assigned this Jun 5, 2017
@stuaxo

stuaxo commented Jun 6, 2017

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jeff5
Expose PyZipImporter through CoreExposed.includes
8ec8720
This cures the constructor-related exception on attempts at importing
modules, but there are still problems with PyZipImporter. Also improved
some comments.
@jeff5

jeff5 commented Jun 11, 2017

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jeff5
Re-work PyZipImoprter construction closer to CPython and add unit test.
5ed6d37
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

jeff5 commented Jun 12, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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 .

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jeff5 added 5 commits June 14, 2017 20:23
@jeff5
Add comment specifying zipimporter.get_data and a test
c47bf0c
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.
@jeff5
Conform behaviour of zipimporter.get_data() to CPython and enable test.
4d008f0
@jeff5
Add comments (including Javadoc) to PyZipImporter and re-order tests.
db133ff
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.
@jeff5
Have PyZipImportTest create its own test material.
b0431b1
@jeff5
Corrections to zipimporter.get_filename and a test.
0b14ff2
Test against careful observation of equivalent behaviour in CPython,
when a .py file name should be returned and when a .class.
@jeff5

jeff5 commented Jul 17, 2017

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@jeff5 jeff5

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@jeff5 @stuaxo
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.