Skip to content
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

gh-114099: Refactor configure and Makefile to accomodate non-macOS frameworks #115120

Merged
merged 6 commits into from Feb 12, 2024

Conversation

freakboy3742
Copy link
Contributor

@freakboy3742 freakboy3742 commented Feb 7, 2024

Part of the PEP 730 work to add iOS support.

This PR lays the groundwork for introducing iOS/tvOS/watchOS frameworks. It is a subset of #115063; that PR may be helpful for seeing the full context for some of the changes that have been made. This PR doesn't add anything iOS specific - it only includes the structural refactoring needed so that iOS branches can be added into in a subsequent PR.

Summary of changes:

  • Updates config.sub to the 2024-01-01 release. This is the "as released" version of config.sub.
  • Adds a RESSRCDIR variable to allow sharing of macOS and iOS Makefile steps
  • Adds an INSTALLTARGETS variable so platforms can customise which targets are actually installed. This will be used to exclude certain targets (e.g., binaries, manfiles) from iOS framework installs.
  • Adds a PYTHONFRAMEWORKINSTALLNAMEPREFIX variable; this is used as the install name for the library. This is needed to allow for iOS frameworks to specify an @rpath-based install name.
  • Evaluates MACHDEP earlier in the configure process so that ac_sys_system is available
  • Modifies _PYTHON_HOST_PLATFORM evaluation for cross-platform builds so that the CPU architecture is differentiated from the host identifier. This will be used to generate a _PYTHON_HOST_PLATFORM definition that includes ABI information, not just CPU architecture.
  • Differentiates between SOABI_PLATFORM and PLATFORM_TRIPLET. SOABI_PLATFORM is used in binary module names, and includes the ABI, but not the OS or CPU architecture (e.g., math.cpython-313-iphonesimulator.dylib). PLATFORM_TRIPLET is used as the sys._multiarch value, and on iOS will contains the ABI and architecture (e.g., iphoneos-arm64). This differentiation hasn't historically been needed because while macOS is a multiarch platform, it uses a bare darwin as PLATFORM_TRIPLE.
  • Removes the use of the deprecated -Wl,-single_module flag when compiling macOS frameworks.
  • Some whitespace normalisation where there was a mix of spaces and tabs in a single block.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Thanks!

@erlend-aasland
Copy link
Contributor

I'll wait for Ned's thumbs up before merging, since you requested his review :)

@erlend-aasland erlend-aasland self-assigned this Feb 7, 2024
Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

Differentiates between SOABI_PLATFORM and PLATFORM_TRIPLET. SOABI_PLATFORM is used in binary module names, and includes the ABI, but not the OS or CPU architecture (e.g., math.cpython-313-iphonesimulator.dylib). PLATFORM_TRIPLET is used as the sys._multiarch value, and on iOS will contains the ABI and architecture (e.g., iphoneos-arm64). This differentiation hasn't historically been needed because while macOS is a multiarch platform, it uses a bare darwin as PLATFORM_TRIPLE.

Why is this differentiation needed now? Using the triplet directly would avoid introducing yet another platform identification variable on top of the dozen or more that already exist in this script.

Also, looking ahead to the next PR in this series, I'm not sure about the PLATFORM_TRIPLET format. The existing platforms all use an actual autoconf triplet with the architecture first, except for macOS which just returns "darwin" on all architectures. iphoneos-arm64 doesn't follow either of those conventions.

Using an actual autoconf triplet would cause "ios" to be duplicated in the sysconfigdata name, but that's already the case on Linux and macOS, and in such a complex area it's helpful to be consistent as much as possible.

configure.ac Outdated Show resolved Hide resolved
@freakboy3742
Copy link
Contributor Author

Differentiates between SOABI_PLATFORM and PLATFORM_TRIPLET. SOABI_PLATFORM is used in binary module names, and includes the ABI, but not the OS or CPU architecture (e.g., math.cpython-313-iphonesimulator.dylib). PLATFORM_TRIPLET is used as the sys._multiarch value, and on iOS will contains the ABI and architecture (e.g., iphoneos-arm64). This differentiation hasn't historically been needed because while macOS is a multiarch platform, it uses a bare darwin as PLATFORM_TRIPLE.

Why is this differentiation needed now? Using the triplet directly would avoid introducing yet another platform identification variable on top of the dozen or more that already exist in this script.

The tl;dr is "because this is what the PEP proposed", with a side order of "that's what I've got working"; but I'll grant that dogmatic adherence to a spec or implementation isn't a very satisfying answer :-)

The longer answer is that iOS has some peculiarities that mean it doesn't fit neatly into the "GNU triple" nomenclature, most of which have existing analogs in the CPython codebase. Also, while the iOS changes around PLATFORM_TRIPLET et al do introduce some additional logic and temporary working variables, the set of variables used at runtime haven't changed. SOABI_PLATFORM isn't used externally - it's only used as a component of the previously existing SOABI.

iOS (and tvOS/watchOS) is a sort of hybrid between the Linux and macOS situation. Unlike Linux, it's a platform where a single binary can/will contain multiple architectures. Unlike macOS, there's multiple incompatible ABIs - the device ABI and the simulator ABI.

Using a true "triple" in PLATFORM_TRIPLET (and thus MULTIARCH and sysconfigdata) would be redundant. The triple is needed for Unix platforms because there are different Unix kernels and C library implementations, in addition to CPU differences. x86_64-linux-gnu tells you three useful things about the platform, and as an added benefit, the rest of the Unix compiler ecosystem uses these triples as configuration values. arm64-apple-ios tells you 2 things, because there isn't a non-Apple iOS; and the only parts of the Apple compiler ecosystem that honor the "triple" form are the parts that are trying to fit into the Unix compiler ecosystem (usually badly, because of the single CPU architecture assumptions baked into the triple format). Native iOS tooling uses iphoneos or iphonesimulator descriptors. iphoneos-arm64 tells you everything you need to know about a single architecture artefact.

It's not just macOS that breaks this "triple" convention, either. WASM/Emscripten and GNU Hurd both avoid the "triple" form (using wasm-emscripten et al, and x86_64-gnu et al respectively), for largely analogous reasons, AFAICT.

You might note that macOS doesn't include the CPU architecture in MULTIARCH. That's because it is possible to compile a universal2 build of macOS in a single pass; and as a result, a single sysconfigdata module can be used. This isn't true of iOS; you can't (to the best of my knowledge) do a single-pass, multi-CPU architecture compilation of an iOS target, so we need to differentiate which sysconfigdata module is appropriate at runtime.

Including the CPU architecture in the SOABI tag is problematic because of how the binary is used at runtime. We can't distribute multiple binaries to support multiple CPU architectures on iOS - we have to use fat binaries that include multiple CPU architectures. When loading a binary module, we need to provide a single unique name to load, and that name can't include a CPU architecture.

This means we're left with needing an SOABI that doesn't include an architecture description. An alternate approach might be to include the architecture in the SOABI name, but havean installation process that strips the architecture off the binary name as part of compile/link process, and a runtime environment that knows that the runtime SOABI is different from the compile-time SOABI. However, that would involve a lot more moving parts and iOS eccentricities than just using an architecture neutral, ABI-specific SOABI tag in the first place. This also mirrors the macOS experience, which uses darwin as the SOABI, regardless of whether it's a x86, arm64, or universal2 binary.

Admittedly, the SOABI issue is slightly muted at present because there's only 1 CPU architecture on physical devices at present, and simulator binaries don't have to be distributed. However, given that the platform has a history of supporting multiple CPU architectures, it seems unwise to bake the assumption that there will only ever be a single CPU architecture into the naming scheme for iOS/tvOS/watchOS binaries.

With all that said - one inconsistency in the PEP/#115063 that might be worth addressing is the ordering of the iOS PLATFORM_TRIPLET value. The convention on every other platform is to put the CPU architecture before the ABI/kernel details, so arm64-iphoneos would be more consistent. That doesn't actually change that much, other than the exact name of of some binary modules.

@freakboy3742
Copy link
Contributor Author

I'll wait for Ned's thumbs up before merging, since you requested his review :)

FWIW - I tagged @ned-deily for review because it seemed appropriate given he's the core team sponsor for this work, and he weighed in on the #115063 version of this patch. While I'd certainly value his input, if it's not needed in terms of CPython core's process, I don't have any inherent reason to need Ned's sign-off specifically. If y'all are happy with me sticking to the default Bevedere reviewers, I'm happy to do that (or, if you'd like me to keep tagging Ned on the iOS patches, I'm happy to do that too).

@ned-deily
Copy link
Member

Thanks for the opportunity to review it. I will be looking at it shortly.

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.

As you know, there are a lot of configure options for framework builds - on top of the enormous number of other configure options - and we currently do very little if any CI or buildbot testing of them. So my highest review priority is to try to ensure there are no regressions with existing configurations of interest (on macOS). So I think you should test a few of these on macOS manually when making updates of PRs affecting configure.ac and Makefile.pre.in. What would be ideal would be to add at least some of these configurations to our standard CI and/or buildbots but that is a bit out of scope here (although PRs welcome).

As the PR stands at this moment, these all fail on my current macOS (14.3, case-sensitive) systems:

# build but avoid installing into /Library and /usr/local
1. ./configure --enable-framework && make -j4 && make pythoninfo

# alternate install location
2. ./configure --enable-framework=$PWD/root/Library/Frameworks && make -j4 && make install && $PWD/root/bin/python3 -m test.pythoninfo

# alternate install location with debug build and with alternate framework name
3. ./configure --enable-framework=$PWD/root/Library/Frameworks --with-framework-name=pytest --with-pydebug && make -j4 && make install && $PWD/root/bin/python3 -m test.pythoninfo 

I'd suggest running on a current HEAD of main to see the results without the PR and then with the PR. Once those tests pass, then we should run the whole test suite before and after. And then finally add in a universal2 configuration though that is more tricky because it requires universal builds of the third-party dependencies; that is something I can test at that point.

Once we feel there are no macOS regressions, we can dig deeper into the iOS changes.

@bedevere-app
Copy link

bedevere-app bot commented Feb 8, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@mhsmith
Copy link
Member

mhsmith commented Feb 8, 2024

The longer answer is ...

Thanks, that all makes sense. Let's just make sure that in the next PR when we add the iOS-specific stuff, that this is explained clearly enough in the code comments so it doesn't get forgotten.

The convention on every other platform is to put the CPU architecture before the ABI/kernel details, so arm64-iphoneos would be more consistent. That doesn't actually change that much, other than the exact name of of some binary modules.

The only thing it would change is the sysconfigdata filename, right? In which case, it probably isn't worth changing now when it's already specified in the PEP, and the wheel tag uses the same order.

@freakboy3742
Copy link
Contributor Author

freakboy3742 commented Feb 9, 2024

As the PR stands at this moment, these all fail on my current macOS (14.3, case-sensitive) systems:

I've found the source of the problem - all three seem to have the same (obvious in hindsight) cause. I've updated the PR with the fix.

Once those tests pass, then we should run the whole test suite before and after. And then finally add in a universal2 configuration though that is more tricky because it requires universal builds of the third-party dependencies; that is something I can test at that point.

I've run an additional universal2 test building on case (3):

4. ./configure --enable-universalsdk="`xcrun --show-sdk-path`" --with-universal-archs=universal2 --enable-framework=$PWD/root/Library/Frameworks --with-framework-name=pytest --with-pydebug && make -j4 && make install && $PWD/root/bin/python3 -m test.pythoninfo 

This succeeded. I then used this configuration to run $PWD/root/bin/python3.13 -m test -j 4. This wasn't completely clean - I got 3 test failures, one each in test_site, test_sys, and test_venv - but as far as I can make out, they all seemed to be due to the test case not fully accomodating the custom installation path and framework name. The same failures occur on the equivalent build & run on a checkout of main.

Lastly, I did a non-framework, installed, universal2 build and test:

5. ./configure --enable-universalsdk="`xcrun --show-sdk-path`" --with-universal-archs=universal2 --prefix=/Users/rkm/projects/python/host && make -j 4 all install && /Users/rkm/projects/python/host/bin/python3.13 -m test -j 4

This passed without error.

@freakboy3742
Copy link
Contributor Author

freakboy3742 commented Feb 9, 2024

The longer answer is ...

Thanks, that all makes sense. Let's just make sure that in the next PR when we add the iOS-specific stuff, that this is explained clearly enough in the code comments so it doesn't get forgotten.

Will do.

[Edit - I've included a description in this PR]

The convention on every other platform is to put the CPU architecture before the ABI/kernel details, so arm64-iphoneos would be more consistent. That doesn't actually change that much, other than the exact name of of some binary modules.

The only thing it would change is the sysconfigdata filename, right? In which case, it probably isn't worth changing now when it's already specified in the PEP, and the wheel tag uses the same order.

Correct, those are the only two locations it would manifest (or, at least, the only places I can think that it would manifest). Ultimately, it doesn't really matter; I just hate seeing inconsistencies in patterns.

Plus, there will need to be a final PEP update to reflect the "implemented" status - AIUI, this update usually includes any tweaks and modifications identified during the final implementation process (see PEP Maintenance in PEP1). If that process suggests a name change is appropriate, the overhead of updating the PEP to reflect that shouldn't be a major consideration, IMHO.

@freakboy3742
Copy link
Contributor Author

I have made the requested changes; please review again.

@mhsmith
Copy link
Member

mhsmith commented Feb 9, 2024

When a PR has already been reviewed, it's probably better to bring in changes from the main branch with a merge rather than a rebase, so the reviewer can use the "changes since last review" feature.

@erlend-aasland
Copy link
Contributor

When a PR has already been reviewed, it's probably better to bring in changes from the main branch with a merge rather than a rebase, so the reviewer can use the "changes since last review" feature.

True, we try to avoid force-pushing. This is also mentioned in the devguide (Life of a Pull Request).

@freakboy3742
Copy link
Contributor Author

True, we try to avoid force-pushing. This is also mentioned in the devguide (Life of a Pull Request).

Apologies - I'll avoid force-pushes in future.

On the matter of process - have I misunderstood the instructions provided by Bevedere? I've left a comment as requested (originally as the last comment in my most recent response to Ned (since edited), then as a standalone comment), but Bevedere doesn't appear to have responded. Have I misunderstood what is needed here?

@AlexWaygood
Copy link
Member

Bevedere doesn't appear to have responded.

He's a little sick at the moment — see python/bedevere#599

@freakboy3742
Copy link
Contributor Author

Bevedere doesn't appear to have responded.

He's a little sick at the moment — see python/bedevere#599

Ah... get well soon, Bevedere :-)

@ned-deily
Copy link
Member

I'm reviewing the changes even without Bev's help.

@ned-deily
Copy link
Member

ned-deily commented Feb 10, 2024

My apologies, I didn't mean to push that merge to your branch but hopefully there should be no harm done: it just updates to the current HEAD. Feel free to force purge it!

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.

Thanks for the fixes and the additional testing. My use cases of interest now show no obvious regressions and other changes LGTM.

@freakboy3742
Copy link
Contributor Author

Just checking - Is there anything else I need to do on this PR? Is the merge an oversight, or is there something procedural that we're waiting on?

(Apologies for any impatience on my part - I definitely appreciate the volunteer efforts of the core team that are keeping the iOS work chugging along)

@erlend-aasland
Copy link
Contributor

Sorry; merge oversight!

@erlend-aasland erlend-aasland merged commit 2f07786 into python:main Feb 12, 2024
35 checks passed
@freakboy3742 freakboy3742 deleted the iOS-configure-reorg branch February 12, 2024 23:11
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
…cOS frameworks (python#115120)

Part of the PEP 730 work to add iOS support.

This change lays the groundwork for introducing iOS/tvOS/watchOS 
frameworks; it includes the structural refactoring needed so that iOS
branches can be added into in a subsequent PR.

Summary of changes:
* Updates config.sub to the 2024-01-01 release. This is the "as 
  released" version of config.sub.
* Adds a RESSRCDIR variable to allow sharing of macOS and iOS Makefile 
  steps.
* Adds an INSTALLTARGETS variable so platforms can customise which
  targets are actually installed. This will be used to exclude certain
  targets (e.g., binaries, manfiles) from iOS framework installs.
* Adds a PYTHONFRAMEWORKINSTALLNAMEPREFIX variable; this is used as
  the install name for the library. This is needed to allow for iOS
  frameworks to specify an @rpath-based install name.
* Evaluates MACHDEP earlier in the configure process so that
  ac_sys_system is available.
* Modifies _PYTHON_HOST_PLATFORM evaluation for cross-platform builds
  so that the CPU architecture is differentiated from the host
  identifier. This will be used to generate a _PYTHON_HOST_PLATFORM
  definition that includes ABI information, not just CPU architecture.
* Differentiates between SOABI_PLATFORM and PLATFORM_TRIPLET.
  SOABI_PLATFORM is used in binary module names, and includes the ABI,
  but not the OS or CPU architecture (e.g.,
  math.cpython-313-iphonesimulator.dylib). PLATFORM_TRIPLET is used
  as the sys._multiarch value, and on iOS will contains the ABI and
  architecture (e.g., iphoneos-arm64). This differentiation hasn't
  historically been needed because while macOS is a multiarch platform,
  it uses a bare darwin as PLATFORM_TRIPLE.
* Removes the use of the deprecated -Wl,-single_module flag when
  compiling macOS frameworks.
* Some whitespace normalisation where there was a mix of spaces and tabs 
  in a single block.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants