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

pyoxidizer 0.24.0 #136910

Merged
merged 2 commits into from
Sep 5, 2023
Merged

Conversation

chenrui333
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

previously

@chenrui333 chenrui333 added help wanted Task(s) needing PRs from the community or maintainers CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Jul 18, 2023
@github-actions github-actions bot added the rust Rust use is a significant feature of the PR or issue label Jul 18, 2023
@chenrui333 chenrui333 added the CI-requeued PR has been re-added to the queue label Sep 2, 2023
@chenrui333 chenrui333 mentioned this pull request Sep 2, 2023
@ZhongRuoyu ZhongRuoyu force-pushed the bump-pyoxidizer-0.24.0 branch 2 times, most recently from f33eb42 to 93dab29 Compare September 3, 2023 16:11
@ZhongRuoyu
Copy link
Member

ZhongRuoyu commented Sep 4, 2023

            Undefined symbols for architecture arm64:
              "_mkfifoat", referenced from:
                  _os_mkfifo_impl in libpyo3_ffi-fa31f950c7862ef8.rlib(posixmodule.o)
              "_mknodat", referenced from:
                  _os_mknod_impl in libpyo3_ffi-fa31f950c7862ef8.rlib(posixmodule.o)
            ld: symbol(s) not found for architecture arm64

The underlying problem is described in python/cpython#97897 -- mkfifoat and mknodat are unavailable before Ventura. pyoxidizer uses pre-built Python distributions from https://github.com/indygreg/python-build-standalone, where the PGO Python distributions for arm64 seem to be built on the repo owner's own M1 machine. And the 13.1 SDK was used:

$ curl -fsL https://github.com/indygreg/python-build-standalone/releases/download/20221220/cpython-3.10.9+20221220-aarch64-apple-darwin-pgo-full.tar.zst | tar --use-compress-program=`which unzstd` -x
$ head python/PYTHON.json                                          
{
    "apple_sdk_canonical_name": "macosx13.1",
    "apple_sdk_deployment_target": "11.0",
    "apple_sdk_platform": "macosx",
    "apple_sdk_version": "13.1",
    "build_info": {
        "core": {
            "links": [
                {
                    "name": "dl",
$ grep -E 'HAVE_MK(FIFOAT|NODAT)' python/PYTHON.json
        "HAVE_MKFIFOAT": "1",
        "HAVE_MKNODAT": "1",
$ nm python/install/lib/libpython3.10.dylib | grep -E 'mk(fifoat|nodat)' 
                 U _mkfifoat
                 U _mknodat
00000000002b1a28 t _probe_mkfifoat
00000000002b1a54 t _probe_mknodat

When you do so with an SDK where mkfifoat and mknodat are available, what you get are only warnings, even though you're targetting an older platform where they are unavailable:

$ cat test.c
#include <sys/types.h>
#include <sys/stat.h>
int main() {
  mkfifoat(42, "/dummy/argument", 0777);
  mknodat(42, "/dummy/argument", 0777, 42);
}
$ /usr/bin/cc test.c -target aarch64-apple-macos11
test.c:4:3: warning: 'mkfifoat' is only available on macOS 13.0 or newer [-Wunguarded-availability-new]
  mkfifoat(42, "/dummy/argument", 0777);
  ^~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/stat.h:394:9: note: 'mkfifoat' has been marked as being introduced in macOS 13.0 here, but the deployment target is macOS 11.0.0
int     mkfifoat(int, const char *, mode_t) __API_AVAILABLE(macos(13.0), ios(16.0), tvos(16.0), watchos(9.0));
        ^
test.c:4:3: note: enclose 'mkfifoat' in a __builtin_available check to silence this warning
  mkfifoat(42, "/dummy/argument", 0777);
  ^~~~~~~~
test.c:5:3: warning: 'mknodat' is only available on macOS 13.0 or newer [-Wunguarded-availability-new]
  mknodat(42, "/dummy/argument", 0777, 42);
  ^~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/stat.h:395:9: note: 'mknodat' has been marked as being introduced in macOS 13.0 here, but the deployment target is macOS 11.0.0
int     mknodat(int, const char *, mode_t, dev_t) __API_AVAILABLE(macos(13.0), ios(16.0), tvos(16.0), watchos(9.0));
        ^
test.c:5:3: note: enclose 'mknodat' in a __builtin_available check to silence this warning
  mknodat(42, "/dummy/argument", 0777, 42);
  ^~~~~~~
2 warnings generated.

So the symbols slipped into the Python binaries even though the target may not have it.

The pre-built x86_64 binaries, however, did not have this issue, because the 11.1 SDK was used:

$ curl -fsL https://github.com/indygreg/python-build-standalone/releases/download/20221220/cpython-3.10.9+20221220-x86_64-apple-darwin-lto-full.tar.zst | tar --use-compress-program=`which unzstd` -x
$ head python/PYTHON.json
{
    "apple_sdk_canonical_name": "macosx11.1",
    "apple_sdk_deployment_target": "10.9",
    "apple_sdk_platform": "macosx",
    "apple_sdk_version": "11.1",
    "build_info": {
        "core": {
            "links": [
                {
                    "name": "dl",
$ grep -E 'HAVE_MK(FIFOAT|NODAT)' python/PYTHON.json
        "HAVE_MKFIFOAT": "0",
        "HAVE_MKNODAT": "0",
$ nm python/install/lib/libpython3.10.dylib | grep -E 'mk(fifoat|nodat)'
[empty]

So, the test failure should be unrelated to pyoxidizer. And we can probably work around that with the default Python 3.9 distribution, which explicitly disables these two symbols; or non-PGO builds, which are cross-built for arm64 on x86_64 (using GitHub-hosted runners) using the 11.3 SDK.

indygreg/python-build-standalone#161 proposed to disable mkfifoat and mknodat unconditionally for macOS targets. When that's handled and released, we should ask upstream to update their default Python distributions.

@ZhongRuoyu ZhongRuoyu force-pushed the bump-pyoxidizer-0.24.0 branch 3 times, most recently from b76e8e8 to e6db617 Compare September 4, 2023 07:18
Co-authored-by: Ruoyu Zhong <[email protected]>
@ZhongRuoyu ZhongRuoyu added ready to merge PR can be merged once CI is green and removed help wanted Task(s) needing PRs from the community or maintainers CI-requeued PR has been re-added to the queue CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Sep 4, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

🤖 An automated task has requested bottles to be published to this PR.

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Sep 5, 2023
@BrewTestBot BrewTestBot added this pull request to the merge queue Sep 5, 2023
Merged via the queue into Homebrew:master with commit 08ef059 Sep 5, 2023
12 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2023
@chenrui333 chenrui333 deleted the bump-pyoxidizer-0.24.0 branch January 22, 2024 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. outdated PR was locked due to age ready to merge PR can be merged once CI is green rust Rust use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants