-
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
Copy the correct file during PEP 517's build process #6236
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.
Some comments.
3a5f398
to
18d230a
Compare
18d230a
to
e013972
Compare
Okay. It seems like setuptool's current backend code makes the same assumption as pip's existing code -- that only one file exists in the dist/ directory once things are built. Not sure what's wrong here anymore. Commenting out the addition of a wheel in the dist/ directory for now due to that. This seems good to go to me. /cc @cjerdonek |
add_files_to_dist_directory(pkg_to_wheel) | ||
|
||
result = script.pip('wheel', pkg_to_wheel, '-w', script.scratch_path) | ||
assert "Installing build dependencies" not in result.stdout, result.stdout |
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.
I would assert something positive here or else make "Installing build dependencies"
a variable that's used in both this test and the previous. Otherwise, if the message ever changes, the test can continue to pass but for the wrong reason.
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.
That's the same pattern as existing tests; we can fix this in a follow up.
logger.info('Stored in directory: %s', output_dir) | ||
return wheel_path | ||
return dest_path | ||
except Exception: | ||
pass |
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.
Note for future: should this be swallowing an exception without logging anything?
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.
No clue. :)
@cjerdonek If you're okay with giving the green light to this, let's merge? :) (the CI failures are due to a PyPI search outage; I pinged folks for that) |
* Return paths from legacy/PEP 517 build handlers * Simplify the wheel moving logic * Sort the result of os.listdir, to increase determinism
setuptools expects only one wheel file to be present in dist/
Let's run the CI again. |
91dfb23
to
51086b3
Compare
Going ahead and merging this; we can address any additional issues in a follow up PR. |
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 #6196