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

windows py38 and py39 #139

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
5 changes: 0 additions & 5 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,6 @@ jobs:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
python-version: [ 3.6, 3.7, 3.8, 3.9 ]
exclude:
- os: windows-latest
python-version: 3.8
- os: windows-latest
python-version: 3.9
env:
OS: ${{ matrix.os }}
PYTHON: '3.9'
Expand Down
4 changes: 2 additions & 2 deletions nbclient/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@ def _cleanup() -> None:
# the fix for python3.7: https://github.com/python/cpython/pull/15706/files
if sys.platform == 'win32':
if sys.version_info < (3, 7):
subprocess._cleanup = _cleanup
subprocess._active = None
subprocess._cleanup = _cleanup # type: ignore
subprocess._active = None # type:ignore
Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

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

Is it needed?

Copy link
Author

Choose a reason for hiding this comment

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

mypy will raise an error for py39 since these methods aren't implemented anymore.

7 changes: 7 additions & 0 deletions nbclient/client.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from os import name as os_name
from sys import version_info
import atexit
import collections
import datetime
Expand Down Expand Up @@ -32,6 +34,11 @@
from .output_widget import OutputWidget


if os_name == "nt" and version_info >= (3, 8):
# patch c.f. https://github.com/tornadoweb/tornado/issues/2608#issuecomment-491489432
asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) # type: ignore


def timestamp() -> str:
return datetime.datetime.utcnow().isoformat() + 'Z'

Expand Down
23 changes: 17 additions & 6 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
[tox]
skipsdist = true
envlist = py{36,37,38, 39}, flake8, mypy, dist, manifest, docs
envlist = py{36,37,38, 39}, flake8, mypy, dist-{win, linux}, manifest, docs
# requires =
# tox-factor
Comment on lines +4 to +5
Copy link
Member

Choose a reason for hiding this comment

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

Left-over?

Copy link
Author

Choose a reason for hiding this comment

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

Left-over?

Yes, this can go

# requires =
#     tox-factor


[gh-actions]
python =
3.6: py36
3.7: py37
3.8: py38
3.9: py39, flake8, mypy, dist, manifest
3.9: py39, flake8, mypy, dist-{win, linux}, manifest

# Linters
[testenv:flake8]
Expand Down Expand Up @@ -39,13 +41,22 @@ commands =
python -c 'import pathlib; print("documentation available under file://\{0\}".format(pathlib.Path(r"{toxworkdir}") / "docs_out" / "index.html"))'

# Distro
[testenv:dist]
[testenv:dist-{win, linux}]
skip_install = true
# Have to use /bin/bash or the `*` will cause that argument to get quoted by the tox command line...
platform =
win: win|msys
linux: linux|darwin
allowlist_externals =
win: powershell
linux: bash
Comment on lines +46 to +51
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why we need this?

Copy link
Author

Choose a reason for hiding this comment

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

The wildcard usage like nbclient*.whl is quite difficult to get into windows, even the gh windows runners have the bash installed. It's all about what is replaced within the quotes. So, what I did here was to distangle Windows and POSIX like systems. tox will check the platform via sys.platform and select the correct environment. Via that it also runs, e.g., on Windows 10 systems, where there is not a bash installed in general.

It might be a bit blown up to get this one whl file into pip install but I haven't found another way.


commands =
python setup.py sdist --dist-dir={distdir} bdist_wheel --dist-dir={distdir}
/bin/bash -c 'python -m pip install -U --force-reinstall {distdir}/nbclient*.whl'
/bin/bash -c 'python -m pip install -U --force-reinstall --no-deps {distdir}/nbclient*.tar.gz'
win: powershell Get-ChildItem -Path {distdir} -Filter nbclient*.whl | foreach \{python -m pip install -U --force-reinstall $_.FullName\}
win: powershell Get-ChildItem -Path {distdir} -Filter nbclient*.tar.gz | foreach \{python -m pip install -U --force-reinstall --no-deps $_.FullName\}
# Have to use /bin/bash or the `*` will cause that argument to get quoted by the tox command line...
linux: bash -c 'python -m pip install -U --force-reinstall {distdir}/nbclient*.whl'
linux: bash -c 'python -m pip install -U --force-reinstall --no-deps {distdir}/nbclient*.tar.gz'
Comment on lines +58 to +59
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to use /bin/bash anymore?

Copy link
Author

Choose a reason for hiding this comment

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

The bash should be in the path. Even cleaner would be to use just sh
linux: sh -c 'python -m pip install -U --force-reinstall {distdir}/nbclient*.whl' etc.


[testenv]
# disable Python's hash randomization for tests that stringify dicts, etc
Expand Down