Skip to content
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

Don't require --platform when running on a development machine #1727

Merged
merged 2 commits into from Feb 28, 2024

Conversation

joerick
Copy link
Contributor

@joerick joerick commented Jan 21, 2024

Historically, we required --platform when running cibuildwheel outside of CI, because cibuildwheel would attempt to globally install Python interpreters and some packages.

That was fixed in #974 a couple of years ago. As a result, there's no reason to require --platform these days, the auto behaviour is perfectly fine for dev machines.

It also makes running tests locally simpler, one can just do pytest test/test_0_basic.py (or nox -s tests -- test/test_0_basic.py) and not have to worry about setting CIBW_PLATFORM most of the time.

Historically, we required `--platform` when running cibuildwheel outside of CI, because cibuildwheel would attempt to globally install Python interpreters and some packages.

That was fixed in #974 a couple of years ago. As a result, there's no reason to require --platform these days, the `auto` behaviour is perfectly fine for dev machines.

It also makes running tests locally simpler, one can just do `pytest test/test_0_basic.py` (or `nox -s tests -- test/test_0_basic.py`) and not have to worry about setting CIBW_PLATFORM most of the time.
cibuildwheel/__main__.py Outdated Show resolved Hide resolved
options = intercepted_build_args.args[0]

# check that the platform was auto detected to build for the current system
if sys.platform.startswith("linux"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this feels like too much logic in a test. Any chance to simplify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what you mean, it's essentially just repeating the logic in the main program. But I would like to be sure that both auto and unspecified are doing the right thing. A little extra verbosity in the test seems like a small price to pay IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, why not try solving this on the level of parametrization? It'd be even clearer if it shows up in the test report.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even not parametrization but simeple dict could solve this:

platform_dkt = {"linux": "linux", "darwin": "macos", "win32": "windows"}

assert sys.platform in platform_dkt, f"Unknown platform: {sys.platform}"
assert platform_dkt[sys.platform] == options.platform

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I appreciate the focus on craft and code quality, there becomes a point when it's not worth our time to debate such minor points. Especially in tests, where the control flow complexity is so low, because each test is an isolated execution. All that matters in a test is the actions that are taken and the asserts that are reached - if these are clear to the reader, it's fine. If they grow to become unmaintainable, it's easy to rewrite them.

@joerick joerick merged commit 554726e into main Feb 28, 2024
12 of 18 checks passed
@joerick joerick deleted the dont-require-platform branch February 28, 2024 09:20
imphil added a commit to imphil/cocotb that referenced this pull request Mar 13, 2024
cibuildwheel 2.17 doesn't need the "--platform" flag any more for
local builds, so get rid of that for simpler code
(pypa/cibuildwheel#1727).
imphil added a commit to imphil/cocotb that referenced this pull request Mar 17, 2024
cibuildwheel 2.17 doesn't need the "--platform" flag any more for
local builds, so get rid of that for simpler code
(pypa/cibuildwheel#1727).
imphil added a commit to imphil/cocotb that referenced this pull request Mar 20, 2024
cibuildwheel 2.17 doesn't need the "--platform" flag any more for
local builds, so get rid of that for simpler code
(pypa/cibuildwheel#1727).
imphil added a commit to cocotb/cocotb that referenced this pull request Mar 20, 2024
cibuildwheel 2.17 doesn't need the "--platform" flag any more for
local builds, so get rid of that for simpler code
(pypa/cibuildwheel#1727).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants