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

Python 3.12 update. #58

Merged
merged 88 commits into from
Oct 10, 2024
Merged

Python 3.12 update. #58

merged 88 commits into from
Oct 10, 2024

Conversation

dumol
Copy link
Contributor

@dumol dumol commented May 28, 2024

Scope

This was supposed to fix #57.

Changes

Updated Python to version 3.12.7, as there's no pywin32 for Python 3.13 for now. Fixes CVE-2024-28757, CVE-2024-45490, CVE-2024-45491 and CVE-2024-45492.

Also updating OpenSSL for Python's ssl module to 3.0.15 on all platforms, fixing CVE-2024-6119, CVE-2024-6119. On Windows, CVE-2024-6119 and CVE-2024-6119 are now fixed too.

Python modules version updates:

  • pip to 24.2
  • setuptools to 75.1.0 (70.3.0 to match the version in server repo)
  • psutil to 6.0.0.

Updated the Alpine Linux version to build on to 3.15 (with musl 1.2.2). Therefore, musl 1.1.x distributions are no longer supported. This follows upstream cryptography, which only builds musl 1.2 wheels since version 43.0.0.

Drive-by changes:

  • Added support for building beta and rc Python versions.
  • Enabled building against libedit on macOS, where it's a system lib.
  • Removed safety and added requirements.txt to leverage GitHub's own security checks.
  • The requirements.txt file is auto-updated when building successfully on Windows through GHA.
  • Updated pythia.* from server repo with minor improvements.
  • Trimmed the packages by installing only the minimum xz stuff, and by bzip'ing copied header files.

Testing

Automated.

@dumol dumol linked an issue May 28, 2024 that may be closed by this pull request
@dumol dumol self-assigned this May 28, 2024
@dumol dumol force-pushed the 57-python-313-update branch from d25e663 to 87713dd Compare May 31, 2024 14:18
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks for the changes

Moving to 3.12 is an important step toward 3.13

Maybe, in the future we should always focus on gettting the next Python version, instead of trying to jump from 3.11 to 3.13 for example


Can we update the docker tests, to include centos 8 and 9 ?

I have found this issue related to the new libnsl dependency

conda-forge/rasterio-feedstock#220


I hope that we can get the "-dev" packages removed from our build system, and then the Python configure script will disable those modules

@@ -36,6 +40,12 @@ jobs:
timeout-minutes: 5
run: bash ./build.sh test

# Commit changed requirements.txt back to the repository
- uses: chevah/git-auto-commit-action@HEAD
Copy link
Member

Choose a reason for hiding this comment

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

I am not happy with this ... but we can look to fix this in a separate PR

The CI run should not modify the source code

Can we have the "requirements.txt" manually updated before a push ?

I guess that this is a question for #62


What we can do as part of the CI, is make sure that "requirements.txt" doesn't need an update during a CI run

We have something similar for chevah/server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What we can do as part of the CI, is make sure that "requirements.txt" doesn't need an update during a CI run

We have something similar for chevah/server

Not sure what that would be. I know it's used for cache updates, but what do you mean by "making sure it doesn't need an update"?

@@ -0,0 +1,5 @@
pip==24.2
psutil==6.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to distribute psutil with pythia ?

For the long term, my expectation is that pythia, will be just python + embedded pip

No other requirements.

setuptools / pywin32 / pycparser can all be installed later in chevah/server and chevah/compat via pip from our PyPI mirror

Copy link
Contributor Author

@dumol dumol Oct 8, 2024

Choose a reason for hiding this comment

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

It has been added as requested through chevah/python-package#125.

It can also be removed as requested. I've added a ticket to deal with all this: #66

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note though that psutil doesn't have a musl wheel, so we need to build it until upstream changes its mind. More at giampaolo/psutil#2126

Copy link
Member

Choose a reason for hiding this comment

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

thanks. That's ok. We just need to add a comment that psutils is still needed and link to the upstream bug report.

In theory, on alpine, we can build the wheel for psutil and then copy it to bin.chevah.com .. it will be available as wheel for Alpine just for us

