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

Project updates for python 3.12 compatibility #194

Merged
merged 5 commits into from
Nov 23, 2023

Conversation

JacobCallahan
Copy link

@JacobCallahan JacobCallahan commented Oct 3, 2023

SafeConfigParser has been deprecated since Python 3.2 and was removed
in Python 3.12 entirely.
Per the release notes [1] it is recommended to use the ConfigParser
class now.

Additionally, the readfp method has been removed in favor of read_file.

Unbind the upper limit of Cython dependency and rebuild with Cython 3.0.5.

Update the vendored version of libssh2.

[1] - https://docs.python.org/3.12/whatsnew/3.12.html#configparser

@JacobCallahan
Copy link
Author

This should resolve the first (and hopefully last) error we're seeing for #193

SafeConfigParser has been deprecated since Python 3.2 and was removed
in Python 3.12 entirely.
Per the release notes [1] it is recommended to use the ConfigParser
class now.

Additionally, the readfp method has been removed in favor of read_file.

[1] - https://docs.python.org/3.12/whatsnew/3.12.html#configparser
@JacobCallahan
Copy link
Author

Also, would it be possible to add python 3.11 and 3.12 to the circleci runs?

@JacobCallahan JacobCallahan changed the title Switch from SafeConfigParser to ConfigParser class Updates for ConfigParser in python 3.12 Oct 3, 2023
@JacobCallahan
Copy link
Author

CI failure related to pypa/setuptools#2497
It looks like we'll need to supply a conformant version to build correctly.

@ogajduse
Copy link

ogajduse commented Oct 9, 2023

@pkittenis Are you still around? Could we get your attention on this PR?
ssh2-python users are not able to use the library with the newest Python unless a patch is made.

@JacobCallahan
Copy link
Author

One option here for moving away from versioneer is what @kdart-brt did in his fork.
pycopia/ssh2-python3@ea47675

@pbrezina
Copy link

pbrezina commented Nov 3, 2023

@enkore @pkittenis May I kindly ask you to review and merge this pull request?

@pbrezina
Copy link

pbrezina commented Nov 7, 2023

@enkore @pkittenis If the project is dead, I would like to talk to you about giving me access to the organization. I don't have time to fully maintain it, but I can keep it functional and I would try to find some contributors. This is a great project, it would be a pity if we can't keep improving it.

This included a number of changes related to the CPython API.

I additionally bumped the supported python versions in CI.
@JacobCallahan
Copy link
Author

Steps forward in latest commit, but still issues to resolve. It now builds locally in python 3.12.0

$ pip install .
Processing /home/jake/Programming/ssh2-python
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: ssh2-python
  Building wheel for ssh2-python (pyproject.toml) ... done
  Created wheel for ssh2-python: filename=ssh2_python-1.0.0+2.g75176e3.dirty-cp312-cp312-linux_x86_64.whl size=786650 sha256=f130033efc5a35c3e46382234f3033cc5b4f96187f7a5647257b6a9467c8f0c1
  Stored in directory: /home/jake/.cache/pip/wheels/2c/19/89/55dc2bd0f7a333b550bcc8bbe940c10f7bde79d2959852e6de
Successfully built ssh2-python
Installing collected packages: ssh2-python
Successfully installed ssh2-python-1.0.0+2.g75176e3.dirty

However, there appears to be an issue still with the package itself.

In [1]: from ssh2 import channel
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[1], line 1
----> 1 from ssh2 import channel

File ~/Programming/ssh2-python/ssh2/channel.pyx:1, in init ssh2.channel()
----> 1 # This file is part of ssh2-python.
      2 # Copyright (C) 2017-2020 Panos Kittenis
      3 #

File ~/Programming/ssh2-python/ssh2/session.pyx:1, in init ssh2.session()
----> 1 # This file is part of ssh2-python.
      2 # Copyright (C) 2017-2020 Panos Kittenis
      3 #

ValueError: builtins.bool size changed, may indicate binary incompatibility. Expected 32 from C header, got 24 from PyObject

Additionally, the cythonize compatibility layer doesn't play nicely with CI.

@JacobCallahan JacobCallahan marked this pull request as draft November 10, 2023 22:46
@enkore
Copy link
Member

enkore commented Nov 10, 2023

@pbrezina @JacobCallahan I can only push to non-master branches in this org.

@JacobCallahan
Copy link
Author

@enkore do you know of a way to contact @pkittenis? Thanks!

key: depsv3-{{ .Branch }}.{{ arch }}-PY<< parameters.python_ver >>
# - python/load-cache: # This command is unavailable in the orb
# dependency-file: requirements_dev.txt
# key: depsv3-{{ .Branch }}.{{ arch }}-PY<< parameters.python_ver >>
Copy link
Member

Choose a reason for hiding this comment

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

See ParallelSSH/ssh-python@97cfd5c

iirc this is now automated and doesn't have to happen explicitly

Copy link
Author

Choose a reason for hiding this comment

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

That is what I was thinking, so can remove it in follow-up commits.

@enkore
Copy link
Member

enkore commented Nov 10, 2023

I don't

@JacobCallahan
Copy link
Author

@enkore bah, ok. Any help with this would definitely be appreciated!

@enkore
Copy link
Member

enkore commented Nov 10, 2023

I've sent a mail to the mail addresses listed on Github, the website and PyPI (they're all different)

@enkore
Copy link
Member

enkore commented Nov 11, 2023

Regarding the issue here #194 (comment)

The .c files need to be re-cythonized with Cython >= 0.29.34 (cython/cython@43ce558). Cython 3 is the future of course, but that migration might be a bit more involved (mostly for setup.py, I started doing this for ssh-python)

@enkore enkore changed the base branch from master to next November 11, 2023 10:42
@enkore
Copy link
Member

enkore commented Nov 11, 2023

I created a next branch so we can apply patches for the time being; I've done the same over at ssh-python (ParallelSSH/ssh-python#78)

@JacobCallahan
Copy link
Author

@enkore the latest commit is working locally in 3.12 and passing CI in 3.8 and 3.10.

Constrained the Cython version to be less than 3.0, but greater than
0.29.34.
Updated the setup.py to account for the updated cythonize parameters.

However, the appveyor build is failing with the incorrect version error.

circleci: 1.0.0+4
appveyor: 1.0.0-5

@enkore
Copy link
Member

enkore commented Nov 13, 2023

The cause for the appveyor build failure is that there is an extra script involved in the build to generate the version number (https://github.com/ParallelSSH/ssh2-python/blob/master/ci/appveyor/fix_version.py), which doesn't generate a PEP440 compliant version number, which is rejected by newer setuptools. I made this change over in ssh-python to work around that: ParallelSSH/ssh-python@3a7b14e


image
Really need AI to read that one for me :)

@JacobCallahan
Copy link
Author

JacobCallahan commented Nov 13, 2023

@enkore ahh yeah i was digging in and noticed that, but took a bit of a different route
https://github.com/ParallelSSH/ssh2-python/pull/194/files#diff-2fe696be428908583b0de2fe7cc8c2db9089e5ca32f85ce3ab18ccad2dbca84fR8-R9

edit: And Appveyor is now green!

@JacobCallahan JacobCallahan marked this pull request as ready for review November 13, 2023 18:26
@@ -139,17 +139,17 @@ jobs:
steps: *manylinux-steps

workflows:
version: 2.1
# version: 2.1 - default?
Copy link
Member

Choose a reason for hiding this comment

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

2 should work


jobs:
python_test:
parameters:
python_ver:
type: string
default: "3.6"
default: "3.11"
docker:
- image: circleci/python:<< parameters.python_ver >>
Copy link
Member

Choose a reason for hiding this comment

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

cimg/... instead of circleci/... should fix the 3.10+ build failures

Constrained the Cython version to be less than 3.0, but greater than
0.29.34.
Updated the setup.py to account for the updated cythonize parameters.
Also made the appveyor version PEP440 compatible
@JacobCallahan
Copy link
Author

@enkore another update: we're now running into issues with the base test class setup trying to perform a handshake for the session. None of the key files have been touched in 6 years, and I'm able to reproduce these failures locally.

Another note: I update my local Cython to 3.0.5 and rebuilt. Everything built and installed as now with the versions pinned in this PR, so we are likely good to unbound the upper limit for Cython. (tests still failing though)

@JacobCallahan
Copy link
Author

@enkore would it be useful to update the vendored libssh2? I see it's been about 1.5 years since the last update.

@pletnes
Copy link

pletnes commented Nov 15, 2023

