Skip to content

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

Merged
rgommers merged 2 commits into
numpy:mainfrom
DWesl:extbuild-meson-specify-python
Jan 24, 2025
Merged

rgommers merged 2 commits into
numpy:mainfrom
DWesl:extbuild-meson-specify-python

Conversation

@DWesl

@DWesl DWesl commented Jan 18, 2025 •

Copy link
Copy Markdown
Contributor

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

charris commented Jan 18, 2025

Copy link
Copy Markdown
Member

close/reopen

@charris charris closed this Jan 18, 2025
@charris charris reopened this Jan 18, 2025
@charris

charris commented Jan 18, 2025

Copy link
Copy Markdown
Member

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

@yakovdan

Copy link
Copy Markdown
Contributor

My PR #28149 fails cygwin_build_test now.

@charris

charris commented Jan 18, 2025

Copy link
Copy Markdown
Member

@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

DWesl commented Jan 18, 2025

Copy link
Copy Markdown
Contributor Author

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

charris commented Jan 20, 2025

Copy link
Copy Markdown
Member

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

@rgommers rgommers left a comment

Copy link
Copy Markdown
Member

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:

Details
$ 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?

Comment thread numpy/testing/_private/extbuild.py Outdated
include_directories: {include_dirs},
)
"""))
# https://github.com/FFY00/meson-python/blob/2510cde44342f3167c44ea05d650c8a524a43879/mesonpy/__init__.py#L615-L618

Copy link
Copy Markdown
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

Comment thread numpy/testing/_private/extbuild.py Outdated
)
else:
subprocess.check_call(["meson", "setup", "--vsenv", ".."],
# https://github.com/FFY00/meson-python/blob/2510cde44342f3167c44ea05d650c8a524a43879/mesonpy/__init__.py#L665-L667

Copy link
Copy Markdown
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

DWesl commented Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

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

@rgommers rgommers left a comment

Copy link
Copy Markdown
Member

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
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