@@ -37,7 +37,10 @@ chevahbs_try() {


chevahbs_cp() {
Copy link
Member

Choose a reason for hiding this comment

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

this needs a comment ... I have no idea why we need this and what it does

Copy link
Contributor Author

@dumol dumol Oct 8, 2024

Choose a reason for hiding this comment

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

It's copying needed files after a successful build. All the commands in chevahbs for this library are also cp commands.

src/xz/chevahbs Outdated
@@ -37,7 +37,10 @@ chevahbs_try() {


chevahbs_cp() {
execute "${MAKE_CMD[@]}" install DESTDIR="$INSTALL_DIR"
# xz's installation copies binaries too, but there's no need for them.
execute cp -R src/liblzma/api/* "$INSTALL_DIR"/include/
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these files in the install dir ?

What is the "install dir" ?

I expect them to be needed in the build dir , but they should not be needed for the final package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$INSTALL_DIR is where the built libraries and Python are installed (or simply copied as needed).

Pruning files needed there but not wanted in the final dist package is done through cleanup_install_dir() in functions_build.sh:

cleanup_install_dir() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added #67 for this.

allowed_deps=[
'libc.so.6',
'libcrypt.so.1',
'libdl.so.2',
'libm.so.6',
'libnsl.so.1',
Copy link
Member

Choose a reason for hiding this comment

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

Is this library directly needed by Python 3.12?

Can we build python without NIS module

https://docs.python.org/3/library/nis.html

This module is not used by the Chevah repos ... and will be removed soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't bother much about it, because it's so small and unimportant, soon to be removed.

We used to patch setup.py for disabling such modules, but that file is gone. Other ideas of how to do this with 3.12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.github/workflows/docker.yaml Show resolved Hide resolved
@dumol
Copy link
Contributor Author

dumol commented Oct 8, 2024

Ooops… needs-changes

@dumol
Copy link
Contributor Author

dumol commented Oct 8, 2024

Thanks for the review!

Can we update the docker tests, to include centos 8 and 9 ?

These are not actually "tests", but rather builds. So we build the musl and glibc Linux packages through Docker on Alpine Linux 3.15 and Amazon Linux 2.

Building on other platforms would result in false positives, e.g. for RHEL 8/9 and clones, the build is going to be contaminated with unwanted dependencies.

I have found this issue related to the new libnsl dependency

conda-forge/rasterio-feedstock#220

I've seen that this is part of glibc on Amazon Linux 2, thus not removable. Checked a number of distro containers, all had libnsl libraries, but I see now that some have libnsl.so.2 as an independent package installed by default, with libnsl.so.1 available through a legacy package, similar to libxcrypt-compat.

Obviously, this is a problem that might arise again in the future. Not sure how to best check for this, now that tests in the server repo are not done on supported platforms. But why didn't compat tests catch this issue?

I hope that we can get the "-dev" packages removed from our build system, and then the Python configure script will disable those modules

Well, on Amazon Linux 2 this would be glibc-headers… I'm going to try the same hack as for avoiding building dbm and gdbm modules.

@dumol
Copy link
Contributor Author

dumol commented Oct 8, 2024

The dependency on libnsl.so.1 was removed, the nis module is no longer built on Amazon Linux 2. I've commented above on this, I'm bothered that it wasn't caught by the tests in the compat repo at chevah/compat#700. What shall we do about it?

I've added tickets for the other suggestions, but left the conversations open until your resolution. Let me know if I missed something. Thanks!

needs-review

@adiroiban
Copy link
Member

I'm bothered that it wasn't caught by the tests in the compat repo at chevah/compat#700. What shall we do about it?

Maybe Oracle Linux 8 has that library ... I think that we are ok.


These are not actually "tests", but rather builds.

True. Makes sense.

well... what we can do is create separate docker test runs as part of GitHub actions.
It will work something like this

  • After the binary .tar.gz is created for each OS, upload that file as an GitHub Action artifact
  • After all the build jobs are done, trigger the docker test job
  • THe docker test job will download the GitHub Action Artifacts for this build and will save them in cache
  • The docker test job can then run a few smoke tests to make sure we can start Python on other OSes ... for example run our custom test file

That's all

With GitHub Action Artifacts we can "publish" the pythia .tar.gz files in kind of a temporary location

More info about GitHub Artifacts here https://github.com/actions/upload-artifact

Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

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

Thanks. Let's have this released.

We can then merge compat ...and also I am looking forward to have this included in chevah/server

Many thanks again! Great work!

@dumol dumol merged commit bb41ace into master Oct 10, 2024
5 checks passed
@dumol dumol deleted the 57-python-313-update branch October 10, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python 3.13 update.
3 participants