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

GitHub Actions Windows: Invalid --only='""' (fixed in 2.16.5) #1740

Closed
jborean93 opened this issue Jan 29, 2024 · 6 comments · Fixed by #1741
Closed

GitHub Actions Windows: Invalid --only='""' (fixed in 2.16.5) #1740

jborean93 opened this issue Jan 29, 2024 · 6 comments · Fixed by #1741

Comments

@jborean93
Copy link
Contributor

jborean93 commented Jan 29, 2024

Description

I've getting random errors when attempting to build Windows wheels using the pypa/cibuildwheel@2.16.4 action.

Run pypa/cibuildwheel@v2.16.4
Run actions/setup-python@v5
Installed versions
Run pipx run --python "C:\hostedtoolcache\windows\Python\3.12.1\x64\python.exe" --spec "D:\a\_actions\pypa\cibuildwheel\v2.16.4" cibuildwheel "." --output-dir '"wheelhouse"' --config-file '""' --only '""'
  pipx run --python "C:\hostedtoolcache\windows\Python\3.12.1\x64\python.exe" --spec "D:\a\_actions\pypa\cibuildwheel\v2.16.4" cibuildwheel "." --output-dir '"wheelhouse"' --config-file '""' --only '""'
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    CIBW_ARCHS: all
    CIBW_TEST_SKIP: *_arm64
    CIBW_BUILD: cp38-win32
    CIBW_BUILD_VERBOSITY: 1
creating virtual environment...
determining package name from 'D:\\a\\_actions\\pypa\\cibuildwheel\\v2.16.4'...
creating virtual environment...
installing cibuildwheel from spec 'D:\\a\\_actions\\pypa\\cibuildwheel\\v2.16.4'...
Invalid --only='""', must be a build selector with a known platform
Error: Process completed with exit code 1.

On a build that works I can see the same command is being run with the --only='""' parameter.

Run pipx run --python "C:\hostedtoolcache\windows\Python\3.12.1\x64\python.exe" --spec "D:\a\_actions\pypa\cibuildwheel\v2.16.4" cibuildwheel "." --output-dir '"wheelhouse"' --config-file '""' --only '""'
  pipx run --python "C:\hostedtoolcache\windows\Python\3.12.1\x64\python.exe" --spec "D:\a\_actions\pypa\cibuildwheel\v2.16.4" cibuildwheel "." --output-dir '"wheelhouse"' --config-file '""' --only '""'
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    CIBW_ARCHS: all
    CIBW_TEST_SKIP: *_arm64
    CIBW_BUILD: cp38-win_amd64
    CIBW_BUILD_VERBOSITY: 1

The weird thing is that this only happens sometimes, other times the build works just fine. I know the 2.15.0 version is also affected. I only started seeing this in the last day. For example here is an older build that failed 2 times while on the 3rd it worked again https://github.com/pythongssapi/python-gssapi/actions/runs/7697806880

image

I'm unsure whether it's my code or something else but this has worked for me for the past year or so whereas now it seems to be popping up. At a guess it might be

Build log

https://github.com/pythongssapi/python-gssapi/actions/runs/7703392610/job/20993685914?pr=341

CI config

https://github.com/pythongssapi/python-gssapi/blob/18f3e2bc5119a3621c7bc14e307ab70ce011e34f/.github/workflows/ci.yml#L123-L129

@jborean93
Copy link
Contributor Author

jborean93 commented Jan 29, 2024

The upgrade to PowerShell 7.4.x actions/runner-images#9115 might be the cause here. The older version is 7.2.x and PowerShell 7.4 changed how arguments are escaped. In particularly how double quotes are treated in an argument value has changed. For example the first example was run with pwsh 7.2.18 while the second example with 7.4.1.

PS C:\Users\vagrant-domain\Downloads\PowerShell-7.2.18-win-x64> C:\temp\print_argv.exe --option '""'
"C:\temp\print_argv.exe" --option ""
[0] --option
[1]


PS C:\Users\vagrant-domain\Downloads\PowerShell-7.4.1-win-x64> C:\temp\print_argv.exe --option '""'
"C:\temp\print_argv.exe" --option "\"\""
[0] --option
[1] ""

There are three things you can do to go back to the old behaviour:

PS C:\Users\vagrant-domain\Downloads\PowerShell-7.4.1-win-x64> $PSNativeCommandArgumentPassing
Windows
PS C:\Users\vagrant-domain\Downloads\PowerShell-7.4.1-win-x64> C:\temp\print_argv.exe --option '""'
"C:\temp\print_argv.exe" --option "\"\""
[0] --option
[1] ""
PS C:\Users\vagrant-domain\Downloads\PowerShell-7.4.1-win-x64> C:\temp\print_argv.exe --option ''
"C:\temp\print_argv.exe" --option ""
[0] --option
[1]

PS C:\Users\vagrant-domain\Downloads\PowerShell-7.4.1-win-x64> $PSNativeCommandArgumentPassing = 'Legacy'
PS C:\Users\vagrant-domain\Downloads\PowerShell-7.4.1-win-x64> C:\temp\print_argv.exe --option '""'
"C:\temp\print_argv.exe" --option ""
[0] --option
[1]

Adding further fuel to that suggestion I can see the runner version of a working build is 20240122.1.0 and a failed build is 20240128.1.0.

  Image: windows-2022
  Version: 20240122.1.0
  Included Software: https://github.com/actions/runner-images/blob/win22/20240122.1/images/windows/Windows2022-Readme.md
  Image Release: https://github.com/actions/runner-images/releases/tag/win22%2F20240122.1
  Image: windows-2022
  Version: 20240128.1.0
  Included Software: https://github.com/actions/runner-images/blob/win22/20240128.1/images/windows/Windows2022-Readme.md
  Image Release: https://github.com/actions/runner-images/releases/tag/win22%2F20240128.1

@jborean93 jborean93 changed the title GitHub Actions Invalid --only='""', must be a build selector with a known platform GitHub Actions Windows Wheels - Invalid --only='""', must be a build selector with a known platform Jan 29, 2024
@henryiii
Copy link
Contributor

henryiii commented Jan 30, 2024

Does anyone remember/know why we double quote on powershell in the first place? I added it in #1346 but not sure it's needed, at least for non-paths. I think it might have been needed for paths with spaces?

@henryiii
Copy link
Contributor

The problem with --x "" --y is that it resolves to --x, --y, rather that --x, empty string, --y. Is there a way to pass an empty string in powershell 7.3/7.4?

@jborean93
Copy link
Contributor Author

jborean93 commented Jan 30, 2024

Is there a way to pass an empty string in powershell 7.3/7.4?

Yep see my 3 workarounds in #1740 (comment).

Essentially the new quoting rules will pass through the actual value that was bound in the pwsh binder and an empty string value will be quoted. You either need to change the code (and break 7.2 compatbility), revert back to the old argument quoting rules, or handle a double quote literal value in the Python argparse code. I went with option 2 in my PR because it is the simplest one available.

@joerick
Copy link
Contributor

joerick commented Jan 30, 2024

According to the docs, we could just use Python as a shell instead...? Though I can't remember the issue with Meson that's mentioned in the comment here.

@henryiii
Copy link
Contributor

You need cmd or powershell to get MSVC properly configured. If you run from other places, you get GCC instead. This affects other build systems too unless they look up MSVC via the registry.

penguinolog pushed a commit to penguinolog/urwid that referenced this issue Jan 31, 2024
penguinolog added a commit to urwid/urwid that referenced this issue Jan 31, 2024
Related pypa/cibuildwheel#1740

Co-authored-by: Aleksei Stepanov <alekseis@nvidia.com>
tttapa added a commit to kul-optec/alpaqa that referenced this issue Jan 31, 2024
syoyo added a commit to lighttransport/jdepp-python that referenced this issue Jan 31, 2024
syoyo added a commit to syoyo/tinyusdz that referenced this issue Jan 31, 2024
tttapa added a commit to kul-optec/QPALM that referenced this issue Jan 31, 2024
emmettbutler pushed a commit to DataDog/dd-trace-py that referenced this issue Jan 31, 2024
Bump to the latest cibuildwheel to fix Windows builds.

See more about the issue:
pypa/cibuildwheel#1740

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
github-actions bot pushed a commit to DataDog/dd-trace-py that referenced this issue Jan 31, 2024
Bump to the latest cibuildwheel to fix Windows builds.

See more about the issue:
pypa/cibuildwheel#1740

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit f2dc31f)
github-actions bot pushed a commit to DataDog/dd-trace-py that referenced this issue Jan 31, 2024
Bump to the latest cibuildwheel to fix Windows builds.

See more about the issue:
pypa/cibuildwheel#1740

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit f2dc31f)
github-actions bot pushed a commit to DataDog/dd-trace-py that referenced this issue Jan 31, 2024
Bump to the latest cibuildwheel to fix Windows builds.

See more about the issue:
pypa/cibuildwheel#1740

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.
- [x] If change touches code that signs or publishes builds or packages,
or handles credentials of any kind, I've requested a review from
`@DataDog/security-design-and-guidance`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit f2dc31f)
janosh added a commit to CederGroupHub/chgnet that referenced this issue Feb 8, 2024
janosh added a commit to CederGroupHub/chgnet that referenced this issue Feb 8, 2024
* update pre-commit hooks

* pre-commit run --all-files

* update GHA runner to macos-14 and pypa/cibuildwheel@v2.16.5

fixes release job, see pypa/cibuildwheel#1740
bauerdavid added a commit to bauerdavid/napari-nD-annotator that referenced this issue Feb 9, 2024
sgillies added a commit to rasterio/rasterio-wheels that referenced this issue Feb 14, 2024
TomWinder added a commit to QuakeMigrate/QuakeMigrate that referenced this issue Feb 17, 2024
hemmelig pushed a commit to QuakeMigrate/QuakeMigrate that referenced this issue Feb 18, 2024
jonasblixt added a commit to jonasblixt/punchboot that referenced this issue Feb 18, 2024
cibuildwheel had issues with newer versions of power shell. Full
details: pypa/cibuildwheel#1740
jonasblixt added a commit to jonasblixt/punchboot that referenced this issue Feb 18, 2024
cibuildwheel had issues with newer versions of power shell. Full
details: pypa/cibuildwheel#1740
jonasblixt added a commit to jonasblixt/punchboot that referenced this issue Feb 18, 2024
cibuildwheel had issues with newer versions of power shell. Full
details: pypa/cibuildwheel#1740
nachovizzo added a commit to PRBonn/kiss-icp that referenced this issue Feb 22, 2024
nachovizzo added a commit to PRBonn/kiss-icp that referenced this issue Feb 22, 2024
yetinam added a commit to yetinam/pyocto that referenced this issue Mar 8, 2024
N-Wouda added a commit to PyVRP/PyVRP that referenced this issue Mar 22, 2024
N-Wouda added a commit to PyVRP/PyVRP that referenced this issue Mar 22, 2024
* Run CD also on push to main

* Fix a small concurrency issue in workflow execution

* Fix CD on Windows by bumping to CIBW v2.16.5

See pypa/cibuildwheel#1740 for details

* Work around macOS's problem with std::optional::value, use macos-14

* Fix uploading and downloading artifacts

See actions/upload-artifact#478 (comment) for details

* Bump to v0.8.1
@henryiii henryiii unpinned this issue Apr 24, 2024
HinTak added a commit to pavpanchekha/skia-python that referenced this issue Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants