python / cpython

The Python programming language
https://www.python.org
Other
63.38k stars 30.35k forks source link

ios buildbot failure: `enclose 'sqlite3_create_window_function' in a __builtin_available check to silence this warning` #120831

Closed sobolevn closed 3 months ago

sobolevn commented 4 months ago

This failed in my PR: https://github.com/python/cpython/pull/120442 But, it does not look related.

Link: https://buildbot.python.org/all/#builders/1380/builds/655

HEAD is now at 8334a1b55c gh-120384: Fix array-out-of-bounds crash in `list_ass_subscript` (#120442)
Switched to and reset branch 'main'

configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

configure: WARNING: no system libmpdecimal found; falling back to bundled libmpdecimal (deprecated and scheduled for removal in Python 3.15)
configure: WARNING: pkg-config is missing. Some dependencies may not be detected correctly.

../../Modules/_sqlite/connection.c:1307:14: warning: 'sqlite3_create_window_function' is only available on iOS 13.0 or newer [-Wunguarded-availability-new]
        rc = sqlite3_create_window_function(self->db, name, num_params, flags,
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator17.5.sdk/usr/include/sqlite3.h:5533:16: note: 'sqlite3_create_window_function' has been marked as being introduced in iOS 13.0 here, but the deployment target is iOS 12.0.0
SQLITE_API int sqlite3_create_window_function(
               ^
../../Modules/_sqlite/connection.c:1307:14: note: enclose 'sqlite3_create_window_function' in a __builtin_available check to silence this warning
        rc = sqlite3_create_window_function(self->db, name, num_params, flags,
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../Modules/_sqlite/connection.c:1315:14: warning: 'sqlite3_create_window_function' is only available on iOS 13.0 or newer [-Wunguarded-availability-new]
        rc = sqlite3_create_window_function(self->db, name, num_params, flags,
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator17.5.sdk/usr/include/sqlite3.h:5533:16: note: 'sqlite3_create_window_function' has been marked as being introduced in iOS 13.0 here, but the deployment target is iOS 12.0.0
SQLITE_API int sqlite3_create_window_function(
               ^
../../Modules/_sqlite/connection.c:1315:14: note: enclose 'sqlite3_create_window_function' in a __builtin_available check to silence this warning
        rc = sqlite3_create_window_function(self->db, name, num_params, flags,
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.

--- xcodebuild: WARNING: Using the first of multiple matching destinations:
{ platform:iOS Simulator, id:C6AEA11C-C34A-47B8-BD67-AF0403ECA353, OS:17.5, name:iPhone SE (3rd generation) }
{ platform:iOS Simulator, id:C6AEA11C-C34A-47B8-BD67-AF0403ECA353, OS:17.5, name:iPhone SE (3rd generation) }
2024-06-21 11:16:45.090 xcodebuild[33218:87319398] [MT] IDETestOperationsObserverDebug: 1075.762 elapsed -- Testing started completed.
2024-06-21 11:16:45.090 xcodebuild[33218:87319398] [MT] IDETestOperationsObserverDebug: 0.000 sec, +0.000 sec -- start
2024-06-21 11:16:45.090 xcodebuild[33218:87319398] [MT] IDETestOperationsObserverDebug: 1075.762 sec, +1075.762 sec -- end
Failing tests:
    -[iOSTestbedTests testPython]

** TEST FAILED **

make: *** [testios] Error 1

Linked PRs

freakboy3742 commented 4 months ago

Thanks for the report. I'll take a closer look on Monday when I'm at my desk, but from a quick first inspection, it looks like there's 2 things going on here.

The first is a compilation warning about the use of an SQLite3 API (sqlite3_create_window_function) that isn't available on iOS 12 (which is what we've currently got set as the minimum iOS API). I'll need to take a closer look, but I'm guessing we either need to bump the minimum supported iOS version, or (as the issue title suggests) put some gates on where the problem function is being used so that the compile warning isn't triggered.

The second problem is a crash of the test suite. This looks like it's a hard crash of the simulator. The next test run has recovered and built fine, so it doesn't seem like it's a problem that #120442 introduced. The compilation issue is just a warning; and given that the test suite runs on an iOS 17.5 simulator, it's not going to be the missing SQLite3 API that is the cause of the test crash. The fact that this is a transient failure leads me to suggest this is an issue on the test machine (or the simulator); unless it becomes reproducible, I don't know there's much we're going to be able to do to address this.

freakboy3742 commented 4 months ago

The specific test failure looks like it was a once off glitch with the simulator. I can't see any explanation for the failure, and the buildbot has been stable for the 28 builds following the one that broke with no other intervention, so I'm inclined to chalk it up to random gremlins. I definitely can't see any reason it would be related to the specific commit that triggered the buildbot failure.

I've also taken a look at the compilation warning. We currently configure a minimum supported iOS version of 12.0; but we compile and test using the current iOS SDK (17.5 at present). The function that is raising the warning (_sqlite3.Connection.create_window_function) has existing error handling if the sqlite3 version number is less than 3.25.0; when running on iOS 12, that branch is triggered. So, there's no actual problem at runtime; this is a mostly cosmetic issue about a compilation warning.

If we put in gating logic around the minimum supported iOS SDK, we'd remove availability of a method that is available on all but the oldest iOS versions; this seems undesirable to me. This is what the error message is recommending with a __builtin_available check.

Another option would be to raise the minimum iOS version to 13.0. This wouldn't be a huge imposition, as Apple is pretty aggressive about pushing version updates; most sources I can find put the market share of iOS 12 as being below 0.2%. However, increasing the minimum version when we actually can support iOS 12 with existing error handling seems excessive.

Yet another option would be to silence the warning; the best option I can think of would be to wrap the create_window_function_impl method in a block of #pragma statements:

#if defined(__APPLE__) && TARGET_OS_IPHONE
#   pragma clang diagnostic push
#   pragma clang diagnostic ignored "-Wunguarded-availability-new"
#endif
... method body here ...
#if defined(__APPLE__) && TARGET_OS_IPHONE
#   pragma clang diagnostic pop
#endif

That would suspend the one specific compiler warning, on iOS, for that one specific function.

Or, we could just ignore the warning on the basis it's cosmetic and not actionable.

Personally, my inclination is to just ignore the compilation warning - if having a compilation warning is unacceptable, bumping to an iOS 13.0 minimum would be my next suggestion.

@ned-deily Any thoughts on this?

ronaldoussoren commented 4 months ago

We use a __builtin_available at a number of places (posixmodule.c, timemodule.c) in similar situations.

In this case I agree that just suppressing the warning would be better due to the check at the start of the function. Ignoring the warning would be fine too, although I'm in general not a fan of doing that because that could hide issues and makes it harder to build with -Werror.

freakboy3742 commented 4 months ago

We use a __builtin_available at a number of places (posixmodule.c, timemodule.c) in similar situations.

Unless I'm missing something, those uses have the same problem - the symbols they're gating ultimately appear in the compiled code; unless those symbols are marked as weak linked, the would generate analogous linking warnings.

ned-deily commented 4 months ago

We use a __builtin_available at a number of places (posixmodule.c, timemodule.c) in similar situations.

Unless I'm missing something, those uses have the same problem - the symbols they're gating ultimately appear in the compiled code; unless those symbols are marked as weak linked, the would generate analogous linking warnings.

I guess the important difference here is that for macOS we use -undefined dynamic_lookup when linking whereas for iOS we don't.

Another option would be to raise the minimum iOS version to 13.0. This wouldn't be a huge imposition, as Apple is pretty aggressive about pushing version updates; most sources I can find put the market share of iOS 12 as being below 0.2%. However, increasing the minimum version when we actually can support iOS 12 with existing error handling seems excessive. [...] Personally, my inclination is to just ignore the compilation warning - if having a compilation warning is unacceptable, bumping to an iOS 13.0 minimum would be my next suggestion.

Eventually this problem will go away when we no longer need to support iOS 12. In the meantime, it's easy enough to change the iOS deployment target at configure time. Perhaps we could change the default in configure.ac to 13.0, display the deployment_target used in the configure output, as we do for the macOS deployment target, to make it a bit more obvious, perhaps document in the iOS README that we do support 12.0 but with the warning and suggest to builders and distributors that they use --host=arm64-apple-ios12.0 et al when building releases if they want to support back that far. We could also configure at least one of our iOS buildbots to explicitly continue to build for 12.0.

Or we could just ignore the warning.

erlend-aasland commented 4 months ago

FTR, we already check for the existence of several SQLite C API functions in configure:

https://github.com/python/cpython/blob/6b280a84988ca221b5bdc1077a914e873790cce5/configure.ac#L4160-L4182

freakboy3742 commented 4 months ago

@erlend-aasland Oh sure - the problem is that those checks pass for iOS, because the APIs do exist on iOS 13+. However, because the minimum supported iOS version defaults to 12, you get a compilation warning that there will be an issue at runtime. There isn't actually an issue at runtime, because there's a runtime check that the SQLite version in use supports the windowing API. This issue is entirely about silencing a warning that can be safely ignored.

We can make this a complete non issue by bumping the default minimum version to iOS 13. That doesn't alter our effective support of iOS 12, but it does make the CI run cleaner, and we can use this as an opportunity to provide a little more documentation and build logging as @ned-deily suggests. I'll work up a patch on that basis.