Skip to content

gh-117886: platform_triplet.c: check if TARGET_OS_* defined #117887

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

jmroot
Copy link
Contributor

@jmroot jmroot commented Apr 15, 2024

Older macOS SDKs don't define any of these, and some clang versions will error if you use them without first checking if they are defined.

Older macOS SDKs don't define any of these, and some clang versions
will error if you use them without first checking if they are defined.
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

I'm intrigued by how old the SDK needs to be to not have these symbols. The use of the OSX name means the symbol should have been introduced any later than 2015, as that was when OS X 10.11 (the last "OS X" version) was released.

Regardless, these changes make sense to me. However, there are also uses of the same symbols in:

Not sure if we want to consolidate all the TARGET_OS fixes into one PR, or if we have a third PR addressing the marshal.c and prim.c versions.

@erlend-aasland
Copy link
Contributor

I'd be fine with one PR.

@ned-deily
Copy link
Member

ned-deily commented Apr 18, 2024

For context, @jmroot is reporting these issues as a maintainer of the MacPorts project which is well-known for providing best-effort support of (much) older versions of Mac hardware and macOS releases.

Since most of the problematic uses were introduced as part of #114099 "Add support for iOS" feature for the upcoming 3.13 release which is still in its alpha stage, my preference would be to treat those as feature fixes and associate any PRs with that issue rather than creating new issues. But I don't have a strong preference about that nor whether to consolidate PRs.

However, I do have a stronger preference about any fix for the pre-existing problematic use in Objects/mimalloc/prim/unix/prim.c since, I believe, that code was essentially vendored into Python from an existing project and thus should be tracked separately and either checked for an existing upstream fix or reported upstream.

@freakboy3742
Copy link
Contributor

Since most of the problematic uses were introduced as part of #114099 "Add support for iOS" feature for the upcoming 3.13 release which is still in its alpha stage, my preference would be to treat those as feature fixes and associate any PRs with that issue rather than creating new issues. But I don't have a strong preference about that nor whether to consolidate PRs.

I've submitted #118073 as a consolidation of the three non-prim.c changes, tagged against the iOS PR.

However, I do have a stronger preference about any fix for the pre-existing problematic use in Objects/mimalloc/prim/unix/prim.c since, I believe, that code was essentially vendored into Python from an existing project and thus should be tracked separately and either checked for an existing upstream fix or reported upstream.

I've opened #118072 to describe the prim.c problem; I've logged the details of my initial investigation there.

@ned-deily
Copy link
Member

Thanks, @jmroot, for reporting the issue(s) and for providing the initial PRs. And, thanks, @freakboy3742, for the follow-ups. Since #118073 incorporates this change, we can close this PR and its issue.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants