tests: add a test case for the `MACOSX_DEPLOYMENT_TARGET` env #11011

kleisauke · 2022-11-08T10:07:13Z

It could cause functions to be incorrectly detected as found when
the prefix: '#include <...>' argument is omitted. This only
happens when compiling with a new XCode toolchain while targeting
an older macOS version.

See: #3482.

Context:
lovell/sharp#3438
lovell/sharp-libvips#164

kleisauke · 2022-11-08T10:55:28Z

Commit f96948e didn't work, as expected.

Compiler stdout:
 
Compiler stderr:
 
Checking for function "pthread_jit_write_protect_np" : YES 

test cases/common/260 has_function darwin/meson.build:13:0: ERROR: Assert failed: not cc.has_function('pthread_jit_write_protect_np')

Let's see if commit d07a712 fixes this.

eli-schwartz · 2022-11-08T12:26:24Z

I think the test case could be directly included in test cases/common/36 has function/.

kleisauke · 2022-11-08T12:28:11Z

Ah, looking at commit ac58c13 and issue #3482, I think this bug only occurs when the prefix: '#include <...>' option of cc.has_function() is ommitted for these symbols.

meson/mesonbuild/compilers/mixins/clike.py

Lines 813 to 824 in e68fcac

    
           # If we have any includes in the prefix supplied by the user, assume 
        
           # that the user wants us to use the symbol prototype defined in those 
        
           # includes. If not, then try to do the Autoconf-style check with 
        
           # a dummy prototype definition of our own. 
        
           # This is needed when the linker determines symbol availability from an 
        
           # SDK based on the prototype in the header provided by the SDK. 
        
           # Ignoring this prototype would result in the symbol always being 
        
           # marked as available. 
        
           if '#include' in prefix: 
        
               head, main = self._have_prototype_templ() 
        
           else: 
        
               head, main = self._no_prototype_templ()

Commit 0230490 adds these accordingly, let's see if the CI passes now.

eli-schwartz · 2022-11-08T12:29:13Z

mesonbuild/compilers/mixins/clang.py

+            # Since macOS 10.13 was EOLed on 1 Dec 2020, there's no need to
+            # increase the severity level of the former warning.
+            # https://github.com/lovell/sharp/issues/3438
+            if mesonlib.version_compare(self.version, '>=9.0'):
+                extra_args.append('-Werror=unguarded-availability-new')


I think while we're here it cannot hurt to increase the severity level of checks for people trying to build code to deploy on a relatively recent OS.

I mean, if we were going to drop support for old OSes we'd presumably drop -Wl,-no_weak_imports, right?

Indeed, but -Wunguarded-availability-new would probably also warn about using APIs introduced in macOS 12, if you compile with the MACOSX_DEPLOYMENT_TARGET="11.0" env set, for example.

Anyways, I think this commit can be reverted, it looks like -Wl,-no_weak_imports would already catch this (if you use this in combination with prefix: '#include <...>').

Ah, so this is basically just several different flags that all kind of do the same thing?

...

If there's some flag that helps out such that even function tests that forgot to add a prefixed #include are correctly detected, that would be quite cool. Do the new flags do that, at least?

Ah, so this is basically just several different flags that all kind of do the same thing?

I think so, which is somewhat confusing, but I'm not a macOS expert. 😅

If there's some flag that helps out such that even function tests that forgot to add a prefixed #include are correctly detected, that would be quite cool. Do the new flags do that, at least?

Indeed, that would fix this. But unfortunately, I don't know of any compile/link flags that can do this.

If there's some flag that helps out such that even function tests that forgot to add a prefixed #include are correctly detected, that would be quite cool. Do the new flags do that, at least?

There is no way to do this. That's why I added the note in the docs to discourage using the check without the right #include in the prefix.

There is no way to do this. That's why I added the note in the docs to discourage using the check without the right #include in the prefix.

Thanks for confirming and mentioning this in the docs!

So, to summarize:

Meson cannot do anything about this in terms of implementation.

Meson already recommends using the prefix: '#include <...>' argument in the docs, which would fix such issues.

Meson could issue a warning if that argument is not used (tracked in issue cc.has_function() should warn when the prefix: does not #include a header #3482).

For macOS, Meson could recommend the -Werror=unguarded-availability-new compilation flag in the docs to detect such issues at a later stage.

This PR is only useful as a regression test to check if the MACOSX_DEPLOYMENT_TARGET env is honored.

Since (5) is actually something that should be handled by the compilers (and not the build system), my feeling is that I should abandon this PR. Am I right?

Yeah I feel like this is not worth a test as the compiler handles it, it would be useful if we in the future add a meson option for the iOS macOS, tvOS, watchOS… target, where we would need to make sure that it properly overrides a possible MACOSX_DEPLOYMENT_TARGET set in the env, and does not override when the meson option is not set, so maybe we can resurrect this PR for that then.

I see, abandoning this PR. Thanks for allowing me to test this on CI. 👍

codecov · 2022-11-08T12:44:17Z

Codecov Report

Merging #11011 (341c917) into master (2ec3fe7) will decrease coverage by 2.62%.
The diff coverage is n/a.

❗ Current head 341c917 differs from pull request most recent head 7efd86a. Consider uploading reports for the commit 7efd86a to get more accurate results

@@            Coverage Diff             @@
##           master   #11011      +/-   ##
==========================================
- Coverage   68.83%   66.20%   -2.63%     
==========================================
  Files         414      207     -207     
  Lines       90016    44929   -45087     
  Branches    21278     9927   -11351     
==========================================
- Hits        61960    29744   -32216     
+ Misses      23402    12852   -10550     
+ Partials     4654     2333    -2321

Impacted Files	Coverage Δ
scripts/run_tool.py	`0.00% <0.00%> (-100.00%)`	⬇️
scripts/clangtidy.py	`0.00% <0.00%> (-93.34%)`	⬇️
templates/cstemplates.py	`35.48% <0.00%> (-64.52%)`	⬇️
scripts/coverage.py	`0.00% <0.00%> (-64.36%)`	⬇️
templates/javatemplates.py	`36.66% <0.00%> (-63.34%)`	⬇️
templates/rusttemplates.py	`37.93% <0.00%> (-62.07%)`	⬇️
templates/dlangtemplates.py	`37.93% <0.00%> (-62.07%)`	⬇️
templates/fortrantemplates.py	`39.28% <0.00%> (-60.72%)`	⬇️
scripts/clangformat.py	`0.00% <0.00%> (-50.00%)`	⬇️
modules/icestorm.py	`57.14% <0.00%> (-40.00%)`	⬇️
... and 283 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kleisauke · 2022-11-08T13:06:47Z

Commit 0230490 seems to fix it. Let's see if CI still passes when I revert commit d07a712.

kleisauke · 2022-11-08T14:15:11Z

... that seems to pass CI as well. I'm not sure if Meson can do anything about this, other than what is described in #3482.

I reported these two upstream issues at:
https://gitlab.gnome.org/GNOME/glib/-/issues/2766#note_1590861
https://gitlab.freedesktop.org/gstreamer/orc/-/merge_requests/78/diffs#note_1629043

Let me know if this PR is still appropriate, it could be useful as a regression test.

It could cause functions to be incorrectly detected as found when the `prefix: '#include <...>'` argument is omitted. This only happens when compiling with a new XCode toolchain while targeting an older macOS version. See: mesonbuild#3482.

kleisauke · 2022-12-10T14:27:19Z

Rebased on top of the master branch, since the CI failures on the macOS runner do not seem to be related to this PR.

kleisauke · 2022-12-12T13:06:56Z

I'll abandon this PR, the -Wl,-no_weak_imports linker flag logic is already covered by test cases/osx/3 has function xcode8 added in commit 97c2321 (unfortunately, that test is only run on XCode 8, see e.g. commit d812a0c, so CI does not catch regressions for that test, IIUC).

meson/mesonbuild/compilers/mixins/clang.py

Lines 104 to 110 in 100456d

    
           # Starting with XCode 8, we need to pass this to force linker 
        
           # visibility to obey OS X/iOS/tvOS minimum version targets with 
        
           # -mmacosx-version-min, -miphoneos-version-min, -mtvos-version-min etc. 
        
           # https://github.com/Homebrew/homebrew-core/issues/3727 
        
           # TODO: this really should be communicated by the linker 
        
           if isinstance(self.linker, AppleDynamicLinker) and mesonlib.version_compare(self.version, '>=8.0'): 
        
               extra_args.append('-Wl,-no_weak_imports')

See also the summary at #11011 (comment) for why this cannot be resolved in Meson itself when the prefix: '#include <...>' argument is omitted.

https://gitlab.gnome.org/GNOME/glib/-/issues/2766#note_1590861

FWIW, macOS 10.13 was EOL'ed 2 years ago, so it's probably not worth fixing that in GLib.

https://gitlab.freedesktop.org/gstreamer/orc/-/merge_requests/78/diffs#note_1629043

This is now being tracked in issue https://gitlab.freedesktop.org/gstreamer/orc/-/issues/44. I deliberately didn't open a PR there because I don't have access to iOS devices.

eli-schwartz reviewed Nov 8, 2022

View reviewed changes

kleisauke force-pushed the macosx-deployment-target-env branch from 6b7cf34 to 6acaf4e Compare November 14, 2022 10:07

kleisauke changed the title ~~Draft: add test for MACOSX_DEPLOYMENT_TARGET env~~ tests: add a test case for the MACOSX_DEPLOYMENT_TARGET env Nov 14, 2022

kleisauke marked this pull request as ready for review November 14, 2022 10:08

kleisauke requested a review from jpakkane as a code owner November 14, 2022 10:08

kleisauke mentioned this pull request Nov 14, 2022

cc.has_function() should warn when the prefix: does not #include a header #3482

Open

kleisauke force-pushed the macosx-deployment-target-env branch from 6acaf4e to 7efd86a Compare December 10, 2022 14:25

kleisauke closed this Dec 12, 2022

kleisauke deleted the macosx-deployment-target-env branch December 12, 2022 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: add a test case for the `MACOSX_DEPLOYMENT_TARGET` env #11011

tests: add a test case for the `MACOSX_DEPLOYMENT_TARGET` env #11011

kleisauke commented Nov 8, 2022 •

edited

kleisauke commented Nov 8, 2022

eli-schwartz commented Nov 8, 2022

kleisauke commented Nov 8, 2022

eli-schwartz Nov 8, 2022

kleisauke Nov 8, 2022

eli-schwartz Nov 8, 2022

kleisauke Nov 8, 2022

ePirat Dec 9, 2022

kleisauke Dec 10, 2022

ePirat Dec 10, 2022 •

edited

kleisauke Dec 12, 2022

codecov bot commented Nov 8, 2022 •

edited

kleisauke commented Nov 8, 2022

kleisauke commented Nov 8, 2022

kleisauke commented Dec 10, 2022

kleisauke commented Dec 12, 2022

tests: add a test case for the MACOSX_DEPLOYMENT_TARGET env #11011

tests: add a test case for the MACOSX_DEPLOYMENT_TARGET env #11011

Conversation

kleisauke commented Nov 8, 2022 • edited

kleisauke commented Nov 8, 2022

eli-schwartz commented Nov 8, 2022

kleisauke commented Nov 8, 2022

eli-schwartz Nov 8, 2022

Choose a reason for hiding this comment

kleisauke Nov 8, 2022

Choose a reason for hiding this comment

eli-schwartz Nov 8, 2022

Choose a reason for hiding this comment

kleisauke Nov 8, 2022

Choose a reason for hiding this comment

ePirat Dec 9, 2022

Choose a reason for hiding this comment

kleisauke Dec 10, 2022

Choose a reason for hiding this comment

ePirat Dec 10, 2022 • edited

Choose a reason for hiding this comment

kleisauke Dec 12, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 8, 2022 • edited

Codecov Report

kleisauke commented Nov 8, 2022

kleisauke commented Nov 8, 2022

kleisauke commented Dec 10, 2022

kleisauke commented Dec 12, 2022

tests: add a test case for the `MACOSX_DEPLOYMENT_TARGET` env #11011

tests: add a test case for the `MACOSX_DEPLOYMENT_TARGET` env #11011

kleisauke commented Nov 8, 2022 •

edited

ePirat Dec 10, 2022 •

edited

codecov bot commented Nov 8, 2022 •

edited