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: Add 3.12.2 #15955

Draft
wants to merge 63 commits into
base: oi/hipster
Choose a base branch
from

Conversation

Bill-Sommerfeld
Copy link
Contributor

@Bill-Sommerfeld Bill-Sommerfeld commented Feb 5, 2024

This is a very very very rough first draft; it makes it all the way through gmake package and passes most of the tests.
patches-todo has patches I haven't dealt with yet.
patches has a bunch of hacks that need to be cleaned up.

@mtelka
Copy link
Contributor

mtelka commented Feb 5, 2024

Just first suggestion: please switch to components/python/python-312. This is to align with other versioned components. Thank you.

@Bill-Sommerfeld
Copy link
Contributor Author

now down to three main loose ends:

  1. _curses_panel isn't being built; need to get to the bottom of why configure doesn't think it could work.
  2. 08-py_db.patch needs rework as the underlying python internals have changed
  3. need to look more closely at 35-20142.patch and figure out if it's actually been fixed in 3.12 in some different way.

and then need to go over all the patches and clean up warts (particularly the 00*.patch hacks).

@mtelka
Copy link
Contributor

mtelka commented Feb 6, 2024

Tests are happy.

Congrats!

@Bill-Sommerfeld
Copy link
Contributor Author

At this point, the one remaining loose end is 08-py_db.patch which enables MDB python support (including ::pystack); this is likely to be tricky as the interpreter structures used by this patch have changed significantly since Python 3.9 was released and it's going to require a rewrite. I don't see lack of this feature as a reason to delay adding Python 3.12 - at this point it would make sense to squash the commits and start review.

Thoughts?

@mtelka
Copy link
Contributor

mtelka commented Feb 6, 2024

FYI, there is refresh-patches make target.

@iigs
Copy link
Contributor

iigs commented Feb 6, 2024

Thank you for working on python 3.12. I do not have any comments for your recent question. Is there a reason that openssl1.1 needs to be used? Please consider changing "USE_OPENSSL11= yes" to 'OPENSSL_VERSION= 3.1" in the Makefile.

@mtelka
Copy link
Contributor

mtelka commented Feb 6, 2024

I would do not bother with 08-py_db.patch unless it is easily portable from https://github.com/oracle/solaris-userland/blob/master/components/python/python311/patches/08-py_db.patch. If it is not portable then just add a note somewhere (in Makefile likely) so we should look at solaris-userland to see whether the debugger support is available for Python 3.12.

Thank you!

@Bill-Sommerfeld
Copy link
Contributor Author

Is there a reason that openssl1.1 needs to be used? Please consider changing "USE_OPENSSL11= yes" to 'OPENSSL_VERSION= 3.1" in the Makefile.

Fixed as you suggested. I started with python39/Makefile, so USE_OPENSSL11 was in there all along; there are likely other stale things in the Makefile.

@Bill-Sommerfeld
Copy link
Contributor Author

FYI, there is refresh-patches make target.

Done.

@Bill-Sommerfeld
Copy link
Contributor Author

I would not bother with 08-py_db.patch unless it is easily portable from https://github.com/oracle/solaris-userland/blob/master/components/python/python311/patches/08-py_db.patch. If it is not portable then just add a note somewhere (in Makefile likely) so we should look at solaris-userland to see whether the debugger support is available for Python 3.12.

The equivalent file in the solaris-userland patch has an odd copyright statement in it that grants rights only to the PSF so I'm not touching it.

I took a closer look at the data structure differences between 3.9 and 3.12 and it didn't look that bad. I've managed to tweak our version into building; to run it I'll need to build a corresponding /usr/lib/mdb/proc/amd64/libpython3.12.so from illumos-gate, which I think can wait for a bit.

@mtelka
Copy link
Contributor

mtelka commented Feb 7, 2024

@Bill-Sommerfeld, please do NOT squash yet. The history contains valuable information that might be needed during review.

Thank you.

CONFIGURE_ENV += ac_cv_func_epoll_ctl=no
CONFIGURE_ENV += ac_cv_func_epoll_create1=no

CONFIGURE_ENV += ac_cv_func_getentropy=no
Copy link
Contributor

Choose a reason for hiding this comment

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

This is similar to line 119 below, but it is commented there. The linked bug (see below) is closed as resolved (with fix merged) for many years now. Could you please make sure this one is still needed? If so, then please add some reasonable comment explaining the reason.

Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell disabling getentropy is not needed and these lines will be gone in the next push. Truss on a short python script shows that os.urandom() generates a call into the getrandom() syscall passing a zero flag (which is the desirable fast-path, unlikely to block) rather than calling getentropy().

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell disabling getentropy is not needed and these lines will be gone in the next push. Truss on a short python script shows that os.urandom() generates a call into the getrandom() syscall passing a zero flag (which is the desirable fast-path, unlikely to block) rather than calling getentropy().

I assume you trussed with these lines removed... (just to be sure we are on the same page :-))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code in Python/bootstrap_hash.c is structured such that it will only build code to use getentropy if getrandom isn't present:

#if defined(HAVE_GETRANDOM) || defined(HAVE_GETRANDOM_SYSCALL)
#define PY_GETRANDOM 1
...
#elif defined(HAVE_GETENTROPY)
#define PY_GETENTROPY 1
...
#endif /* defined(HAVE_GETENTROPY) && !(defined(__sun) && defined(__SVR4)) */

Just to be sure I'll test this again with a build with the lines removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be sure I'll test this again with a build with the lines removed.

I've retested. As expected, removing the ...getentropy=no lines from the Makefile had no effect on the behavior - os.urandom() results in a getrandom() syscall with them either present or removed.;

@mtelka
Copy link
Contributor

mtelka commented Feb 7, 2024

@Bill-Sommerfeld, you probably already noticed, but if not, then: https://github.com/Bill-Sommerfeld/oi-userland/pulls

@Bill-Sommerfeld Bill-Sommerfeld marked this pull request as ready for review February 8, 2024 08:07
@Bill-Sommerfeld Bill-Sommerfeld changed the title Python 3.12 import: work in progress Python 3.12: Add 3.12.2 Feb 8, 2024
@@ -9,7 +9,7 @@
"library/libffi",
"library/ncurses",
"library/readline",
"library/security/openssl-31",
"library/security/openssl-11",
Copy link
Contributor

Choose a reason for hiding this comment

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

Something is broken with openssl. :-(

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you switched back to 1.1. That explains this change.

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. When I get a chance I'll dig into the test failures but until then, working 1.1 is better than broken 3.1...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sure. Thanks.


Without this patch, python detects and uses epoll which only exists in
OmniOS for lx zones and Linux compatibility. It is not quite the same as
the Linux implementation and can cause socket related failures in python.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is something that needs some love:

  • we are not OmniOS :-)
  • we do not have lx zones
  • it is strange that OmniOS claims (this patch was apparently copied from OmniOS) that since epoll is for Linux compatibility then it should not be used. And the reason is that our implementation is not quite the same as on Linux. The epoll support was added to be fully compatible with Linux and to make porting of software from Linux easier. If there is some notable difference, then it is a bug in illumos that should be (at least) reported, and then investigated and eventually fixed.
  • it should be checked/tested if this patch is still needed; if it is still needed then it means that something in illumos epoll does not work as expected (see above), or there is some bug in Python itself. Either way we need to report this as a bug.

Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

The epoll support was not intended to be fully compatible as some behaviours which were considered as buggy in the Linux implementation were not replicated (there was a presentation by Cantrill along these lines), therefore illumos epoll can only be used reliably in a subset of the situations encountered. I cannot remember the details but there are situations where the Linux epoll "works" which should be considered as undefined behaviour, like referencing resources that are long gone. Mimicing Linux's behaviour would rely on border effects. In such cases disabling epoll was the only reasonable way to go. The good news is that usually epoll would just hang on illumos when such edge cases are met.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the same reason epoll is disabled in Xorg and other graphical toolkits.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, understood. But still we need better comment here in this patch. Reference to more info would be great. For exactly the reason you stated: "I cannot remember..."

Also, hang in epoll (is the hang in illumos or in the epoll consumer?) cannot be considered as good news. It sounds like a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

The hang was in the consumer and we were told that the use case was of one of those not covered by the illumos implementation because the Linux implementation only "works" because of a border effect.
Sorry that I cannot remember better, we had a thread a few years ago but a lot has happened since then.

Copy link
Contributor

@mtelka mtelka Feb 8, 2024

Choose a reason for hiding this comment

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

It looks like there are many problems related to epoll in Python itself.

Examples:

But I found only one (long time ago solved) issue reported there related to illumos and epoll:

So we definitely need more bug reports :-).

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the epoll(7) man page contains all needed information, including caveats about Linux epoll implementation.

@mtelka
Copy link
Contributor

mtelka commented Feb 8, 2024

I think it would be better to mark this as draft so it is not merged accidentally :-).

@mtelka
Copy link
Contributor

mtelka commented Feb 8, 2024

Once #16009 is merged please rebase on top of it and generate new sample-manifest.

@Bill-Sommerfeld
Copy link
Contributor Author

Once #16009 is merged please rebase on top of it and generate new sample-manifest.

Done.

Copy link
Contributor Author

@Bill-Sommerfeld Bill-Sommerfeld left a comment

Choose a reason for hiding this comment

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

I've submitted 9e956fd as a separate pull request: #16200

@AndWac
Copy link
Contributor

AndWac commented May 4, 2024

Any news here?

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.

5 participants