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
Open

windows py38 and py39 #139

wants to merge 16 commits into from

Conversation

juhuebner
Copy link

@juhuebner juhuebner commented Mar 7, 2021

fixes #85, fixes #125

forgot to squash my dev branch ;)

poss.

nbclient/client.py Outdated Show resolved Hide resolved
Comment on lines +4 to +5
# requires =
# tox-factor
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

Comment on lines +16 to +17
subprocess._cleanup = _cleanup # type: ignore
subprocess._active = None # type:ignore
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.

Comment on lines +46 to +51
platform =
win: win|msys
linux: linux|darwin
allowlist_externals =
win: powershell
linux: bash
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.

Comment on lines +58 to +59
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'
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.

@davidbrochart
Copy link
Member

Thanks a lot @juhuebner, I made some comments mainly for me to understand better your changes.

juhuebner and others added 2 commits March 8, 2021 17:23
python version check >= (3, 8)

Co-authored-by: David Brochart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants