Skip to content

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

Merged
merged 5 commits into from
Jan 7, 2025

Conversation

freakboy3742
Copy link
Contributor

@freakboy3742 freakboy3742 commented Dec 22, 2024

#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

@freakboy3742
Copy link
Contributor Author

!buildbot iOS

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 9c0b0d2 🤖

The command will test the builders whose names match following regular expression: iOS

The builders matched are:

  • iOS ARM64 Simulator PR

@freakboy3742
Copy link
Contributor Author

@ned-deily Are you able to explain the macOS build failures here? AFAICT, TARGET_OS_IPHONE and TARGET_OS_OSX should both be defined by importing AvailabilityMacros.h (which imports TargetConditionals.h). The current main branch has the exact same preprocessor directive on L2967, and it compiles fine - but adding a copy of the exact same preprocessor directive at the top of the file causes both directives to fail to compile.

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 gcc an alias of clang.

Am I missing something obvious?

Copy link
Contributor

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.

@aeiouaeiouaeiouaeiouaeiouaeiou
Copy link
Contributor

aeiouaeiouaeiouaeiouaeiouaeiou commented Dec 22, 2024

AFAICT, TARGET_OS_IPHONE and TARGET_OS_OSX should both be defined by importing AvailabilityMacros.h (which imports TargetConditionals.h).

If look at the SDK mirror, this never actually happens.
I think this behavior has only been fixed in the latest macOS SDKs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated review.

@freakboy3742
Copy link
Contributor Author

@aeiouaeiouaeiouaeiouaeiouaeiou

It is normal for CI to fail because TARGET_OS_IPHONE is not defined.

On older SDKs, sure. But the current mainline has a reference to TARGET_OS_IPHONE, and is currently passing CI.

AFAICT, TARGET_OS_IPHONE and TARGET_OS_OSX should both be defined by importing AvailabilityMacros.h (which imports TargetConditionals.h).

If look at the SDK mirror, this never actually happens.

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.

@freakboy3742
Copy link
Contributor Author

!buildbot iOS

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit f81a68e 🤖

The command will test the builders whose names match following regular expression: iOS

The builders matched are:

  • iOS ARM64 Simulator PR

@freakboy3742
Copy link
Contributor Author

And I've worked it out. os/log.h imports the target conditionals, so when it's included unconditionally, the later references work. However, when the import of os/log.h is made conditional, and you've got an SDK that doesn't include the target conditional import, it fails.

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.

@freakboy3742
Copy link
Contributor Author

!buildbot iOS

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 0a689b3 🤖

The command will test the builders whose names match following regular expression: iOS

The builders matched are:

  • iOS ARM64 Simulator PR

@aeiouaeiouaeiouaeiouaeiouaeiou
Copy link
Contributor

Looks perfect, now I will test these changes locally on my system and VM.

@picnixz picnixz removed 3.13 bugs and security fixes 3.14 bugs and security fixes labels Dec 22, 2024
@barracuda156
Copy link

Sorry for a delay, I was away from my PowerPC until now. Will run the build now.

@barracuda156
Copy link

@freakboy3742 Thank you! With your patches it builds now.

P. S. There are two other errors:

  1. Possibly new one:
/opt/local/bin/gcc-mp-14  -fno-strict-overflow -Wsign-compare -fno-common -dynamic -DNDEBUG -g -O3 -Wall -pipe -Os -arch ppc   -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include -I/opt/local/include   -DPy_BUILD_CORE_BUILTIN -c ./Modules/_threadmodule.c -o Modules/_threadmodule.o
In file included from ./Include/Python.h:64,
                 from ./Modules/_threadmodule.c:4:
./Modules/_threadmodule.c: In function '_thread__get_name_impl':
./Include/pymacro.h:105:5: error: passing argument 3 of 'pthread_getname_np' makes pointer from integer without a cast [-Wint-conversion]
  105 |     (sizeof(array) / sizeof((array)[0]))
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |     |
      |     long unsigned int
./Modules/_threadmodule.c:2381:47: note: in expansion of macro 'Py_ARRAY_LENGTH'
 2381 |     int rc = pthread_getname_np(thread, name, Py_ARRAY_LENGTH(name));
      |                                               ^~~~~~~~~~~~~~~
In file included from ./Include/cpython/pythread.h:17,
                 from ./Include/pythread.h:124,
                 from ./Include/Python.h:120:
/usr/include/pthread.h:360:52: note: expected 'size_t *' {aka 'long unsigned int *'} but argument is of type 'long unsigned int'
  360 | int             pthread_getname_np(pthread_t,char*,size_t*);
      |                                                    ^~~~~~~
  1. The known bug from python 3.13:
/opt/local/bin/gcc-mp-14  -fno-strict-overflow -Wsign-compare -fno-common -dynamic -DNDEBUG -g -O3 -Wall -pipe -Os -arch ppc   -std=c11 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I./Include/internal -I./Include/internal/mimalloc  -I. -I./Include -I/opt/local/include   -DPy_BUILD_CORE_BUILTIN -c ./Modules/posixmodule.c -o Modules/posixmodule.o
./Modules/posixmodule.c: In function '_pystatvfs_fromstructstatfs':
./Modules/posixmodule.c:13231:5: error: static assertion failed: "assuming large file"
13231 |     _Static_assert(sizeof(st.f_blocks) == sizeof(long long), "assuming large file");
      |     ^~~~~~~~~~~~~~

The second is fixable via:

--- Modules/posixmodule.c
+++ Modules/posixmodule.c	2024-10-31 03:05:47.000000000 +0800
@@ -13212,6 +13212,11 @@
  * os.statvfs is implemented in terms of statfs(2).
  */
 
+#if defined(__i386__) || defined(__ppc__)
+#define statfs statfs64
+#define fstatfs fstatfs64
+#endif
+
 static PyObject*
 _pystatvfs_fromstructstatfs(PyObject *module, struct statfs st) {
     PyObject *StatVFSResultType = get_posix_state(module)->StatVFSResultType;

For the first I just passed a flag to downgrade it back to a warning (pre-gcc14 behavior): -Wno-error=int-conversion.

@freakboy3742
Copy link
Contributor Author

@freakboy3742 Thank you! With your patches it builds now.

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).

P. S. There are two other errors:

  1. Possibly new one:

This doesn't appear to be related to the os_log change; could I please ask you to report this as a separate issue.

  1. The known bug from python 3.13:

When you say "known" - known to who? Is there an existing CPython ticket for this problem?

The second is fixable via:

--- Modules/posixmodule.c
+++ Modules/posixmodule.c	2024-10-31 03:05:47.000000000 +0800
@@ -13212,6 +13212,11 @@
  * os.statvfs is implemented in terms of statfs(2).
  */
 
+#if defined(__i386__) || defined(__ppc__)
+#define statfs statfs64
+#define fstatfs fstatfs64
+#endif
+
 static PyObject*
 _pystatvfs_fromstructstatfs(PyObject *module, struct statfs st) {
     PyObject *StatVFSResultType = get_posix_state(module)->StatVFSResultType;

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.

@aeiouaeiouaeiouaeiouaeiouaeiou
Copy link
Contributor

@freakboy3742 this PR looks good as is, so when will it be merged?

@barracuda156
Copy link

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 Thank you and sorry for a delay, a lot of stuff in a pipeline. I will deal with this in a day or two.

@freakboy3742
Copy link
Contributor Author

@freakboy3742 this PR looks good as is, so when will it be merged?

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?

Copy link
Member

@ned-deily ned-deily left a 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.

@freakboy3742 freakboy3742 merged commit e837a1f into python:main Jan 7, 2025
50 checks passed
@miss-islington-app
Copy link

Thanks @freakboy3742 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 7, 2025
…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>
@bedevere-app
Copy link

bedevere-app bot commented Jan 7, 2025

GH-128575 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 7, 2025
@freakboy3742 freakboy3742 deleted the old-macos-fix branch January 7, 2025 05:14
freakboy3742 added a commit that referenced this pull request Jan 7, 2025
…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>
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
…thon#128165)

Reworks the handling of Apple system log handling to account for older macOS 
versions that don't provide os-log.
freakboy3742 added a commit to freakboy3742/cpython that referenced this pull request Mar 16, 2025
…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>
freakboy3742 added a commit to freakboy3742/cpython that referenced this pull request Mar 16, 2025
…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>
freakboy3742 added a commit to freakboy3742/cpython that referenced this pull request Mar 16, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants