Skip to content

MAINT: testing: specify python executable to use in extbuild #28183

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

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

DWesl
Copy link
Contributor

@DWesl DWesl commented Jan 18, 2025

This would cause failures during testing distribution packages on Cygwin. The code is copied from meson-python, as advised in mesonbuild/meson-python#51 and amended in mesonbuild/meson-python#137.
Note the changed organization in many of the links.

I found the failure specifically in numpy._core.tests.test_array_interface.py on Cygwin, where the system meson has a python3.9 shebang, so the test failed when it couldn't find a 3.12 extension module.

This would cause failures during testing distribution packages on Cygwin.
The code is copied from meson-python, as advised in mesonbuild/meson-python#51
and amended in mesonbuild/meson-python#137.
Note the changed organization in many of the links.
@charris
Copy link
Member

charris commented Jan 18, 2025

close/reopen

@charris charris closed this Jan 18, 2025
@charris charris reopened this Jan 18, 2025
@charris
Copy link
Member

charris commented Jan 18, 2025

I have reenabled the cygwin test, let's see how it does.

@yakovdan
Copy link
Contributor

My PR #28149 fails cygwin_build_test now.

@charris
Copy link
Member

charris commented Jan 18, 2025

@yakovdan Just ignore it for now, if this PR continues to fail for a few days I will disable it again.

@DWesl Perhaps you could disable this test in the code so that it only runs in the PR.

@DWesl
Copy link
Contributor Author

DWesl commented Jan 18, 2025

DWesl#68 tracks my progress in getting Cygwin's Python 3.12 test package working, just re-enabling the current Python 3.9 CI on Cygwin is likely to fail. I don't expect either Cygwin Python version to pass CI at this time.

This particular error would primarily affect distribution package managers testing packages for multiple Python versions at once.

The test I found it in is currently skipped on Cygwin, and will likely remain so until I convince CI to run to completion, rather than hanging for six hours at around 26% completion. I found that test because a test run hung after saying it started that file. Running just that test succeeded, but I decided to run the test anyway and find out why it failed. After a few times staring at the test output saying it successfully created a Py3.9 extension but couldn't find the 3.12 extension, I worked out what kind of problem it was and found the meson-python issue with the solution.

@charris
Copy link
Member

charris commented Jan 20, 2025

Going to disable the test again. @DWesl keep us posted on your progress.

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 @DWesl. These changes look fine to me. This same pattern is present in several other test files that build extension modules:

$ rg -C2 native-file
_core/tests/test_limited_api.py
43-    # Ensure we use the correct Python interpreter even when `meson` is
44-    # installed in a different Python environment (see gh-24956)
45:    native_file = str(build_dir / 'interpreter-native-file.ini')
46-    with open(native_file, 'w') as f:
47-        f.write("[binaries]\n")
--
57-                               "--werror",
58-                               "--buildtype=release",
59:                               "--vsenv", "--native-file", native_file,
60-                               str(srcdir)],
61-                              cwd=build_dir,
--
63-    else:
64-        subprocess.check_call(["meson", "setup", "--werror",
65:                               "--native-file", native_file, str(srcdir)],
66-                              cwd=build_dir
67-                              )

_core/tests/test_cython.py
44-    # Ensure we use the correct Python interpreter even when `meson` is
45-    # installed in a different Python environment (see gh-24956)
46:    native_file = str(build_dir / 'interpreter-native-file.ini')
47-    with open(native_file, 'w') as f:
48-        f.write("[binaries]\n")
--
57-        subprocess.check_call(["meson", "setup",
58-                               "--buildtype=release",
59:                               "--vsenv", "--native-file", native_file,
60-                               str(srcdir)],
61-                              cwd=build_dir,
--
63-    else:
64-        subprocess.check_call(["meson", "setup",
65:                               "--native-file", native_file, str(srcdir)],
66-                              cwd=build_dir
67-                              )
--
71-        print("----------------")
72-        print("meson build failed when doing")
73:        print(f"'meson setup --native-file {native_file} {srcdir}'")
74-        print("'meson compile -vv'")
75-        print(f"in {build_dir}")

random/tests/test_extending.py
66-    # Ensure we use the correct Python interpreter even when `meson` is
67-    # installed in a different Python environment (see gh-24956)
68:    native_file = str(build_dir / 'interpreter-native-file.ini')
69-    with open(native_file, 'w') as f:
70-        f.write("[binaries]\n")
--
74-        subprocess.check_call(["meson", "setup",
75-                               "--buildtype=release",
76:                               "--vsenv", "--native-file", native_file,
77-                               str(build_dir)],
78-                              cwd=target_dir,
--
80-    else:
81-        subprocess.check_call(["meson", "setup",
82:                               "--native-file", native_file, str(build_dir)],
83-                              cwd=target_dir
84-                              )

For some reason it was overlooked here and didn't cause a problem before. Making this change can only improve correctness.

Are you happy with this, or still iterating?

@@ -220,14 +220,22 @@ def build(cfile, outputfilename, compile_extra, link_extra,
include_directories: {include_dirs},
)
"""))
# https://github.com/FFY00/meson-python/blob/2510cde44342f3167c44ea05d650c8a524a43879/mesonpy/__init__.py#L615-L618
Copy link
Member

Choose a reason for hiding this comment

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

Fine to leave out this comment in my opinion

if sys.platform == "win32":
subprocess.check_call(["meson", "setup",
"--buildtype=release",
"--vsenv", ".."],
cwd=build_dir,
)
else:
subprocess.check_call(["meson", "setup", "--vsenv", ".."],
# https://github.com/FFY00/meson-python/blob/2510cde44342f3167c44ea05d650c8a524a43879/mesonpy/__init__.py#L665-L667
Copy link
Member

Choose a reason for hiding this comment

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

same here, no need for the comment

@DWesl
Copy link
Contributor Author

DWesl commented Jan 24, 2025

Are you happy with this, or still iterating?

I'll likely have a few more changes as I try to get NumPy compiling on Cygwin's Python 3.12 test release, but it'll be a while before I get python to stop hanging for long enough to get test results. This is complete as it stands, so I'm fine with it going in.

@rgommers rgommers added this to the 2.3.0 release milestone Jan 24, 2025
@rgommers rgommers changed the title ENH: Specify python version in extbuild. MAINT: testing: specify python executable to use in extbuild Jan 24, 2025
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.

Sounds good, in it goes. Thanks @DWesl!

@rgommers rgommers merged commit 0b49d43 into numpy:main Jan 24, 2025
67 checks passed
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.

4 participants