-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Permit --no-use-pep517 with editable mode in more cases. #6442
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me, looks good, let's get this shipped.
0a3ff31
to
f069769
Compare
I tweaked the news entry, FYI. |
Works for me 👍 Any chance for a release today so I can apply a fix in tox today too? |
That would be up to @dstufft as he'd most likely be the one to cut the release (assuming he can do it). Also, there are at least a couple other post-release fixes that should go out IMO, too, so there's a question of whether to put those in the same release or not. |
No time to do more than a quick review, but this LGTM, and I specifically like the approach you've taken. +1 on merging. I guess it's then up to @dstufft (as the person handling the 19.1 release) to look at getting it released (although if he's unavailable, someone else could take that on). |
Thanks, @pfmoore. I need to go off-line, but someone else is welcome to click the merge button. @gaborbernat Perhaps @jaraco can offer the $50 to @dstufft instead as an enticement. :) |
I appreciate the work on this. Although I understand why you settled on this approach and that it’s superior to the status quo, it still will require updating every affected project to support pip 19.1.1, which was the busy work I was hoping to avoid when offering the bounty. Edit: @dstufft, the bounty is yours if this can release today... let me know if you want to split it with Chris. |
all tox driven projects will not need update (as tox will auto enable the feature). |
* Use --no-use-pep517 flag for editable mode during installation and update docs to reflect this - pypa/pip#6434 - pypa/pip#6442 * Add .eggs to Black exclude list
@cjerdonek @pfmoore I've been AWOL, so sorry if it feels like I'm barging out of place - is there any specific reason we're requiring packages using a setuptools backend to have to specify --no-pep-517 for editable installs? We already didn't support other use cases for editable anyway and the PEP doesn't explicitly disallow use of editable install mechanisms, since it's outside of the scope of the PEP. |
@pradyunsg This is where @pfmoore first provided the rationale when I asked him about this case: #6331 (comment) |
My reasoning is that what we now have does the job, while remaining with the spirit of the PEP 517 changes - we take the use of the PEP 517 We could have taken Beyond that, it was mostly about finding something that we could implement quickly, but without leaving us with maintenance issues down the line that we didn't understand the consequences of. What I mostly want to come from this is for people to look at actually adding editable mode to PEP 517. If that doesn't happen, then I'll be very disappointed, because this issue has demonstrated that there's a significant need for that work. 1 The only real reason PEP 517 doesn't support editable installs was because the participants at the time were basically burned out working on what we did get. No-one thought they were insignificant, or that a PEP without them was the final answer. But we had to stop somewhere, and start implementing the design, or we'd never have any real world feedback to validate what we did have. |
Basically, my main concern with this approach is that we're going to have some legacy overhead for editable installs until that void is filled within our standards, but disrupting user workflows as a part of the transition would negatively affect how folks perceive pip and PyPA broadly.
OTOH, requiring users to specify |
FWIW, I agree that adding more options is not a good idea. With |
I'm happy to duck out at this point and leave the decision to others. If the consensus is to revert @cjerdonek's change in favour of an alternative approach that's fine with me. There was a lot of pressure yesterday for a quick solution, and if as a result I let myself be pushed into arguing for the wrong approach, then I apologise. |
I think it would be better to discuss this on the Discourse category instead of on this closed issue. |
I'm in phase with @pradyunsg analysis. |
@pfmoore You don't need to apologize for anything. I hope I didn't come across wrong as I don't mean to undermine the work that you and Chris have done for this release cycle. Thank you for doing the post-release firefighting and sorry for not being there. In fact, you've stated your reasoning and position very clearly and that has definitely been very helpful. |
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. |
Fixes #6434.
The two new messages this PR introduces are as follows:
InstallationError
:And for comparison, I'm including one of the messages that already exists from before and remains the same.
InstallationError
: