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

feat: add --only #1098

Merged
merged 13 commits into from Sep 9, 2022
Merged

feat: add --only #1098

merged 13 commits into from Sep 9, 2022

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Apr 28, 2022

Closes #1087 by adding a --only arg. Closes #1262. By providing an alternative that only uses the wheel identifier.

@joerick
Copy link
Contributor

joerick commented May 9, 2022

Hmm. I'm thinking more about this and I wonder if perhaps --build should override both CIBW_BUILD and CIBW_SKIP from environment/config. There seems something weird about the user doing explicitly cibuildwheel --build 'pp38-manylinux_x86_64' but that selects zero identifiers because there's a CIBW_SKIP=pp* in the environment. Perhaps --build should override both build & skip from environment and config. What do you think @henryiii ?

@joerick
Copy link
Contributor

joerick commented May 9, 2022

Ah, in fact the issue might be deeper than that, for development machine use. This comment raises another potential confusion, which is that cibuildwheel --build 'pp38-manylinux_x86_64' wouldn't even work at all on Windows/Mac, because we require the --platform flag too, which feels redundant - cibuildwheel --build 'pp38-manylinux_x86_64' --platform linux.

That makes me think that maybe we should have a flag that's more designed around the 'testing locally' use-case... and that it should override all the build selector arguments --platform, --archs, CIBW_BUILD and CIBW_SKIP. That way, you can just grab an identifier from your build logs or the docs and run a build. e.g.

--build-one IDENTIFIER
    Just run one build, as specified by IDENTIFIER. Intended to be used for
    local testing - all other build selection options and arguments are ignored
    if this flag is set.

Names could be --build-one, --build-only, or just --build...

@henryiii
Copy link
Contributor Author

henryiii commented May 10, 2022

--target=<...> or --local=<...> or --only=<...> or --identifier might be options? Given it is merging several things, not just BUILD, and selector expressions are not valid. Personally I'd often do CIBW_BUILD=cp310-* since I'm lazy and don't want to look up the spelling of the rest of the os/arch tag, but being able to skip --platform (and maybe --arch) would likely make it more worth while.

@joerick
Copy link
Contributor

joerick commented May 11, 2022

--only makes sense to me!

@henryiii henryiii marked this pull request as draft May 16, 2022 21:28
@henryiii henryiii changed the title feat: add --build feat: add --only Aug 22, 2022
@henryiii henryiii force-pushed the henryiii/feat/buildarg branch 3 times, most recently from 6949cbd to 111a37a Compare August 23, 2022 01:08
@henryiii henryiii marked this pull request as ready for review August 24, 2022 14:05
Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

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

Overall structure looks good!

cibuildwheel/options.py Show resolved Hide resolved
docs/options.md Outdated Show resolved Hide resolved
docs/options.md Show resolved Hide resolved
@pytest.mark.parametrize("only", ("cp311-manylxinux_x86_64", "some_linux_thing"))
def test_only_failed(intercepted_build_args, monkeypatch, only):
monkeypatch.setattr(sys, "argv", sys.argv + ["--only", only])
monkeypatch.delenv("CIBW_PLATFORM")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why these delenvs are needed? Also on line 244, and 254.

Copy link
Contributor Author

@henryiii henryiii Aug 31, 2022

Choose a reason for hiding this comment

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

Because intercepted_build_args uses platform which injects it. The point of this is to see if adding something like --platform or --archs fails or some broken --only is used, but instead it will fail because CIBW_PLATFORM is set & --only is present, and passes the test for the wrong reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've removed the coupling of intercepted_build_args from platform - it now monkeypatches all three. I'll push that to this PR so you can have a look.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@joerick
Copy link
Contributor

joerick commented Sep 4, 2022

I've just pushed a few commits. After removing the intercepted_build_args to platform coupling, I realised that CIBW_PLATFORM was causing an error when used with the --only command line option, which doesn't seem right to me (command line options should override env vars). So I wrote a test for that and got it passing.

Let me know if that works for you!

edit: just looking at these CI failures.... done

@joerick
Copy link
Contributor

joerick commented Sep 4, 2022

The CI failures are the

WARNING: Skipping page https://pypi.org/simple/xxx/ because the GET request got Content-Type: Unknown
ones.

Having seen more of this recently I investigated a little #1254

@henryiii
Copy link
Contributor Author

henryiii commented Sep 6, 2022

That's fine with me - IMO it should be really rare to have CIBW_PLATFORM & --only together, and it's probably a mistake on the user's part if they are both there, so I'd be fine with the error, but don't have a strong feeling on it.

cibuildwheel/__main__.py Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor Author

henryiii commented Sep 9, 2022

Added only: to the Action, since I think it would replace #1262. --only "" is now equivalent to not passing only, since that makes it easier to template, and is how other options in cibw tend to work.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii merged commit eb39da0 into pypa:main Sep 9, 2022
@henryiii henryiii deleted the henryiii/feat/buildarg branch September 9, 2022 12:34
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.

How do I build one specific wheel from the command line?
2 participants