fix PEP 518 support #5190
fix PEP 518 support #5190
Conversation
pfmoore
left a comment
pfmoore
left a comment
There was a problem hiding this comment.
Relatively minor points here. The bulk of the changes look fine to me.
src/pip/_internal/build_env.py
Outdated
There was a problem hiding this comment.
It's os.pathsep, not os.path.sep.
src/pip/_internal/build_env.py
Outdated
There was a problem hiding this comment.
I assume the changes to this block are related to PyPy support? Having a comment here explaining what's going on would be really useful (from my tests on Windows, get_python_lib returns absolute paths, so what's up with appending platlib to purelib?)
(I know this is basically code that was already present, so I'm happy if the answer is that you don't know enough of the detail to write a reasonable comment).
There was a problem hiding this comment.
I'll add a comment. As for appending, see comment above.
There was a problem hiding this comment.
Realising it's os.pathsep, the comment's probably not needed...
There was a problem hiding this comment.
nit: plural "requirements" looks odd if there's only one missing requirement. Maybe reword? Something like "Missing buildtime requirements in pyproject.toml for %s: %s"?
* fix build environment handling when using PyPy * do use the build environment for all build commands * allow installing and building a wheel of a PEP 518 enabled package without prior installation of setuptools and/or wheels * fix check for minimum supported requirements for PEP 518 support: - correctly handle complex requirements - both setuptools and wheels are needed
benoit-pierre
force-pushed
the
fix_pep518_support
branch
from
52a233e to
6da1d9e
Compare
|
Amended. |
|
It would be great if someone with permissions on AppVeyor could enable rolling builds. My previous build was still running... |
|
Correction, it's still running... |
|
Agreed, and I've done that. @pypa/pip-committers If anyone has any concerns about having rolling builds on, let me know (but it seems like a great idea to me). |
|
@pradyunsg If you have time to take a look at this, I'd appreciate a second review (but if you're busy, I'm OK with merging this myself). |
pradyunsg
left a comment
pradyunsg
left a comment
There was a problem hiding this comment.
No time, sorry but the code looks good on skimming using the Github Web UI.
|
No problem @pradyunsg and thanks for the fixes @benoit-pierre! |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
lock
bot
added
the
auto-locked
3 participants
Fix #5188.