[Refactoring... WIP] Implement CPython-compatible gzip.decompress, restore uzlib. Enable both for atmel-samd #1274
Conversation
extmod/modgzip.c
Outdated
| // information anywhere (which feels like an odd oversight, and some third | ||
| // parties seem to agree: the pure-Python gzip implementation "gzippy" | ||
| // explicitly has a Header class which can be read from) | ||
|
|
There was a problem hiding this comment.
Can you use uzlib_gzip_parse_header() to do the parsing?
There was a problem hiding this comment.
Had I known that function existed, yep, probably could have. It'd require a bit more tinkering to moduzlib.c to rebase to that, but... seems sane to me. Looks like they do the same "discard most everything" logic I do.
There was a problem hiding this comment.
This refactor was WAY easier than I thought and the final implementation is significantly cleaner, thank you for this!
extmod/modgzip.h
Outdated
| extern mp_obj_t mod_gzip_decompress(size_t n_args, const mp_obj_t *args); | ||
|
|
||
| MP_DECLARE_CONST_FUN_OBJ_VAR_BETWEEN(mod_gzip_decompress_obj); | ||
|
|
There was a problem hiding this comment.
Do you need this? I can't see that they are used outside the module.
There was a problem hiding this comment.
I basically copied this file from some other module so this header file existing is mostly a leftover. Not really needed I don't think?
| return mp_obj_new_int_from_uint(crc ^ 0xffffffff); | ||
| } | ||
| STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_uzlib_crc32_obj, 1, 2, mod_uzlib_crc32); | ||
|
|
There was a problem hiding this comment.
Look like you've copied mod_binascii_crc32() here.
I think you can just use mod_binascii_crc32_obj() directly instead protected by MICROPY_PY_UBINASCII_CRC32.
There was a problem hiding this comment.
I did copy that function directly - I made this separate so there'd be no config clashing (if MICROPY_PY_UBINASCII_CRC32 is defined, it shouldn't impact something in uzlib, I guess?)
Happy to refactor this out to a generic crc32 function that both modules use behind their own mpconfigport.h flags, though.
There was a problem hiding this comment.
Admittedly this function isn't even used by my gzip implementation - it was used in my Python prototype of importing gzipped Python modules. Can also just... rip this out, though it seems nice to have in uzlib for compatibility with CPython's zlib
ports/atmel-samd/mpconfigport.h
Outdated
| @@ -23,7 +23,7 @@ | |||
| // Turn off for consistency | |||
| #define MICROPY_CPYTHON_COMPAT (0) | |||
| #define MICROPY_MEM_STATS (0) | |||
| #define MICROPY_DEBUG_PRINTERS (0) | |||
| #define MICROPY_DEBUG_PRINTERS (1) | |||
| #define MICROPY_ENABLE_GC (1) | |||
There was a problem hiding this comment.
I suppose you forgot to remove this debug option before committing.
|
Would you mind refactoring this into the CircuitPython shared-bindings and common-hal structure? That way it'll list with all of our other modules in the docs. time, os and struct have already been done here: https://github.com/adafruit/circuitpython/tree/master/shared-bindings I think for making uzlib a subset of CPython's zlib you'll need to remove DecompIO or move it to a new module. While you do that I'll make some space for you in the CPX crickit build. |
|
Now that I think about this more, let's make it M4 only. The M0 is quickly running out of code space and can't hold much code in memory anyway. Thanks! |
|
Sorry for the over a month of radio silence on this - burned out for a bit there on OSS work (and the project this PR was ultimately to be in support of). I'm back and plan to fix this branch up some time in the next week or two - looks like the only conflicts to merge in are localization-related, thankfully. |
|
@klardotsh no rush :) whenever you're ready - take breaks whenever ya need! |
8609f47 to
45258d7
Compare
|
The cause of the build failures is that flash is too full on circuitplayground_express_crickit -- disabling the new feature for that specific board might allow the build to be green. The translations need to be refreshed again (no surprise there) and there are conflict(s) in mpconfigport.h to be resolved which shouldn't be too subtle. |
7295768 to
bf3ff73
Compare
|
I'm going to close this. Please reopen when the refactoring is complete. Thanks! |
This on its own is likely of only marginal use (though I'm sure someone will find a use for it). My real motivation for this PR lies in KMKfw/kmk_firmware#52 (comment), where I'd eventually like to be able to both import a single gzipped Python module (this one is arguably less useful, but again, I'm sure some advanced user would find use for it - this functionality is not part of this pull request), and eventually import modules from a ZIP folder. Since compressed ZIPs and GZIP files use the same algo under the hood (
DEFLATE), this felt like a great starting point to both get an understanding of CircuitPython's internals (and especially the buffer system), as well as contribute a useful CPython backport to the project.An example of this in use is below. The file sizes are also listed here for giggles.