GH-51487: [CI] Fix Emscripten wheel load failure by disabling llvm-strip on install - #51488
Conversation
|
|
|
@github-actions crossbow submit test-conda-python-emscripten |
|
Revision: f001b5c Submitted crossbow builds: ursacomputing/crossbow @ actions-8b6863eb0b
|
tadeja
changed the title
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Disables Emscripten wheel installation stripping to preserve LLVM’s required dylink.0 section.
Changes:
- Sets
SKBUILD_INSTALL_STRIP=falsebeforepyodide build.
| File | Description |
|---|---|
ci/scripts/python_build_emscripten.sh |
Disables stripping during Emscripten wheel creation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| pushd "${python_build_dir}" | ||
| # For LLVM 23 llvm-strip to not remove required dylink.0 | ||
| export SKBUILD_INSTALL_STRIP=false |
There was a problem hiding this comment.
I am wondering whether we want this happening on the rest of the wheels too.
Should we just try to add this section to the pyproject.toml:
[tool.scikit-build]
install.strip = falseWe manually strip debug symbols on wheels when building but I am unsure we want to strip all as discussed in the past. Details here:
@rok what do you think?
There was a problem hiding this comment.
For my understanding - what level of symbols do we want to distribute in wheels? Debug or less? Should we have debug wheels (that we perhaps only distribute through github and not pypi to save pypi quota).
How about distributing debug symbols separately? (see generated prototype here https://github.com/rok/arrow/pull/59/changes)
There was a problem hiding this comment.
I think we can merge this. This seems to have been the behaviour since scikit-build-core 0.5 and we've had several releases with it. No need to change current behavior globally.
github-actions
Bot
added
awaiting changes
| fi | ||
|
|
||
| pushd "${python_build_dir}" | ||
| # For LLVM 23 llvm-strip to not remove required dylink.0 |
There was a problem hiding this comment.
I think the comment should also refer to scikit-build-core striping symbols.
| # For LLVM 23 llvm-strip to not remove required dylink.0 | |
| # scikit-build-core strips unnecessary symbols by default. | |
| # Avoid LLVM 23 llvm-strip to strip the required dylink.0 |
There was a problem hiding this comment.
@raulcd I've added your suggestion and also updated my previous comment with llvm/llvm-project#180246. I hope that looks alright now.
github-actions
Bot
added
awaiting review
Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
github-actions
Bot
added
awaiting review
github-actions
Bot
added
awaiting changes
github-actions
Bot
added
awaiting change review
github-actions
Bot
added
awaiting changes
raulcd
left a comment
raulcd
left a comment
There was a problem hiding this comment.
I am merging as the changes since last approval where just about the comment.
Rationale for this change
Fix #51487 - starting with LLVM 23 the required dylink.0 is not kept during CMake install strip. (see llvm/llvm-project@52eb82e)
What changes are included in this PR?
Disable wheel install stripping with
SKBUILD_INSTALL_STRIP=false(applying alternative workaround llvm-strip --keep-section=dylink.0 would require more code changes)
Are these changes tested?
Verified by CI.
Are there any user-facing changes?
No.
Was AI used for this PR?
Just gpt-6-sol during initial analysis
PR code and description written by:
Reviewed before submission by: