TST: enable --fatal-meson-warnings for all tests #768
Conversation
dnicolodi
force-pushed
the
meson-fatal-warnings
branch
3 times, most recently
from
2725c98 to
0173a9c
Compare
| 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}] |
There was a problem hiding this comment.
@freakboy3742 Can you please verify that this is correct? I have no experience with building for iOS. Thanks!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Understood - I'll investigate and report back shortly (likely tomorrow my time, at this point)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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... 🤷
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:)).
|
There are still test failures. These are the tests where we set the RPATH via explicit linker arguments. We stop doing that in #724 thus I don't think it is necessary to find a way to allow these warnings to do not make the tests fail. |
dnicolodi
force-pushed
the
meson-fatal-warnings
branch
from
0173a9c to
d674d94
Compare
| 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'] |
There was a problem hiding this comment.
| meson_args['setup'] = meson_args.get('setup', []) + ['--fatal-meson-warnings'] | |
| meson_args.setdefault('setup', []).append('--fatal-meson-warnings') |
|
Enabling |
The test packages also work as examples. I think it is better to make sure that they are as correct as possible.