Remove unused OpnSSL installs from the wheel builds - #973
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Windows wheel build no longer installs OpenSSL through Chocolatey. The Linux cibuildwheel Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change removes unused OpenSSL installation work from wheel builds without an identified impact on product behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description identifies both configuration changes and follows the repository template. It does not state why the OpenSSL installations are unused, and the corresponding checklist item remains unchecked. Resolution Add the reason for removing the installations, such as Windows builds not using system OpenSSL and Linux manylinux images already providing the required OpenSSL support. Mark the PR-summary checklist item as checked after updating the description.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
89a6067 to
d3f04e7
Compare
Neither install was ever used by anything we build. Windows: nothing in the job ever used it. Nothing there compiles against OpenSSL, so no headers were needed. And everything that does TLS at runtime carries its own copy - Python brings its own libssl/libcrypto, and the cryptography package has OpenSSL built in - so nothing ever looks for a system-wide install. The step just put a copy on the runner that sat unused. Linux: the same, in two halves. `openssl` was already in the manylinux image, so asking for it changed nothing. `openssl-devel` (the headers) did install something new, but nothing ever compiled against it - every test dependency arrives as a ready-made wheel, and the only one that needs OpenSSL, cryptography, has it built into that wheel.
d3f04e7 to
f4b2f80
Compare
patrycja-ziemkiewicz
changed the title
There was a problem hiding this comment.
Pull request overview
Removes unused OpenSSL installation steps from wheel builds.
Changes:
- Removes OpenSSL packages from Linux wheel setup.
- Removes the Windows OpenSSL installation step.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pyproject.toml |
Removes unused Linux OpenSSL packages. |
.github/workflows/lib-build.yml |
Removes unused Windows OpenSSL installation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Changes
Removes the Windows
Install OpenSSL for Windowsstep andopenssl openssl-develfrom the Linuxbefore-buildinpyproject.toml.Pre-review checklist
./docs/source/.Fixes:annotations to PR description.