-
Notifications
You must be signed in to change notification settings - Fork 933
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
chore: convert setup.py to build #2642
Conversation
12de833
to
8a652e7
Compare
8a652e7
to
d13e7c2
Compare
@@ -60,132 +108,78 @@ def download_driver(zip_name: str) -> None: | |||
|
|||
|
|||
class PlaywrightBDistWheelCommand(BDistWheelCommand): | |||
user_options = BDistWheelCommand.user_options + [ |
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.
Env var seems simpler, since option passing via build
is not fully working anyways: pypa/setuptools#2491. Changed it from --all
to explicitly passing the output wheel name.
print(bundle["wheel"]) | ||
exit(0) | ||
|
||
from setuptools import setup # noqa: E402 |
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.
The order here is non-common because we have the dependencies available only in non-build environments since I removed the deps from local-requirements.txt
.
@@ -683,7 +683,7 @@ def fulfill( | |||
json: typing.Optional[typing.Any] = None, | |||
path: typing.Optional[typing.Union[str, pathlib.Path]] = None, | |||
content_type: typing.Optional[str] = None, | |||
response: typing.Optional["APIResponse"] = None |
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'll extract them into a separate PR tomorrow.
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.
Extracted into #2646.
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.
Its hard to split the changes, since black
starts reading [project.requires-python]
from the pyproject.toml
. Lets land together.
|
||
wheels = base_wheel_bundles | ||
if not self.all: | ||
# Limit to 1, since for MacOS e.g. we have multiple wheels for the same platform and architecture and Conda expects 1. |
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.
is this comment still relevant? if so restore?
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.
Not anymore since we always output a single wheel now instead of multiple like before.
Similar to microsoft/playwright-pytest#254 - a little bit more complex here, because we previously were generating multiple wheels out of a single
setup.py
invocation. This is not supported with their new build infrastructure. To workaround this, I manage the loop from the outside which seems to work as well.This addresses #2124 (comment).