feat: scaffold portablemc 5.x build and packaging#8
Merged
Conversation
Create build scripts and package structure for portablemc 5.x wheel generation Co-authored-by: PythonChicken123 <101510622+PythonChicken123@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideScaffolds support for building and vendoring the portablemc 5.x native extension from maturin-generated wheels, updating wheel discovery, extension detection, extraction logic, and the JVM bootstrap error message/instructions. Flow diagram for updated extraction of native extension from wheelflowchart TD
A[Start _extract_pyd_from_wheel] --> B[Create portablemc package dir if needed]
B --> C[Open wheel as zip file]
C --> D[Set prefixes to portablemc/ and portablemc_py/]
D --> E[Scan zip names to find first matching prefix]
E --> F{Found matching prefix?}
F -- No --> G[Log warning: No portablemc package found]
G --> H[Return False]
F -- Yes --> I[Initialize extracted counter to 0]
I --> J[For each name in zip file]
J --> K{Name starts with found prefix?}
K -- No --> J
K -- Yes --> L{Name has __pycache__ or ends with / ?}
L -- Yes --> J
L -- No --> M{Name contains .dist-info?}
M -- Yes --> J
M -- No --> N[Strip found prefix to get rel]
N --> O{rel is empty?}
O -- Yes --> J
O -- No --> P[Replace portablemc_py with _portablemc in rel]
P --> Q[Compute dest path under portablemc package dir]
Q --> R[Create dest parent dirs]
R --> S[Read data from zip entry]
S --> T[Write data to dest]
T --> U[Increment extracted counter]
U --> J
J --> V[All names processed]
V --> W[Log number of extracted files and wheel name]
W --> X{extracted > 0?}
X -- Yes --> Y[Return True]
X -- No --> Z[Return False]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The error message in
resolverefers specifically to_portablemc.pyd, which is Windows-only; consider making this platform-agnostic (e.g., mentioning.pyd/.soor “native extension”) to avoid confusion on Unix-like systems. - In
_extract_pyd_from_wheel, therel = rel.replace("portablemc_py", "_portablemc")transformation may unintentionally rewrite occurrences ofportablemc_pyin non-module parts of the path; tightening this to only affect the expected filename/module segments (e.g., basename checks) would be safer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The error message in `resolve` refers specifically to `_portablemc.pyd`, which is Windows-only; consider making this platform-agnostic (e.g., mentioning `.pyd`/`.so` or “native extension”) to avoid confusion on Unix-like systems.
- In `_extract_pyd_from_wheel`, the `rel = rel.replace("portablemc_py", "_portablemc")` transformation may unintentionally rewrite occurrences of `portablemc_py` in non-module parts of the path; tightening this to only affect the expected filename/module segments (e.g., basename checks) would be safer.
## Individual Comments
### Comment 1
<location path="scripts/local_portablemc.py" line_range="58-60" />
<code_context>
- for whl in sorted(_WHEEL_SEARCH.glob("portablemc-*.whl")):
- return whl
+ # Try different wheel naming patterns (maturin uses portablemc_py-*)
+ patterns = ["portablemc_py-*.whl", "portablemc-*.whl"]
+ for pattern in patterns:
+ for whl in sorted(_WHEEL_SEARCH.glob(pattern), reverse=True):
+ return whl # Return most recent
return None
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Wheel selection uses lexicographic order and prefers `portablemc_py-*` even if a newer `portablemc-*` exists.
This contradicts the “Return most recent” intent: selection is based on filename ordering within each pattern, plus unconditional preference for `portablemc_py-*`. If actual recency matters, consider using `st_mtime` or parsed versions to compare wheels, and make the precedence between `portablemc_py-*` and `portablemc-*` explicit when their versions differ.
</issue_to_address>
### Comment 2
<location path="scripts/jvm_boot_glue.py" line_range="2246" />
<code_context>
- "Or ensure the wheel is available at:\n"
- f" {_SCRIPTS_DIR / 'portablemc' / 'target' / 'wheels'}"
+ "portablemc 5.x environment initialization failed.\n\n"
+ "The native extension (_portablemc.pyd) is required but not found.\n\n"
+ "To fix this, run the build script:\n"
+ f" python {pmc_root / 'build_wheel.py'}\n\n"
</code_context>
<issue_to_address>
**issue:** Error message only mentions `.pyd` even though `.so` is now supported elsewhere.
Since `_pyd_present` now also checks for `.so` artifacts, this message should either reference both `_portablemc.pyd` and `_portablemc.so`, or be rephrased in a platform-neutral way (e.g., “native extension (_portablemc.*) is required”) to avoid confusing non-Windows users.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+58
to
+60
| patterns = ["portablemc_py-*.whl", "portablemc-*.whl"] | ||
| for pattern in patterns: | ||
| for whl in sorted(_WHEEL_SEARCH.glob(pattern), reverse=True): |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Wheel selection uses lexicographic order and prefers portablemc_py-* even if a newer portablemc-* exists.
This contradicts the “Return most recent” intent: selection is based on filename ordering within each pattern, plus unconditional preference for portablemc_py-*. If actual recency matters, consider using st_mtime or parsed versions to compare wheels, and make the precedence between portablemc_py-* and portablemc-* explicit when their versions differ.
| "Or ensure the wheel is available at:\n" | ||
| f" {_SCRIPTS_DIR / 'portablemc' / 'target' / 'wheels'}" | ||
| "portablemc 5.x environment initialization failed.\n\n" | ||
| "The native extension (_portablemc.pyd) is required but not found.\n\n" |
Contributor
There was a problem hiding this comment.
issue: Error message only mentions .pyd even though .so is now supported elsewhere.
Since _pyd_present now also checks for .so artifacts, this message should either reference both _portablemc.pyd and _portablemc.so, or be rephrased in a platform-neutral way (e.g., “native extension (_portablemc.*) is required”) to avoid confusing non-Windows users.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Create build scripts and package structure for portablemc 5.x wheel generation
Summary by Sourcery
Improve portablemc 5.x wheel handling and environment initialization guidance.
Enhancements: