Skip to content

Push some local cleanups #3276

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.

Sign up for GitHub

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

Merged
A5rocks merged 1 commit into from
Jun 1, 2025
Merged

Push some local cleanups #3276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,13 +182,11 @@ jobs:
# lsp_extract_file: ''
# extra_name: ', with non-IFS LSP'

# ***REMEMBER*** to remove the 3.14 line once windows+cffi works again
continue-on-error: >-
${{
(
endsWith(matrix.python, '-dev')
|| endsWith(matrix.python, '-nightly')
|| matrix.python == '3.14'
)
&& true
|| false
Expand All @@ -202,18 +200,11 @@ jobs:
- name: Setup python
uses: actions/setup-python@v5
with:
# This allows the matrix to specify just the major.minor version while still
# expanding it to get the latest patch version including alpha releases.
# This avoids the need to update for each new alpha, beta, release candidate,
# and then finally an actual release version. actions/setup-python doesn't
# support this for PyPy presently so we get no help there.
#
# 'CPython' -> '3.9.0-alpha - 3.9.X'
# 'PyPy' -> 'pypy-3.9'
python-version: ${{ fromJSON(format('["{0}", "{1}"]', format('{0}.0-alpha - {0}.X', matrix.python), matrix.python))[startsWith(matrix.python, 'pypy')] }}
python-version: '${{ matrix.python }}'
Copy link
Member

@jakkdl jakkdl May 30, 2025

Choose a reason for hiding this comment

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

was this the logic that allowed 3.14-dev? (and the comment being outdated/incorrect) If so, and we're getting rid of it, then we should simplify continue-on-error.

Copy link
Contributor Author

@A5rocks A5rocks May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The new logic allows both 3.14-dev and 3.14, which will both point to the latest beta. The only difference is that one indicates that Trio doesn't support it completely yet so is allowed to fail.

Copy link
Contributor Author

@A5rocks A5rocks May 30, 2025

Choose a reason for hiding this comment

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

To be clear, the comment you posted above was not possible (I didn't realized it at the time) due to the old logic here. The options are:

  • remove the old logic, keep the continue-on-error
  • keep the old logic, remove the continue-on-error

I chose the first because I think ideally we would keep e.g. 3.15-dev until 3.15 releases a beta (so failure doesn't matter), then change it to 3.15 along with any necessary changes to ensure that Trio supports 3.15.

architecture: '${{ matrix.arch }}'
cache: pip
cache-dependency-path: test-requirements.txt
allow-prereleases: true
- name: Run tests
run: ./ci.sh
shell: bash
Expand Down Expand Up @@ -276,9 +267,10 @@ jobs:
- name: Setup python
uses: actions/setup-python@v5
with:
python-version: ${{ fromJSON(format('["{0}", "{1}"]', format('{0}.0-alpha - {0}.X', matrix.python), matrix.python))[startsWith(matrix.python, 'pypy')] }}
python-version: '${{ matrix.python }}'
cache: pip
cache-dependency-path: test-requirements.txt
allow-prereleases: true
- name: Check Formatting
if: matrix.check_formatting == '1'
run:
Expand Down Expand Up @@ -328,9 +320,10 @@ jobs:
- name: Setup python
uses: actions/setup-python@v5
with:
python-version: ${{ fromJSON(format('["{0}", "{1}"]', format('{0}.0-alpha - {0}.X', matrix.python), matrix.python))[startsWith(matrix.python, 'pypy')] }}
python-version: '${{ matrix.python }}'
cache: pip
cache-dependency-path: test-requirements.txt
allow-prereleases: true
- name: Run tests
run: ./ci.sh
- if: always()
Expand All @@ -356,19 +349,29 @@ jobs:
# `nodejs` for pyright (`node-env` pulls in nodejs but that takes a while and can time out the test).
# `perl` for a platform independent `sed -i` alternative
run: apk update && apk add python3-dev bash nodejs perl

- name: Retrieve the project source from an sdist inside the GHA artifact
# must be after `apk add` because it relies on `bash` existing
uses: re-actors/checkout-python-sdist@release/v2
with:
source-tarball-name: ${{ needs.build.outputs.sdist-artifact-name }}
workflow-artifact-name: ${{ env.dists-artifact-name }}

- name: Enter virtual environment
run: python -m venv .venv

- name: Run tests
run: source .venv/bin/activate && ./ci.sh

- name: Get Python version for codecov flag
id: get-version
run: echo "version=$(python -V | cut -d' ' -f2 | cut -d'.' -f1,2)" >> "${GITHUB_OUTPUT}"
shell: python
run: |
import sys, os
with open(os.environ["GITHUB_OUTPUT"], "a") as f:
f.write("version=" + ".".join(map(str, sys.version_info[:2])))
f.write("\n")

- if: always()
uses: codecov/codecov-action@v3
with:
Expand Down Expand Up @@ -425,11 +428,14 @@ jobs:
- name: import & run module
run: coverage run -m tests.cython.run_test_cython

- name: get Python version for codecov flag
- name: Get Python version for codecov flag
id: get-version
run: >-
echo "version=$(python -V | cut -d' ' -f2 | cut -d'.' -f1,2)"
>> "${GITHUB_OUTPUT}"
shell: python
run: |
import sys, os
with open(os.environ["GITHUB_OUTPUT"], "a") as f:
f.write("version=" + ".".join(map(str, sys.version_info[:2])))
f.write("\n")

- run: |
coverage combine
Expand Down
7 changes: 3 additions & 4 deletions src/trio/_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,8 +736,7 @@ async def read_output(

# Opening the process does not need to be inside the nursery, so we put it outside
# so any exceptions get directly seen by users.
# options needs a complex TypedDict. The overload error only occurs on Unix.
proc = await open_process(command, **options) # type: ignore[arg-type, call-overload, unused-ignore]
proc = await _open_process(command, **options) # type: ignore[arg-type]
async with trio.open_nursery() as nursery:
try:
if input_ is not None:
Expand Down Expand Up @@ -1164,7 +1163,7 @@ async def open_process(
async def run_process(
command: StrOrBytesPath,
*,
stdin: bytes | bytearray | memoryview | int | HasFileno | None = None,
stdin: bytes | bytearray | memoryview | int | HasFileno | None = b"",
shell: Literal[True],
**kwargs: Unpack[UnixRunProcessArgs],
) -> subprocess.CompletedProcess[bytes]: ...
Expand All @@ -1173,7 +1172,7 @@ async def run_process(
async def run_process(
command: Sequence[StrOrBytesPath],
*,
stdin: bytes | bytearray | memoryview | int | HasFileno | None = None,
stdin: bytes | bytearray | memoryview | int | HasFileno | None = b"",
shell: bool = False,
**kwargs: Unpack[UnixRunProcessArgs],
) -> subprocess.CompletedProcess[bytes]: ...
Expand Down
2 changes: 1 addition & 1 deletion src/trio/_tests/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,7 +733,7 @@ async def test_run_process_internal_error(monkeypatch: pytest.MonkeyPatch) -> No
async def very_broken_open(*args: object, **kwargs: object) -> str:
return "oops"

monkeypatch.setattr(trio._subprocess, "open_process", very_broken_open)
monkeypatch.setattr(trio._subprocess, "_open_process", very_broken_open)
with RaisesGroup(AttributeError, AttributeError):
await run_process(EXIT_TRUE, capture_stdout=True)

Expand Down
Loading