Excited to see some activity here, this project is useful to me. Is there work being done on windows binary builds, which are missing from some releases? Is there anything I could do to help?

@JacobCallahan
Copy link
Author

@pletnes if we can get it released via this project, it should be able to use the existing build pipeline to deliver windows binaries.
If you'd like to test out the changes here, you can try pulling my fork locally and pip install from there. You shouldn't need to do anything special since the cython-compiled pieces have already been included.

@enkore
Copy link
Member

enkore commented Nov 16, 2023

Regarding how we can continue with the projects I created a dedicated discussion ticket here: ParallelSSH/parallel-ssh#386

Rebuilt the project with the latest version of Cython and unbound the
upper limit of Cython. This was a big jump, but now it a good time to
make it.
This commit adds all relevant files from the libssh2 repo, as of commit
631e773, with some additional attribution and licenses.
@JacobCallahan
Copy link
Author

Huge scope expansion with the latest two commits. I've both updated the latest compiled filed with Cython 3.0.5 as well as updated the vendored libssh2 for this repo.

However, even with these updates, we're still seeing the issues with key exchanges.

Locally, password-based auth workflows are working flawlessly as exercised by my own project's tests.

@JacobCallahan JacobCallahan changed the title Updates for ConfigParser in python 3.12 Project updates for python 3.12 compatibility Nov 22, 2023
@enkore
Copy link
Member

enkore commented Nov 23, 2023

Thank you very much!
I'll merge this as-is, the KexFailure only popped up after switching the base image, so it's likely an artifact of that (might be an issue with the default settings of openssh-server in whatever distro the CI image is based on). I'll look into it.

@enkore enkore merged commit 6f88957 into ParallelSSH:next Nov 23, 2023
1 of 5 checks passed
@Red-M
Copy link
Contributor

Red-M commented Nov 23, 2023

That KexFailure is due to ssh-rsa keys being depreciated in openssh and thus being made by default, a disabled key exchange for openssh-server.

@enkore
Copy link
Member

enkore commented Nov 23, 2023

Yes, looks like libssh2 is being built without support for the newer rsa-sha2 schemes:

>>> ssh2.session.Session().supported_algs(ssh2.session.LIBSSH2_METHOD_HOSTKEY)
['ecdsa-sha2-nistp256', 'ecdsa-sha2-nistp384', 'ecdsa-sha2-nistp521', 'ecdsa-sha2-nistp256-cert-v01@openssh.com',
 'ecdsa-sha2-nistp384-cert-v01@openssh.com', 'ecdsa-sha2-nistp521-cert-v01@openssh.com',
  'ssh-ed25519', 'ssh-rsa', 'ssh-dss']

Which is kinda strange because both the SHA1 and SHA2 variants should either all be present or entirely absent in the OpenSSL backend:

#ifdef OPENSSL_NO_RSA
# define LIBSSH2_RSA 0
# define LIBSSH2_RSA_SHA1 0
# define LIBSSH2_RSA_SHA2 0
#else
# define LIBSSH2_RSA 1
# define LIBSSH2_RSA_SHA1 1
# define LIBSSH2_RSA_SHA2 1
#endif

@enkore
Copy link
Member

enkore commented Nov 23, 2023

Oh so you see we have two nested copies of the libssh2 source tree and end up building the older, inner one. Whoopsie.

@@ -13,6 +13,7 @@
try:
from Cython.Distutils.extension import Extension
from Cython.Distutils import build_ext
from Cython.Build import cythonize
Copy link
Member

Choose a reason for hiding this comment

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

cythonize should not be used here. Need to commit pre-built cython sources (the .c files) directly or the source files will get re-generated on each build which may not work depending on the system doing the building.

@pkittenis
Copy link
Member

Should also update the https://github.com/ParallelSSH/ssh2-python/blob/master/ci/docker/manylinux/libssh2-1.10.0.tar.gz source file (used for manylinux builds) if updating libssh2 sources.

The build failures look related to which libssh2 is being used for building on the CI. Once those are resolved can be merged.

@JacobCallahan
Copy link
Author

@pkittenis is this something you're wanting to take on, or would you like me to? If the latter, can you provide some documentation of what you'd like done? Ideally, we could use some project documentation for how to handle situations like this in the future.
Thanks!

@besendorf besendorf mentioned this pull request Aug 7, 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
Development

Successfully merging this pull request may close these issues.

7 participants