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

Add information about lack of possibility of cross compile Poetry #1416

Merged
merged 11 commits into from
Jul 9, 2023

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented Feb 10, 2023

Based on discussion form #1415 I think that it is a good idea to add information about such a problem to the documentation.

docs/faq.md Outdated Show resolved Hide resolved
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
docs/faq.md Outdated

!!! note
Currently it is impossible to cross-compile project that uses Poetry as a build backend.
Reladed issue [here](https://github.com/python-poetry/poetry/issues/7107)

Choose a reason for hiding this comment

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

Hello Team, with regards to #1415 that caused the current PR, I would like to clarify that I can to cross-compile project that uses Poetry as a build backend. Perhaps it is my understanding but this documentation change is a little confusing (at least to me)

To elaborate, I can compile

  1. arm64 compatible wheel on MacOS (x86_64) runner
  2. arm64 compatible wheel on Linux (x86_64) runner

In case of#1, the compiled wheel filename just contained x86_64 in it, instead of arm64, which is why I raised the ticket. But, a Apple Silicon M1 mac client was successfully able to download, install and execute the wheel in another Poetry-based project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is put in the section about cross-compiling MacOS wheels. Cross-compilation of Linux ARM works differently than cross-compilation on MacOS.

And the wrong name of the produced wheel is, for me something that makes the building wheel not work.

If you have instructions that will allow you to build the wheel in a way that allows you to upload them directly from CI to PyPI without manual intervention, then this warning such be replaced.

At this moment, I do not have such a project and M1 machine that will allow validating such instruction.

Choose a reason for hiding this comment

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

Perhaps my comment intention was not clear and I could have articulated myself more clearly - I did not mean that the warning should not be added at all. Thank you for clarifying your thoughts and intention.

Copy link

@TeoZosa TeoZosa Feb 13, 2023

Choose a reason for hiding this comment

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

If you have instructions that will allow you to build the wheel in a way that allows you to upload them directly from CI to PyPI without manual intervention, then this warning such be replaced.

Chiming in since I maintain a project which came across this same issue.

What are we defining as "manual intervention"? Looking at #1415, it looks like @azagniotov included instructions to build the wheel which would allow them to be uploaded from CI to PyPI. Namely, by setting the CIBW_REPAIR_WHEEL_COMMAND_MACOS env var like so (which is exactly how my project resolves this issue as well):

...
    steps:
      - uses: actions/checkout@v3
      - name: Build wheels
        uses: pypa/cibuildwheel@v2.12.0
        env:
          CIBW_REPAIR_WHEEL_COMMAND_MACOS: >
            delocate-wheel --require-archs {delocate_archs} -w {dest_dir} -v {wheel} &&
            for file in {dest_dir}/*.whl ; do mv $file ${file//x86_64/arm64} ; done
          ...
          CIBW_ARCHS_MACOS: "arm64"
          ...
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solution is really bad as it skip whole delocate steep (as delocate will look fot x86 libs and dependencies)

Choose a reason for hiding this comment

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

@Czaki you raise a good point re To have the correct workflow you need to first rename the wheel file and then execute delocate on renamed file. <= thank you for your guidance and observation, I will look into this 👍 Much appreciated

Copy link

@azagniotov azagniotov Feb 14, 2023

Choose a reason for hiding this comment

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

@Czaki I followed your pointer and swapped the order of operations where:

  1. The wheel was renamed first
  2. The execute delocate command was on the renamed wheel
CIBW_REPAIR_WHEEL_COMMAND_MACOS: |
    echo "Target delocate archs: {delocate_archs}"

    ORIGINAL_WHEEL={wheel}

    echo "Running delocate-listdeps to list linked original wheel dependencies"
    delocate-listdeps --all $ORIGINAL_WHEEL

    if [ "${{ matrix.cibuildwheel_platform_id }}" == "macosx_arm64" ];
    then
      echo "Renaming .whl file when architecture is 'macosx_arm64'"
      RENAMED_WHEEL=${ORIGINAL_WHEEL//x86_64/arm64}

      echo "Wheel will be renamed to $RENAMED_WHEEL"
      mv $ORIGINAL_WHEEL $RENAMED_WHEEL
    else
      RENAMED_WHEEL=$ORIGINAL_WHEEL
    fi

    echo "Running delocate-wheel command on $RENAMED_WHEEL"
    delocate-wheel --require-archs {delocate_archs} -w {dest_dir} -v $RENAMED_WHEEL

    echo "Running delocate-listdeps to list linked wheel dependencies"
    WHEEL_SIMPLE_FILENAME="${RENAMED_WHEEL##*/}"
    delocate-listdeps --all {dest_dir}/$WHEEL_SIMPLE_FILENAME

    echo "DONE."

The output is as follows:

Repairing wheel...
  
Target delocate archs: arm64
Running delocate-listdeps to list linked original wheel dependencies
/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
/usr/lib/libSystem.B.dylib
/usr/lib/libobjc.A.dylib
Renaming .whl file when architecture is 'macosx_arm64'
Wheel will be renamed to /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-6h5tsf4q/cp310-macosx_arm64/built_wheel/my_project-0.0.1-cp310-cp310-macosx_12_0_arm64.whl
Running delocate-wheel command on /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-6h5tsf4q/cp310-macosx_arm64/built_wheel/my_project-0.0.1-cp310-cp310-macosx_12_0_arm64.whl
Fixing: /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/cibw-run-6h5tsf4q/cp310-macosx_arm64/built_wheel/my_project-0.0.1-cp310-cp310-macosx_12_0_arm64.whl
Running delocate-listdeps to list linked wheel dependencies
/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
/usr/lib/libSystem.B.dylib
/usr/lib/libobjc.A.dylib
DONE.

The repairing wheel step completed successfully also in this case, and the compiled wheel was successfully installed on Apple M1 and invoked at runtime.

Thoughts? 🙇🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.
For now I do not know if just link to this response or embed whole script in the documentation.

Choose a reason for hiding this comment

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

Thanks, @Czaki .

I personally lean towards a more defensive approach and I would just include a link to this discussion where appropriate as oppose to including my code snippet in the official docs.

But, that being said, I defer to you and your team regarding this. Please do what you think is best for the Poetry-based community and such a widely used library (cibuildwheel).

Thank you for your time on this, regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the description to be more correct. Did it look better?

docs/faq.md Outdated
Reladed issue [here](https://github.com/python-poetry/poetry/issues/7107).
Some packages can be built with arm64 wheels, but their file names will be incorrect,
with the platform tag showing `x86_64` instead of `arm64`.
As a workaround, the file can be renamed before running delocate to repair the wheel.
Copy link
Contributor

Choose a reason for hiding this comment

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

The internal tags will still be broken? You'd need pypa/wheel#422 I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this renaming suggestion and just say the tags have to be corrected. An example is in https://github.com/scikit-build/cmake-python-distributions/blob/master/scripts/repair_wheel.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now better?

docs/faq.md Outdated Show resolved Hide resolved
docs/faq.md Outdated Show resolved Hide resolved
Czaki and others added 2 commits February 15, 2023 00:00
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
horta added a commit to EBI-Metagenomics/deciphon-core that referenced this pull request Feb 23, 2023
@joerick
Copy link
Contributor

joerick commented Mar 11, 2023

Sorry for the delay on this @Czaki - I'm sorta hoping/waiting til we can get pypa/wheel#422 in so we can give some concrete advice on this, because the file rename advice is imperfect as an approach and has 'gotchas' that make it harder to explain. Hopefully that wheel feature will come soon!

@Czaki
Copy link
Contributor Author

Czaki commented Mar 11, 2023

Lets keep it open until pypa/wheel was released so people can find it if they cannot wait.

@henryiii
Copy link
Contributor

wheel 0.40 is out!

@Czaki
Copy link
Contributor Author

Czaki commented Mar 14, 2023

So could you prepare proper instructions? I'm not on time with wheel. Then I will close this PR.

@henryiii
Copy link
Contributor

If we want it to be done in the cibuildwheel steps (like repair wheel?), we should probably wait until it's in our pinned deps.

@anderssonjohan
Copy link

Just wanted to confirm that the wheel tags command seems to do the trick.

Context: We're building python 3.11 wheels of the pendulum package and noticed we got the x86_64 tag for macos arm64 wheels

When adding the opt-in flag to cross-compile for macos arm64 we got the AlreadyBuiltWheelError complaining about the duplicate x86_64 wheel file.

Solution:
Due to the extra steps for the wheel tags command I did a copy-paste and created a separate job to update the tags for the apple silicon wheel like this:
https://gist.github.com/anderssonjohan/49f07e33fc5cb2420515a8ac76dc0c95

@henryiii
Copy link
Contributor

It's in the pinned deps in the latest release, we can update docs now.

@joerick
Copy link
Contributor

joerick commented Jun 18, 2023

I've had a pass on this, including a general improvement to the whole section, comments welcome

@joerick joerick requested a review from henryiii July 8, 2023 19:47
Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

Technically, you can’t build universal natively, not sure what that would mean. I guess emulation for the Intel part then fusing? But that’s okay, not worth changing it (as it can be tested).

@joerick
Copy link
Contributor

joerick commented Jul 9, 2023

Yeah, good point. That statement has been in the docs for a while, but I'll have a think on how to reword that.

docs/faq.md Outdated Show resolved Hide resolved
@joerick joerick merged commit 7f6fe8a into pypa:main Jul 9, 2023
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.

6 participants