-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-128146: Exclude os/log.h import on older macOS versions. #128165
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.
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
Conversation
!buildbot iOS |
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 9c0b0d2 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
@ned-deily Are you able to explain the macOS build failures here? AFAICT, I can't reproduce these compilation errors locally, and I can't see anything obvious in the CI configuration that is different from my setup. As best as I can make out, the error would be caused if the compilation is being done by a "bare" gcc, rather than clang... but CI is finding the macOS SDK, which should (I think?) be making Am I missing something obvious? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is normal for CI to fail because TARGET_OS_IPHONE
is not defined.
If look at the SDK mirror, this never actually happens. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated review.
@aeiouaeiouaeiouaeiouaeiouaeiou
On older SDKs, sure. But the current mainline has a reference to TARGET_OS_IPHONE, and is currently passing CI.
It definitely does on more recent SDKs (MacOSX SDK 15.2 has this import chain). So - the changes you've suggested definitely make sense for guaranteeing support on older SDKs - but they don't explain why main is passing CI, but this PR doesn't. |
!buildbot iOS |
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit f81a68e 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
And I've worked it out. There was also a stray usage of an OS_LOG symbol when the logging methods were registered, so I've made the compilation of the entire system logger tooling conditional on support existing in the first place. |
!buildbot iOS |
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 0a689b3 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
Looks perfect, now I will test these changes locally on my system and VM. |
Sorry for a delay, I was away from my PowerPC until now. Will run the build now. |
@freakboy3742 Thank you! With your patches it builds now. P. S. There are two other errors:
The second is fixable via:
For the first I just passed a flag to downgrade it back to a warning (pre-gcc14 behavior): |
Thanks for that testing - I'll merge once I've got a review from someone on the core team (that should be mostly procedural, but might take a couple of days given the time of year).
This doesn't appear to be related to the os_log change; could I please ask you to report this as a separate issue.
When you say "known" - known to who? Is there an existing CPython ticket for this problem?
That seems like a reasonable patch based on my reading of the man page for statfs; If you want to submit that change (plus a small docstring explaining why/where the definition is needed) as a PR and tag me, I'll gladly review and merge it. |
@freakboy3742 this PR looks good as is, so when will it be merged? |
@freakboy3742 Thank you and sorry for a delay, a lot of stuff in a pipeline. I will deal with this in a day or two. |
I've been waiting for a review from another member of core; a lot of people have low availability over the new year break. @ned-deily Any chance you'll have time over the next few days to take a look at this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. And I verified that current main builds fail on macOS 10.11 without this PR and no longer fail with it.
Thanks @freakboy3742 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…thonGH-128165) Reworks the handling of Apple system log handling to account for older macOS versions that don't provide os-log. (cherry picked from commit e837a1f) Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
GH-128575 is a backport of this pull request to the 3.13 branch. |
…H-128165) (#128575) gh-128146: Exclude os/log.h import on older macOS versions. (GH-128165) Reworks the handling of Apple system log handling to account for older macOS versions that don't provide os-log. (cherry picked from commit e837a1f) Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
…thon#128165) Reworks the handling of Apple system log handling to account for older macOS versions that don't provide os-log.
…ns. (pythonGH-128165) (python#128575) pythongh-128146: Exclude os/log.h import on older macOS versions. (pythonGH-128165) Reworks the handling of Apple system log handling to account for older macOS versions that don't provide os-log. (cherry picked from commit e837a1f) Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
…ns. (pythonGH-128165) (python#128575) pythongh-128146: Exclude os/log.h import on older macOS versions. (pythonGH-128165) Reworks the handling of Apple system log handling to account for older macOS versions that don't provide os-log. (cherry picked from commit e837a1f) Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
…s. (pythonGH-128165) (python#128575) pythongh-128146: Exclude os/log.h import on older macOS versions. (pythonGH-128165) Reworks the handling of Apple system log handling to account for older macOS versions that don't provide os-log. (cherry picked from commit e837a1f) Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
#127592 recently added support for redirecting stdout/err to the Apple System logs, using the os_log API. This API was added in macOS 10.12; and while the usage of the API was gated with a preprocessor directive, the header declaration was not. This adds preprocessor gating on the header definition.
/cc @barracuda156
pylifecycle.c:50:12: fatal error: os/log.h: No such file or directory
#128146