Websockets 12.0, fixed index builder#5
Conversation
Reviewer's Guide by SourceryThis pull request refactors the hashing and metadata handling for wheel files, updates the HTML anchors and S3 metadata in the index builder, and adds support for websockets version 12.0 in the build configurations. A new meta.yaml file for websockets version 12.0 is also included. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @FeodorFitsner - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| remote_file_path = os.path.basename(file) | ||
|
|
||
| # extract and upload metadata | ||
| zip = zipfile.ZipFile(file) |
There was a problem hiding this comment.
suggestion: Consider using a context manager for ZipFile
Using a context manager (with zipfile.ZipFile(file) as zip:) ensures that the file is properly closed after its block of code is executed, even if an exception is raised.
| zip = zipfile.ZipFile(file) | |
| with zipfile.ZipFile(file) as zip: |
| metadata_filename = next( | ||
| filter(lambda f: f.endswith(".dist-info/METADATA"), zip.namelist()) | ||
| ) | ||
| f = zip.open(metadata_filename) |
There was a problem hiding this comment.
suggestion: Consider using a context manager for file handling
Using a context manager (with zip.open(metadata_filename) as f:) ensures that the file is properly closed after its block of code is executed, even if an exception is raised.
| f = zip.open(metadata_filename) | |
| with zip.open(metadata_filename) as f: | |
| content = f.read() |
| cf_bucket_name, | ||
| file, | ||
| remote_file_path, | ||
| wheel_hash=get_file_sha256(file), |
There was a problem hiding this comment.
suggestion (performance): Potential inefficiency in reading file twice
The file is read twice: once for calculating the SHA256 hash and once for uploading. Consider reading the file once and using the content for both operations to improve efficiency.
| wheel_hash=get_file_sha256(file), | |
| with open(file, 'rb') as f: | |
| file_content = f.read() | |
| wheel_hash = hashlib.sha256(file_content).hexdigest() |
| if not package_name in index: | ||
| index[package_name] = [] | ||
| elif not key.endswith("index.html"): | ||
| elif not key.endswith("index.html") and key.endswith(".whl"): |
There was a problem hiding this comment.
suggestion: Redundant condition in elif statement
The condition not key.endswith("index.html") is redundant because key.endswith(".whl") will never be true if the key ends with index.html. Consider removing the redundant condition.
| elif not key.endswith("index.html") and key.endswith(".whl"): | |
| elif key.endswith(".whl"): |
| # - job_name: 'Android: websockets' | ||
| # job_group: build_android | ||
| # FORGE_ARCH: android | ||
| # FORGE_PACKAGES: >- | ||
| # websockets:12.0 |
There was a problem hiding this comment.
suggestion: Remove commented-out job configurations
If these job configurations are not needed, consider removing them to keep the configuration file clean and maintainable.
| remote_file_path = os.path.basename(file) | ||
|
|
||
| # extract and upload metadata | ||
| zip = zipfile.ZipFile(file) |
There was a problem hiding this comment.
issue (code-quality): Don't assign to builtin variable zip (avoid-builtin-shadow)
Explanation
Python has a number ofbuiltin variables: functions and constants thatform a part of the language, such as
list, getattr, and type(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.
| @@ -56,15 +56,21 @@ def normalize(name): | |||
| package_name = parts[1] | |||
| if not package_name in index: | |||
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Simplify logical expression using De Morgan identities (
de-morgan) - Simplify sequence length comparison (
simplify-len-comparison)
flet-dev/python-build#5 has landed. The new v3.12 tarball relocates sysconfigdata's baked NDK paths at runtime AND ships libpython3.so as a proper linker script (with libpython3.12.so now carrying its DT_SONAME, per Feodor's follow-up patchelf fix). Both consumer-side band-aids in this repo become no-ops against that tarball — remove them. Removed: 1. `.ci/install_ndk.sh`: the `$HOME/ndk/<letter>` compat-symlink block. Existed because sysconfigdata baked `/home/runner/ndk/<letter>/...` compiler paths, so crossenv looked for clang there. PR flet-dev#5's tarball-side relocation consults `$NDK_HOME` (and a fallback list) at sysconfig import time, so the symlink is no longer needed. 2. `.github/workflows/build-wheels.yml`: the `for libpy in ...` loop that replaced libpython3.so with a GNU ld linker script `INPUT ( -lpython3.12 )`. PR flet-dev#5's `replace_libpython_stub` writes the same linker script in-place at tarball-assembly time, and the new SONAME on libpython3.12.so closes the link-name-fallback path that originally made the workaround necessary. The loop is now a no-op against any extracted tarball — delete it. bash -n + YAML validation both pass.
The original assertion `hasattr(blis, "py")` could never pass: blis's `__init__.py` is literally `from .cy import init; init()`, which loads `blis/cy.cpython-*.so` only. Python doesn't auto-import sibling submodules, so `blis.py` stays unset after `import blis` — regardless of arch or whether libcpp_shared is bundled. The docstring claimed the test was an x86_64 libcpp canary that failed because the published numpy-2.2.2-4 wheel had no `Requires-Dist: flet-libcpp-shared`, but that rationale is doubly stale: * numpy is now published at flet-dev#5 with `Requires-Dist: flet-libcpp-shared (>=27.2.12479018)`, so x86_64 Flet apps bundle libc++_shared.so. Confirmed in run 26836214663: `test_einsum` and `test_numpy_fft` both pass on x86_64 in the same job — neither could load numpy if libcpp were missing. * Even if numpy still lacked the dep, the assertion would fail on every platform, not just x86_64, because `import blis` never tries to load `blis/py.cpython-*.so` in the first place. The fix matches the test to its docstring's intent: explicitly `from blis import py` so `blis/py.cpython-*.so` actually dlopens, then assert both `blis.cy` and `blis.py` are present. If libcpp_shared ever goes missing again, both `.so` files fail to dlopen and both assertions catch it — so the test still serves as the libcpp canary it was meant to be, just correctly now.
1 participant
Summary by Sourcery
This pull request introduces support for Websockets version 12.0, enhances the upload and index builder scripts to handle SHA256 hashes for both wheel files and their metadata, and updates the AppVeyor build configuration to include the new Websockets version.