GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-117886: platform_triplet.c: check if TARGET_OS_* defined by jmroot · Pull Request #117887 · python/cpython

Conversation

@jmroot

@jmroot jmroot commented Apr 15, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

  • Issue: platform_triplet.c uses possibly undefined TARGET_OS_* macros #117886

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jmroot
pythongh-117886: platform_triplet.c: check if TARGET_OS_* defined
c36fce1
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.
@bedevere-app bedevere-app Bot added the awaiting review label Apr 15, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Apr 15, 2024
platform_triplet.c uses possibly undefined TARGET_OS_* macros #117886
Closed
@erlend-aasland
erlend-aasland requested a review from freakboy3742 April 15, 2024 11:07
freakboy3742
freakboy3742 reviewed Apr 16, 2024
View reviewed changes

@freakboy3742 freakboy3742 left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

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:

  • Modules/_testexternalinspection.c (seems to be addressed in #117891?)
  • Python/marshal.c
  • Objects/mimalloc/prim/unix/prim.c (these definitions predate the PEP 730 work, and use symbols that were deprecated in iOS 15)

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@erlend-aasland

erlend-aasland commented Apr 16, 2024

Copy link
Copy Markdown
Contributor

I'd be fine with one PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@freakboy3742 freakboy3742 mentioned this pull request Apr 18, 2024
gh-117891: handle undefined TARGET_OS_OSX #117892
Closed
@ned-deily

ned-deily commented Apr 18, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@freakboy3742 freakboy3742 mentioned this pull request Apr 19, 2024
gh-114099: Modify preprocessor symbol usage to support older macOS SDKs #118073
Merged
@freakboy3742

freakboy3742 commented Apr 19, 2024

Copy link
Copy Markdown
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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ned-deily

ned-deily commented Apr 19, 2024

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ned-deily ned-deily closed this Apr 19, 2024
@ned-deily ned-deily mentioned this pull request Apr 19, 2024
_testexternalinspection.c doesn't check for undefined TARGET_OS_OSX #117891
Closed
@jmroot
jmroot deleted the platform_triplet-fix branch April 19, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@freakboy3742 freakboy3742 freakboy3742 left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@jmroot @erlend-aasland @ned-deily @freakboy3742
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.