Skip to content

TST: enable --fatal-meson-warnings for all tests #768

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

TST: enable --fatal-meson-warnings for all tests #768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81a4714
TST: enable --fatal-meson-warnings for all tests
dnicolodi Jun 14, 2025
95cb26e
ENH: define strip binary in synthesized cross files
dnicolodi Jun 14, 2025
d674d94
TST: define minimum meson version required
dnicolodi Jun 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion mesonpy/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,8 @@ def __init__(
cpp = ['c++', '-arch', {arch!r}]
objc = ['cc', '-arch', {arch!r}]
objcpp = ['c++', '-arch', {arch!r}]
strip = ['strip', '-arch', {arch!r}]

[host_machine]
system = 'darwin'
cpu = {arch!r}
Expand All @@ -731,11 +733,12 @@ def __init__(

cross_file_data = textwrap.dedent(f'''
[binaries]
ar = '{arch}-apple-{subsystem}-ar'
c = '{arch}-apple-{subsystem}-clang'
cpp = '{arch}-apple-{subsystem}-clang++'
objc = '{arch}-apple-{subsystem}-clang'
objcpp = '{arch}-apple-{subsystem}-clang++'
ar = '{arch}-apple-{subsystem}-ar'
strip = ['strip', '-arch', {arch!r}]
Copy link
Member Author

Choose a reason for hiding this comment

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

@freakboy3742 Can you please verify that this is correct? I have no experience with building for iOS. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

This will be a valid invocation for strip; but I've never needed to call strip on any iOS program, so I'm not sure where I'd be looking to verify this works. Have you got an example in mind that would be using this?

Copy link
Member Author

@dnicolodi dnicolodi Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! Maybe calling strip in this way and checking that the resulting binary is still working would be a way to verify that this does something that at least is not harmful. The reason I'm adding this is that, without it, meson emits a warning https://github.com/mesonbuild/meson-python/actions/runs/15653996265/job/44102572727#step:10:1105

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood - I'll investigate and report back shortly (likely tomorrow my time, at this point)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the correct way to get to the strip utility is to run something like xcrun --sdk iphoneos -f strip. The system strip may not work for iOS binaries.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would also be an entirely viable option. The only question I'd have is over consistency with the rest of the CPython ecosystem. If a change is made to the shims (like the recent introduction of IPHONEOS_DEPLOYMENT_TARGET enforcement), then meson-python would get that update as soon as the new version of Python is available.

On the other hand, the inverse is also true - meson-python can be in charge of its own destiny, and fix a bug before Python does. This might be more important as Python versions get older - if Python 3.9 had iOS support, and a problem was found, it would be a long time (if at all) before that update propagates. That said, I'm not anticipating a lot of need for this sort of change.

So... 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

I think meson-python releases can happen more easily than CPython releases, thus applying fixes does not seem an issue, as long as we are informed of them. Another advantage is performance: not having the wrappers spares forking one extra sh processes which stays around for the duration of the execution (because the wrappers do not use exec) for each compilation command. Actually using xcrun to query the path to the compiler binaries two forks could be spared. Although, in practice this may be a pretty small performance penalty.

@rgommers What do yo think?

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 to me like the performance difference is going to be very small compared to the overall time it takes to build a wheel. So I'd go with the simplest solution, which is invoking the shims directly. Fixing a bug that may show up in a version of CPython that doesn't get bug fix releases anymore is years away, and if such a bug fix is needed, the unwrapping can then still be done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the performance considerations are moot. Still, a shim for strip does not yet exist and if we invoke xcrun directly for strip I don't see why we cannot do the same for all other compilation tools. I don't know what happens if meson-python writes a cross file that specifies a strip binary that does not exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can, the CPython shims just look a lot more complex.

Anyway, I am fine with that solution too if you prefer, any choice is valid here. Just my $2c that I'd make the minimal change only for strip here (maybe I'm just lazy:)).


[host_machine]
system = 'ios'
Expand Down
15 changes: 15 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,3 +192,18 @@ def cleanenv():
# $MACOSX_DEPLOYMENT_TARGET affects the computation of the platform tag on macOS.
yield mpatch.delenv('MACOSX_DEPLOYMENT_TARGET', raising=False)
mpatch.undo()


@pytest.fixture(autouse=True, scope='session')
def meson_fatal_warnings():
# Cannot use the 'monkeypatch' fixture because of scope mismatch.
mpatch = pytest.MonkeyPatch()
mesonpy_project_init = mesonpy.Project.__init__

def __init__(self, source_dir, build_dir, meson_args=None, editable_verbose=False):
if meson_args is None:
meson_args = {}
meson_args['setup'] = meson_args.get('setup', []) + ['--fatal-meson-warnings']
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
meson_args['setup'] = meson_args.get('setup', []) + ['--fatal-meson-warnings']
meson_args.setdefault('setup', []).append('--fatal-meson-warnings')

mesonpy_project_init(self, source_dir, build_dir, meson_args, editable_verbose)

mpatch.setattr(mesonpy.Project, '__init__', __init__)
2 changes: 1 addition & 1 deletion tests/packages/limited-api/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#
# SPDX-License-Identifier: MIT

project('limited-api', 'c', version: '1.0.0')
project('limited-api', 'c', version: '1.0.0', meson_version: '>= 1.3')

py = import('python').find_installation(pure: false)

Expand Down
2 changes: 1 addition & 1 deletion tests/test_project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@
project = mesonpy.Project(source_dir=package_simple, build_dir=tmp_path)

# Meson configuration points at the cross file
assert project._meson_args['setup'] == ['--cross-file', os.fspath(tmp_path / 'meson-python-cross-file.ini')]
assert project._meson_args['setup'][-2:] == ['--cross-file', os.fspath(tmp_path / 'meson-python-cross-file.ini')]

Check warning on line 403 in tests/test_project.py

View check run for this annotation

Codecov / codecov/patch

tests/test_project.py#L403 

Added line #L403 was not covered by tests

# Meson config files exist, and have some relevant keys
assert (tmp_path / 'meson-python-native-file.ini').exists()
Expand Down
Loading