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

Update to upstream Python 3.10.11 #15

Closed
wants to merge 5 commits into from

Conversation

andreittr
Copy link
Contributor

@andreittr andreittr commented Jul 25, 2023

This change set updates the unikraft Python port to 3.10.11, along with necessary compatibility patches.

This is part of a larger work and depends on several other PRs:

Tested by running python's own test suite. Tests usually pass, with some functionality like fork/exec unavailable.
Built with GCC & Clang with the following libraries:

LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-compiler-rt
LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-musl
LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-libffi
LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-lwip
LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-bzip2
LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-sqlite
LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-openssl
LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-libuuid
LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-zlib
LIBS-y := $(LIBS-y):$(UK_LIBS)/lib-python3

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Thanks @andreittr. Besides the few comments:

  • When I try to build with the libraries list you provided an with no extensions enabled for libpython, I get the following build error:
/usr/bin/ld: workdir/apps/python3/build/libffi.o: in function `ffi_prep_cif_var':
workdir/apps/python3/build/libffi/origin/libffi-3.4.4/src/prep_cif.c:236: undefined reference to `ffi_type_sint32'

If the build depends on the python extensions being enabled, maybe we can check that in the Config.uk of the libraries (libffi), or enable the exensions by default.

  • If I build with the extensions enabled it builds fine, but it fails at runtime with the following error:
Python path configuration:
  PYTHONHOME = '/lib/python3.10'
  PYTHONPATH = '/lib/python3.10'
  program name = 'build/python3_qemu-x86_64'
  isolated = 0
  environment = 1
  user site = 1
  import site = 1
  sys._base_executable = '/build/python3_qemu-x86_64'
  sys.base_prefix = '/lib/python3.10'
  sys.base_exec_prefix = '/lib/python3.10'
  sys.platlibdir = 'lib'
  sys.executable = '/build/python3_qemu-x86_64'
  sys.prefix = '/lib/python3.10'
  sys.exec_prefix = '/lib/python3.10'
  sys.path = [
    '/lib/python3.10',
    '/lib/python3.10/lib/python310.zip',
    '/lib/python3.10',
    '/lib/python3.10/lib/python3.10/lib-dynload',
  ]
Fatal Python error: init_fs_encoding: failed to get the Python codec of the filesystem encoding
Python runtime state: core initialized
ModuleNotFoundError: No module named 'encodings'

It works if I change the filesystem and replace all the 3.7 entries to 3.10, so the rootfs.tar.gz (in the app-python3 repository) should also be updated.

Comment on lines +18 to +21
imply LIBLWIP
imply LWIP_DHCP
imply LWIP_DNS
imply LWIP_IPV6
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to imply instead of select?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so one can select an alternative network stack without hacking the Config.uk file, while keeping a sane default.

Makefile.uk Outdated
Comment on lines 82 to 99
LIBPYTHON3_SUPPRESS_FLAGS-y += -Wno-unused-parameter
LIBPYTHON3_SUPPRESS_FLAGS-y += -Wno-unused-variable
LIBPYTHON3_SUPPRESS_FLAGS-y += -Wno-unused-value
LIBPYTHON3_SUPPRESS_FLAGS-y += -Wno-unused-function
LIBPYTHON3_SUPPRESS_FLAGS-y += -Wno-missing-field-initializers
LIBPYTHON3_SUPPRESS_FLAGS-y += -Wno-implicit-fallthrough
LIBPYTHON3_SUPPRESS_FLAGS-$(call clang_version_ge,13,0) += -Wno-cast-function-type
LIBPYTHON3_SUPPRESS_FLAGS-$(call have_gcc) += -Wno-stringop-truncation
LIBPYTHON3_SUPPRESS_FLAGS-y += -Wno-char-subscripts
LIBPYTHON3_SUPPRESS_FLAGS-y += -Wno-sign-compare
LIBPYTHON3_SUPPRESS_FLAGS-$(call have_gcc) += -Wno-maybe-uninitialized
LIBPYTHON3_SUPPRESS_FLAGS-$(call have_clang) += -Wno-documentation-unknown-command
LIBPYTHON3_SUPRESS_CFLAGS-y += $(LIBPYTHON3_SUPPRESS_FLAGS-y) -Wno-pointer-to-int-cast -Wno-int-to-pointer-cast
LIBPYTHON3_SUPRESS_CXXFLAGS-y += $(LIBPYTHON3_SUPPRESS_FLAGS-y)
Copy link
Member

Choose a reason for hiding this comment

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

I would allign these all to += or / and do more than one flag per line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd avoid more than one flag per line unless all flags act as an inseparable whole, otherwise it will make future diffs more unwieldy.
Adding large amounts of whitespace only to align long lines on the assignment operator also looks ugly IMO, so I think cleanest would be to group the conditional flags at the end.

From f607d18e9ee479dd30d4c1f737c08f2d7642122a Mon Sep 17 00:00:00 2001
From: Andrei Tatar <andrei@unikraft.io>
Date: Mon, 10 Jul 2023 16:23:27 +0200
Subject: [PATCH 5/5] Replace getdents64 syscall with libc call
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

@andreittr andreittr Aug 7, 2023

Choose a reason for hiding this comment

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

The patch itself? To be honest I no longer remember, it's been a long time. I did some basic tests now with the two syscall replacing patches removed and it seems ok, so must've been a fix for an issue that's no longer relevant. I'll remove the patches but keep them on hand in case issues spring back up.

@andreittr
Copy link
Contributor Author

* When I try to build with the libraries list you provided an with no extensions enabled for libpython, I get the following build error:

I believe that is due to building libffi (without the python extension requiring it => it's not getting used) but not enabling its option to provide the ffi_type functions; the reason is that either libffi or python's ctypes defines these functions, and by default libffi doesn't provide them itself.
I think this can be made clearer in libffi's Config.uk (make condition negative; have python's ctypes select said condition); will update libffi's PR with that.

* If I build with the extensions enabled it builds fine, but it fails at runtime with the following error:

app-python-3 has not been updated at all for py3.10 (there's no PR for it yet) and the prebuilt rootfs is evidently out of date and could never work. A more extensive update to that repo is in the works.
For the time being please test rebuilding the rootfs with the new version.

This change updates the unikraft port of python to 3.10, bringing
patches, pyconfig.h and modules_config.c up to date.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Previously the python Makefile would forcefully set the Kconfig value
`STACK_SIZE_PAGE_ORDER` to 10, a minimum value for python to work.
This change removes this fairly invasive behavior in favor of an
explicit check and error message if the value is too low.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
This change reworks the make targets that generate the python rootfs:
- Do not use hardcoded python version
- Provide the option of generating .pyc files in the root or not
- Error out or warn the user if a python version mismatch might subtly
  break the resulting rootfs

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
This change removes malloc_closure.c from the build of the ctypes
extension, as its function is provided by libffi, upon which ctypes
depends.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@StefanJum
Copy link
Member

will update libffi's PR with that.

Perfect, thanks.

app-python-3 has not been updated at all for py3.10 (there's no PR for it yet) and the prebuilt rootfs is evidently out of date and could never work. A more extensive update to that repo is in the works. For the time being please test rebuilding the rootfs with the new version.

Yes, I did test using an updated rootfs and everything works fine 🚀 , but I think the updates on app-python3 should be opened and merged as part of this PR set, to keep stuff working.

@andreittr
Copy link
Contributor Author

Yes, I did test using an updated rootfs and everything works fine rocket , but I think the updates on app-python3 should be opened and merged as part of this PR set, to keep stuff working.

I agree, app PR is coming soon.

Reformat to Markdown, remove obsolete references, point users at website

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@andreittr
Copy link
Contributor Author

App update up: unikraft/app-python3#16

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good, thank you @andreittr ❤️
I'll add the tag after the discussions on libuuid and lib-sqlite (on throwing build-time errors instead of using depends on) are solved, so they can be merged before this.

Copy link
Contributor

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

All good on my side, thanks a lot for the consistent work and attention @andreittr @StefanJum!

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Previously the python Makefile would forcefully set the Kconfig value
`STACK_SIZE_PAGE_ORDER` to 10, a minimum value for python to work.
This change removes this fairly invasive behavior in favor of an
explicit check and error message if the value is too low.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #15
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
This change reworks the make targets that generate the python rootfs:
- Do not use hardcoded python version
- Provide the option of generating .pyc files in the root or not
- Error out or warn the user if a python version mismatch might subtly
  break the resulting rootfs

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #15
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
This change removes malloc_closure.c from the build of the ctypes
extension, as its function is provided by libffi, upon which ctypes
depends.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #15
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Reformat to Markdown, remove obsolete references, point users at website

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #15
@andreittr andreittr deleted the ttr/py3.10.11 branch August 11, 2023 16:20
@razvand razvand mentioned this pull request Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/merged enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants