Zipimports #232

frmdstryr · 2017-11-15T19:00:19Z

I'm not sure why but git made me commit my other changes so this also includes adding autocomplete to the scintilla widget.

I can break it apart if needed.

codecov-io · 2017-11-15T19:31:41Z

Codecov Report

Merging #232 into master will increase coverage by 0.49%.
The diff coverage is 76.15%.

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   62.83%   63.33%   +0.49%     
==========================================
  Files         289      289              
  Lines       23303    23550     +247     
==========================================
+ Hits        14643    14915     +272     
+ Misses       8660     8635      -25

Impacted Files	Coverage Δ
enaml/compat.py	`56.89% <45.83%> (-5.06%)`	⬇️
enaml/core/import_hooks.py	`80.08% <93.9%> (+6.36%)`	⬆️
enaml/core/code_tracing.py	`78.14% <0%> (-3.11%)`	⬇️
enaml/qt/qt_widget.py	`53.81% <0%> (+0.36%)`	⬆️
enaml/layout/layout_manager.py	`92.74% <0%> (+0.51%)`	⬆️
enaml/widgets/widget.py	`62.38% <0%> (+1.83%)`	⬆️
enaml/widgets/constraints_widget.py	`85.71% <0%> (+3.57%)`	⬆️
enaml/styling.py	`79.63% <0%> (+4%)`	⬆️
enaml/qt/qt_container.py	`84.19% <0%> (+5.65%)`	⬆️
enaml/widgets/container.py	`92.3% <0%> (+7.69%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd07a3a...2dbe193. Read the comment docs.

MatthieuDartiailh

I left a number of inline comment and I focused on the zip importer. Some of them may be related to me not following completely what happens inside the zip importer in particular related to the handling of cached files. On the whole this looks good, but could you please ensure that the lines are no longer than 79 characters (PEP 8), it does not appear to always be the case.
Also could you please make a separate PR for the autocompletion ? I will more time to review this and I think it requires more discussion.
Thanks for your work on this.

MatthieuDartiailh · 2017-11-16T08:08:30Z

enaml/core/import_hooks.py

@@ -337,7 +339,27 @@ def _get_magic_info(self, file_info):
            timestamp = struct.unpack('i', cache_file.read(4))[0]
        return (magic, timestamp)

-    def compile_code(self):
+    def read_source(self, src_path):


I do not see the point of passing explicitly the src_path, everything is in the file_info so I would simply use that.

See further comments for how to avoid code duplication in the ZipImporter

MatthieuDartiailh · 2017-11-16T08:09:02Z

enaml/core/import_hooks.py

+        """
+        return read_source(src_path)
+
+    def get_src_mod_time(self, src_path):


MatthieuDartiailh · 2017-11-16T08:09:09Z

enaml/core/import_hooks.py

+        """ Get the last modified time of the given source path """
+        return int(os.path.getmtime(src_path))
+
+    def compile_code(self, src_path):


MatthieuDartiailh · 2017-11-16T08:09:43Z

enaml/core/import_hooks.py

-        return (code, file_info.src_path)
+        return (code, src_path)
+
+    def get_cached_code(self, src_path):


MatthieuDartiailh · 2017-11-16T08:09:59Z

enaml/core/import_hooks.py

+    def get_cached_code(self, src_path):
+        """ Loads and returns the code object for the Enaml module from
+        the cache if it exists.
+        Paramters


Missing blank line before parameters section

MatthieuDartiailh · 2017-11-16T08:15:42Z

enaml/core/import_hooks.py

+        return super(EnamlZipImporter, self).get_src_mod_time(self.archive_path)
+
+    def read_source(self, src_path):
+        """ Overridden to read the source from the currently opened archive 


MatthieuDartiailh · 2017-11-16T08:23:02Z

tests/test_zipimporter.py

+            zf.writestr(dst, generate_cache(src), compress_type=zipfile.ZIP_DEFLATED)
+
+
+def with_library(f):


This could be turned into a pytest fixture. I can do it if you are not familiar with them.

Yes that would be good.

MatthieuDartiailh · 2017-11-16T08:34:09Z

enaml/core/import_hooks.py

+                        os.path.exists(archive_path)):
+
+                    # Path where code should be within the archive
+                    code_path = '/'.join(pkgpath+[leaf])


Why not os.path.join ?

MatthieuDartiailh · 2017-11-16T08:43:01Z

enaml/core/import_hooks.py

+
+        """
+
+        # Try to load from the cache on disk


I am not sure to follow in which case this can actually apply. Could you explain it to me ?

MatthieuDartiailh · 2017-11-16T08:45:24Z

enaml/core/import_hooks.py

+            # Otherwise, compile from source and attempt to cache it on the system
+            code_path = os.path.relpath(file_info.src_path,
+                                        self.archive_path).replace("\\", "/")
+            result = self.compile_code(code_path)


Where will the cache be written here ? To me it appears, this should lead to write the cache in the archive.

…orts

frmdstryr · 2017-11-16T15:24:00Z

Ok thanks for looking. I pulled out the scintilla changes and tried to address all the pep8 and doc issues. I also got rid of passing the src_file changes as suggested and instead used an attribute and some method overrides.

The caching is a bit confusing. There's two possibilities:

__enamlcache__ exists on the filesystem from previously generated cache when the code is compiled. I tried to retain this as it prevents having to open and read the zip file.
__enamlcache__ exists within the .zip file.

I'm not sure at the moment if the standard importer will cover importing from 1 even if the code was compiled from source within the zip file. If this is the case, then the get_cached_code can be removed and left unchanged as the standard importer will find and use this.

However the source_exists and get_source_modified_time methods are still required because these have to be done on the .zip archive and not where python expects the file to normally be on the file system.

As for the .replace("\\","/"). I wrote this a year ago so I don't remember exactly, I think this was because even though the ZipFile supports \ separators it still had / separators in the zip file (but it may have been that cx_Freeze was doing this). Nevertheless using replace ensures it always works (the app I used this for supported win, mac, & linux). Edit: Also windows 7 and 8.1 (and I assume 10) handle / path separators the same as \.

MatthieuDartiailh · 2017-11-16T15:27:52Z

I guess what I find the more confusing about the first possibility you evoke concerning the cache is the location of the cache file. Does it live outside the archive ? and how did it end up there, and how can it be found ?
Sorry for the perhaps stupid question but I fail to see how a cache file for a file inside an archive could be generated outside the archive and still be findable by the import machinery.
I will look over the changes tomorrow.

frmdstryr · 2017-11-16T15:30:00Z

My understanding is the EnamlImporter attempts to create this in in https://github.com/nucleic/enaml/blob/master/enaml/core/import_hooks.py#L294

during the compile_code
https://github.com/nucleic/enaml/blob/master/enaml/core/import_hooks.py#L356

so yes, if it has write access it will create the cache outside the zip file (if the source exists in the zip). But if the __enamlcache__ is in the zip it will not attempt to create the cache (as the compile_code is never called).

frmdstryr · 2017-11-16T16:20:41Z

I looked into this more, it turns out it always attempts to write the cache but fails since the cache_file and cache_dir point to within the library.zip and hence the situation in 1 never occurs (mkdir fails).

EnamlFileInfo(src_path='library.zip/buttons.enaml', cache_path='library.zip/__enamlcache__/buttons.enaml-py35-cv25.enamlc', cache_dir='library.zip/__enamlcache__')

EnamlFileInfo(src_path='library.zip/package/subpackage/slider.enaml', cache_path='library.zip/package/subpackage/__enamlcache__/slider.enaml-py35-cv25.enamlc', cache_dir='library.zip/package/subpackage/__enamlcache__')

So, should I make it write cache to outside the zip or is that undesired behavior?

MatthieuDartiailh · 2017-11-16T16:35:21Z

That is what I suspected (and also why I failed to understand why we were looking for a cache outside the archive). I am not being familiar with working with zip archive but I guess it would be pretty surprising to modify the content of the archive. So I would say that we can bypass the first cache check and expect to fail writing the cache. However I guess we should first check how Python handles this case.

sccolbert · 2017-11-16T18:40:31Z

I would follow whatever Python is doing. The .enamlc files and correlating logic basically mirrors python's .pyc logic. If Python's zip importer is not writing cache files, neither should we.

MatthieuDartiailh · 2017-11-17T09:07:11Z

Looking at the Python ZipImporter https://github.com/python/cpython/blob/master/Modules/zipimport.c I do not seed any logic related to writing cache (but I may be wrong).

MatthieuDartiailh · 2017-11-17T10:07:11Z

This historical PEP goes into the same direction.

- on windows the tests were crashing because we were non-using universal newlines when reading the files - fix issue with alternative encoding (slight reformating of Python 2 compatibility code) - fix some PEP8 issues - use pytest fixtures in the tests

MatthieuDartiailh · 2017-11-17T11:29:05Z

I fixed some issues while porting the tests to pytest.
First on windows all tests were failing because files were not opened with universal newline support. Second the importer was not honoring the encoding of the files. I don't yet have tests for this but I plan to add some at one point. I also fixed some PEP 8 issues.

frmdstryr · 2017-11-17T18:20:06Z

Thanks, merged your changes and also removed the unnecessary caching checks and disabled the _write_cache since it doesn't work with the zip archive.

Basically fixing my own mistake.

MatthieuDartiailh · 2017-11-26T15:38:32Z

This is good to go for me. The only thing that could be improved is adding tests to check that we handle properly encoding declarations. @frmdstryr if you have time to do it feel free to do it.
I will merge in a week if nobody comment before.

MatthieuDartiailh · 2017-12-01T14:41:39Z

@sccolbert any comment on this ?

sccolbert · 2017-12-01T15:12:06Z

Looks generally fine to me. I didn't audit the logic, but if it works, 👍

MatthieuDartiailh · 2017-12-09T17:31:46Z

Merging, thanks @frmdstryr for the contribution and sorry for the delay.

Closes #115

frmdstryr added 4 commits November 15, 2017 13:52

Update zipfile import and with test for travis

5586225

Merge updates

ea64621

Make py 2.7 happy as a clam

5242495

Pep8 and only decode in py3

d0254a4

frmdstryr mentioned this pull request Nov 16, 2017

Add EnamlZipImporter #214

Closed

MatthieuDartiailh requested changes Nov 16, 2017

View reviewed changes

frmdstryr added 5 commits November 16, 2017 09:15

Merge branch 'master' of https://github.com/nucleic/enaml into zipimp…

a9f94ca

…orts

Refactor to address pep8 and @MatthieuDartiailh comments

128dd50

Revert scintilla changes

0492b86

Revert all of them

9077d4e

Make travis retry

dcba513

frmdstryr added 2 commits November 17, 2017 13:10

Merge and disable attempting to write cache as it will always fail

85fdbc1

Remove now unused source_exists method

39555cc

compat: make detect_encoding closer to Python 2

2dbe193

Basically fixing my own mistake.

MatthieuDartiailh approved these changes Nov 26, 2017

View reviewed changes

MatthieuDartiailh merged commit 0a60e15 into nucleic:master Dec 9, 2017

MatthieuDartiailh mentioned this pull request Jan 9, 2018

with enaml.imports() support executing from .zip file (PEP 441) #115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zipimports #232

Zipimports #232

frmdstryr commented Nov 15, 2017

codecov-io commented Nov 15, 2017 •

edited

Loading

MatthieuDartiailh left a comment

MatthieuDartiailh Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

frmdstryr Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

MatthieuDartiailh Nov 16, 2017

frmdstryr commented Nov 16, 2017 •

edited

Loading

MatthieuDartiailh commented Nov 16, 2017

frmdstryr commented Nov 16, 2017 •

edited

Loading

frmdstryr commented Nov 16, 2017

MatthieuDartiailh commented Nov 16, 2017

sccolbert commented Nov 16, 2017

MatthieuDartiailh commented Nov 17, 2017

MatthieuDartiailh commented Nov 17, 2017

MatthieuDartiailh commented Nov 17, 2017

frmdstryr commented Nov 17, 2017

MatthieuDartiailh commented Nov 26, 2017

MatthieuDartiailh commented Dec 1, 2017

sccolbert commented Dec 1, 2017

MatthieuDartiailh commented Dec 9, 2017

		zf.writestr(dst, generate_cache(src), compress_type=zipfile.ZIP_DEFLATED)


		def with_library(f):

Zipimports #232

Zipimports #232

Conversation

frmdstryr commented Nov 15, 2017

codecov-io commented Nov 15, 2017 • edited Loading

Codecov Report

MatthieuDartiailh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frmdstryr commented Nov 16, 2017 • edited Loading

MatthieuDartiailh commented Nov 16, 2017

frmdstryr commented Nov 16, 2017 • edited Loading

frmdstryr commented Nov 16, 2017

MatthieuDartiailh commented Nov 16, 2017

sccolbert commented Nov 16, 2017

MatthieuDartiailh commented Nov 17, 2017

MatthieuDartiailh commented Nov 17, 2017

MatthieuDartiailh commented Nov 17, 2017

frmdstryr commented Nov 17, 2017

MatthieuDartiailh commented Nov 26, 2017

MatthieuDartiailh commented Dec 1, 2017

sccolbert commented Dec 1, 2017

MatthieuDartiailh commented Dec 9, 2017

codecov-io commented Nov 15, 2017 •

edited

Loading

frmdstryr commented Nov 16, 2017 •

edited

Loading

frmdstryr commented Nov 16, 2017 •

edited

Loading