Skip to content

Conversation

@ngoldbaum
Copy link
Member

@ngoldbaum ngoldbaum commented Nov 22, 2024

This enables CI for the free-threaded build on Windows. The CI config follows what I did for Linux and MacOS.

It adds two new CI jobs to our Github Actions config.

This also fixes a number of issues with the extbuild helper for testing C extensions, both on the free-threaded build and in general.

While I was here, I fixed some questionable software engineering choices like using mutables as the default value for keyword arguments and an unnecessary try/except I found annoying for debugging.

It also fixes the way libpython is linked against on the free-threaded build. Meson's link_args can use unix-style -L/path -lname_of_library syntax on all platforms, so I used it here even if it looks weird in Windows-specific configuration.

Also fixes issues caused by python.h not setting Py_GIL_DISABLED on Windows, so we need to do that ourselves.

@ngoldbaum ngoldbaum added the 39 - free-threading PRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703) label Nov 22, 2024
@ngoldbaum
Copy link
Member Author

Darn looks like this breaks 32-bit 3.10 on windows. I'll try to fix that on Monday.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks Nathan. Overall this looks good. I would prefer to not add two new jobs, that's a bit too heavyweight. Why not just switch one job over? E.g., use MSVC for 3.11 and Clang-cl for 3.13t. There shouldn't be much (if any) cross-talk between compilers and Python versions.

@ngoldbaum
Copy link
Member Author

Last push adds py.extension_module. I might need some more changes if CI shakes out issues on platforms I didn't test on locally.

@ngoldbaum

This comment was marked as outdated.

@ngoldbaum
Copy link
Member Author

@rgommers I think this is ready now.

@rgommers rgommers added this to the 2.3.0 release milestone Nov 25, 2024
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Looks quite good overall - a few small comments/questions.

if sys.platform == 'win32':
compile_extra = ["/we4013"]
link_extra = ["/LIBPATH:" + os.path.join(sys.base_prefix, 'libs')]
link_extra.append('/DEBUG') # generate .pdb file
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this will be robust, but I don't mind adding it if CI is happy

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just keeping the behavior we had before, there's another sys.platform == 'win32' block below I merged with this one.

I did delete the LIBPATH stuff because meson handles this now.

and os.path.exists(s + 'include')):
include_dirs.append(s + 'include')
if s + 'lib' not in library_dirs and os.path.exists(s + 'lib'):
library_dirs.append(s + 'lib')
Copy link
Member

Choose a reason for hiding this comment

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

This mess can indeed go now I think.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM now, let's give this a go. Thanks Nathan!

@rgommers rgommers merged commit cf95985 into numpy:main Nov 25, 2024
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

39 - free-threading PRs and issues related to support for free-threading CPython (a.k.a. no-GIL, PEP 703) component: CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants