-
Notifications
You must be signed in to change notification settings - Fork 158
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
dist_info: implement command #190
Conversation
Codecov Report
@@ Coverage Diff @@
## master #190 +/- ##
=========================================
- Coverage 69.86% 69.4% -0.46%
=========================================
Files 18 19 +1
Lines 1812 1824 +12
=========================================
Hits 1266 1266
- Misses 546 558 +12
Continue to review full report at Codecov.
|
I added a new command called "dist_info', which is exactly like egg_info but generates a dist_info directory.
@agronholm Can you merge this and then release? |
I cannot make any releases before the ReadTheDocs configuration has been adjusted to point to the Github repository. I've asked @dholth for maintainer access there twice now but have not received a response. Also, I am not happy about the fact that your new command has no tests whatsoever attached. |
I'm also wondering whether wheel should keep relying on setuptools, given the PEP 517 effort. But that is a topic for another time. |
I actually added a test but then removed it because of some json validator test failures that I don't understand. |
That is why this command is needed in fact. Setuptools needs to generate a dist-info to comply with the specification but currently cannot. |
Why is |
Shouldn't the setuptools code for egg_info not be copied here then, instead of importing setuptools directly? |
I don't know why it's failing, but the test suite passes on master. So this PR must've done something to cause them to fail. Would you mind figuring that out? I'm going to sleep in an hour or two. |
The PR passed on master 3 hours ago. Let me try to re-run master. |
I picked this up from the logs: |
Will do. |
That makes sense. |
If you run |
Running |
What could possibly cause the validation failure? The command did not touch any existing tests or functionality. |
It's probably a tox failure because it only happened when I ran tox. |
Let me downgrade tox and see. |
I'm checking out now – let me know what you find. I'll get back to this tomorrow. |
Downgrading tox failed to resolve the problem, and tox was released September 1; the problem happened sometime between now and 6 hours ago. The question is: what changed in the last six hours? |
I am also seeing the errors locally now that I upgraded tox. |
Maybe it is tox then. Does EDIT: botched tox version. |
Downgrading to 2.6.0 and erasing the .tox directory did not help. The same failure still happens. The only two libs it upgraded at the same time were pluggy and py. |
I noticed though that a new version of setuptools was just released. Could that have something to do with it? |
Does tox download requirements? |
I'm not really sure how it works exactly :) But I have come across situations where it does download things. |
I'm guessing pypa/setuptools#1075? |
Which PEP governs the allowed fields in metadata? |
Well, I don't see this PEP having been amended to include that field. |
Also, PEP 345 |
Ditto for 345 and 426 (which replaces it). |
You're correct. However, there were a lot of requests for markdown support and I can attest that submitting a PEP is a nightmare. So it appears that @msabramo decided to implement markdown support unilaterally. I cannot say I disagree with this though because markdown support is far more valuable than squabbling over a PEP. |
I too am +1 on markdown support, but the latest PEP is a draft anyway. Could that not have been amended too? How else are we going to keep track of the fields going forward? |
Which PEP is that? |
@ericholscher @agjohnson @humitos Would you mind tranferring ownership of wheel to @agronholm. |
The wheel project is fundamentally a setuptools plugin. I assume you realize all setuptools commands are plugins. However there are some things in there, like metadata converters, that are more generally useful.An older revision of the pep 426? described the key/value metadata. Later it was changed to the draft json format. Wheel should stop including the draft metadata.On Sep 3, 2017 17:08, xoviat <notifications@github.com> wrote:
I'm also wondering whether wheel should keep relying on setuptools, given the PEP 517 effort. But that is a topic for another time.
That is why this command is needed in fact. Setuptools needs to generate a dist-info to comply with the specification but currently cannot.
—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
|
@dholth Would you please transfer read the docs ownership? |
While wheel may have been a setuptools plugin, I don't think it should remain as such, going forward. But decisions like this were the primary motivation for arranging the Hangouts meeting. |
@xoviat looks like Daniel has given me maintainer access to the wheel docs on RTD now, so that is no longer a blocker. Now I would like to properly understand how the dist_info command fits to the picture now. If you can point me to tickets or mailing list messages or other material, that would be appreciated. |
|
You can also see the proposed setuptools build backend at pypa/setuptools#1039. Basically it maps command line calls onto the new functions, but there is currently no way to generate a |
Ok, thanks for the info. We're holding a closed circle meeting tomorrow, during which wheel's role in all of this should become much clearer. I will have to hold off on merging this until at least then. |
Just be aware that depending on the outcome of the discussions, I may require even heavy refactoring of the code. Thankfully there's not much of it, so it shouldn't be a big issue :) |
If it is not a setuptools plugin, where does "setup.py bdist_wheel" come from, and do you propose wheel provide only dist-info as a setuptools plugin?On Sep 4, 2017 03:28, Alex Grönholm <notifications@github.com> wrote:While wheel may have been a setuptools plugin, I don't think it should remain as such, going forward. But decisions like this were the primary motivation for arranging the Hangouts meeting.
—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
|
My vision is this: "wheel" becomes independent of setuptools, and setuptools adds a bdist_wheel command of its own, which relies on the wheel library. This way, other build backends do not have to rely on the existence of setuptools. |
Most wheel generators ignore the wheel code, proving that it's a real standard and not just an implementation, although it would be nice to have a library. The manifest is probably the trickiest. Enscons uses wheel code to do the requirements.It's no accident that wheel does not list setuptools in its install_requires.I think it is probably still useful to be able to adjust bdist_wheel independent of the setuptools release.On Sep 4, 2017 10:00, Alex Grönholm <notifications@github.com> wrote:My vision is this: "wheel" becomes independent of setuptools, and setuptools adds a bdist_wheel command of its own, which relies on the wheel library. This way, other build backends do not have to rely on the existence of setuptools.
—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.
|
If that is the consensus in tomorrow's meeting, I will respect it. |
@agronholm What was the consensus of the meeting? |
It was rescheduled for today. I'll let you know afterwards. |
So, the consensus was that this command is a better fit for setuptools rather than wheel. I will further discuss this with @jaraco. |
So @xoviat, would you mind making a PR against setuptools? Jason also agreed that it should probably go to setuptools rather than wheel. |
I don't see that the consistency in that but will do.
…On Sep 6, 2017 1:44 PM, "Alex Grönholm" ***@***.***> wrote:
So @xoviat <https://github.com/xoviat>, would you mind making a PR
against setuptools? Jason also agreed that it should probably go to
setuptools rather than wheel.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#190 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AOo8_OwBEM7vIYVJnsvSi3URN2ljArRDks5sfuf0gaJpZM4PLQQg>
.
|
I'm sorry about the whitespace changes, but working on pip requires flake8; it's frustrating to turn it off and then on again.