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 Python 3.12.1 #656

Merged
merged 7 commits into from
Dec 22, 2023
Merged

Update to Python 3.12.1 #656

merged 7 commits into from
Dec 22, 2023

Conversation

ismail
Copy link
Contributor

@ismail ismail commented Dec 10, 2023

Refresh 0005-Unvendor-openssl.patch to apply

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@ismail
Copy link
Contributor Author

ismail commented Dec 10, 2023

@conda-forge-admin, please rerender

Refresh 0005-Unvendor-openssl.patch to apply

Signed-off-by: İsmail Dönmez <[email protected]>
@ismail
Copy link
Contributor Author

ismail commented Dec 10, 2023

@conda-forge-admin, please rerender

@ismail
Copy link
Contributor Author

ismail commented Dec 10, 2023

macOS x86-64 builds fail with an ncurses related error

[...]/lib-dynload/_curses.cpython-312-darwin.so, 2): Symbol not found: _extended_color_content

Python has this check to see if it can use _extended_color_content (uninteresting parts omitted):

#if NCURSES_EXT_FUNCS+0 >= 20170401 && NCURSES_EXT_COLORS+0 >= 20170401
#define _NCURSES_EXTENDED_COLOR_FUNCS   1
#else
#define _NCURSES_EXTENDED_COLOR_FUNCS   0
#endif

#if _NCURSES_EXTENDED_COLOR_FUNCS
#define _CURSES_INIT_COLOR_FUNC         init_extended_color
#else
#define _CURSES_INIT_COLOR_FUNC         init_color
#endif  /* _NCURSES_EXTENDED_COLOR_FUNCS */

If I check ncurses package on macOS arm64:

❯ nm -g ./.pixi/env/lib/libncursesw.6.dylib | grep init_extended_color
0000000000009990 T _init_extended_color
0000000000009740 T _init_extended_color_sp

I downloaded the osx-64/ncurses-6.4-h93d8f39_1.conda to double check and sure enough symbol exists there too

❯ file ./lib/libncursesw.6.dylib
./lib/libncursesw.6.dylib: Mach-O 64-bit x86_64 dynamically linked shared library, flags:<NOUNDEFS|DYLDLINK|TWOLEVEL>

❯ nm -g ./lib/libncursesw.6.dylib | grep init_extended_color
0000000000008160 T _init_extended_color
0000000000007eb0 T _init_extended_color_sp

Interestingly, _curses.cpython-312-darwin.so is linking to libncurses.6.dylib

❯ otool -L ./osx-arm64/lib/python3.12/lib-dynload/_curses.cpython-312-darwin.so
./osx-arm64/lib/python3.12/lib-dynload/_curses.cpython-312-darwin.so:
	@rpath/libncurses.6.dylib (compatibility version 6.0.0, current version 6.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.0.0)

which also should make arm64 test fail, but that's not happening.

@ismail
Copy link
Contributor Author

ismail commented Dec 10, 2023

Apparently until python/cpython@d34650e Python3 was not correctly detecting ncursesw on macOS, so this explains why this was not seen before.

@ismail
Copy link
Contributor Author

ismail commented Dec 12, 2023

Someone else, please feel free to pick this up.

I think building non-wide curses on macOS doesn't make sense, but it'd be too big of a hammer. We just need to make sure Python links to ncursesw on macOS.

@mbargull mbargull marked this pull request as draft December 14, 2023 23:06
There was a behavioral change in 3.12.0a1 which omits checking for
separate ncursesw/panelw libraries on macOS.
AFAICT, acc. to those changes, the system ncurses library provides by
Apple has ncursesw/panelw merged into ncurses/panel such that only the
latter need to be linked to.
conda-forge::ncurses builds do have separate ncursesw/panelw, though.

refs:
- python/cpython@e925241#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7L1106

Signed-off-by: Marcel Bargull <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

There are multiple ways we can go about this:

  1. Use this patch.
  2. Upstream this patch.
  3. Change our ncurses builds to make wide character support available from the non-*w names libraries.

re. 1.:
It should restore pre-3.12 linking behavior (look for ncursesw/panelw first and prefer those if found).
The added test command failed for 3.12.0 as expected at https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=844414&view=logs&j=f23f984a-2b8a-59f7-ed33-e22fa8dd9908&t=1d351711-1535-5fad-3bab-ff535f39fb55&l=11742 and confirms that 3.12.1 has proper wide character support in the curses module (one can also see in the logs when ncurses or ncursesw are linked to).
But maintaining such patch here is not optimal, of course.

re. 2.:
There might be some preference to link to ncurses/panel on macOS if they have wide character support (since those from Apple/Homebrew/MacPorts builds do).
This would mean the order of preference would need to be reversed here (possibly some symbol checks added).
Note that for <=3.11 there it would link to the *w libraries first, i.e., just like this patch does again; ref: python/cpython@e925241#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7L1106 .

re. 3.:

I don't know what (if any) downsides there are to not having the wide character supported merged in to the non-*w named libraries.
(^-- This should be copied into a discussion in the ncurses feedstock; noting it here for now so we have context to discuss on what to do with this patch.)

Copy link
Member

Choose a reason for hiding this comment

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

re. 3.: Opened conda-forge/ncurses-feedstock#76 -- that's still WIP (and should only be merged after careful review after the holidays).

@mbargull
Copy link
Member

@conda-forge-admin, please rerender

@mbargull
Copy link
Member

@conda-forge-admin, please rerender

@mbargull
Copy link
Member

@ismail, thanks for looking into this!
I've added a patch so that we link to ncursesw again and added a test so don't regress on this again.

Apparently until python/cpython@d34650e Python3 was not correctly detecting ncursesw on macOS, so this explains why this was not seen before.

Yep, conda-forge::python=3.12.0 definitely has wide character support broken on macOS; confirmed with https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=844414&view=logs&j=f23f984a-2b8a-59f7-ed33-e22fa8dd9908&t=1d351711-1535-5fad-3bab-ff535f39fb55&l=11742 .
The non-3.12 builds should be fine, though, since it broke with python/cpython@e925241 which is in 3.12.0a1 onward.

which also should make arm64 test fail, but that's not happening.

Our macOS CI workers are currently x86-only such that we have to cross-compile the osx-arm64 builds with no automated testing happening for them for now. Meaning, the tests didn't fail because they weren't run.

@mbargull
Copy link
Member

@conda-forge-admin, please restart ci

@mbargull
Copy link
Member

@conda-forge-admin, please rerender

@mbargull
Copy link
Member

My suggestion would be to merge this and revisit further possible actions from #656 (comment) later.

@conda-forge/python, anyone else to review?

@mbargull mbargull merged commit 0c7ef16 into conda-forge:main Dec 22, 2023
10 checks passed
This was referenced Dec 22, 2023
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.

2 participants