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

Disable coreutils on M1 Apple Silicon with arm64 #2020

Merged
merged 5 commits into from
Sep 9, 2021

Conversation

yackrru
Copy link
Contributor

@yackrru yackrru commented Aug 1, 2021

Make sure you have checked all steps below.

Prerequisite

  • Please consider implementing the feature as a hook script or plugin as a first step.
    • pyenv has some powerful support for plugins and hook scripts. Please refer to Authoring plugins for details and try to implement it as a plugin if possible.
  • Please consider contributing the patch upstream to rbenv, since we have borrowed most of the code from that project.
    • We occasionally import the changes from rbenv. In general, you can expect changes made in rbenv will be imported to pyenv too, eventually.
    • Generally speaking, we prefer not to make changes in the core in order to keep compatibility with rbenv.
    • [Author comment] I don't patch to rbenv.
  • My PR addresses the following pyenv issue (if any)

Description

  • Commands of coreutils installed with homebrew can negatively affect installation, which pyenv install on M1 Apple Silicon with arm64.
  • Therefore, the patch temporarily excludes coreutils bin from PATH so that pyenv install does not reference the commands of coreutils.
    • coreutils
      • The path of coreutils bin, which is $(brew --prefix coreutils)/libexec/gnubin, overrides native path of commands included in coreutils.
      • The overriding allows direct access to commands without using g*.
  • I don't think this patch satisfies best practices of high maintainability codes because blacklist method lacks versatility.
    • For the time being, I propose a patch as one of the solutions.

Problem

  • Platform information (e.g. Ubuntu Linux 16.04): macOS Big Sur 11.4
  • OS architecture (e.g. amd64): arm64
  • pyenv version: 2.0.4-1-gb005dcc7
  • Python version: 3.9.6
  • C Compiler information (e.g. gcc 7.3): I have tried the following two.
    • Apple clang version 12.0.5
    • Homebrew GCC 11.2.0
  • Please attach verbose build log as gist: https://gist.github.com/ttksm/f287528cb664cc4bd8e1bbe2ea5d6dfe
    • You can turn on verbose debug logging using by setting PYENV_DEBUG=1, e.g. env PYENV_DEBUG=1 pyenv install -v 3.6.4

Tests

  • My PR adds the following unit tests (if any)

@native-api
Copy link
Member

Could you please showcase the problem that this is intended to fix -- with diagnostic info as per the issue template?

@yackrru
Copy link
Contributor Author

yackrru commented Aug 2, 2021

@native-api
Thank you for your comment.
I've added information such as the environment in which the error occurred.
Please take a look when you get a chance.

@native-api
Copy link
Member

native-api commented Aug 3, 2021

Thank you.

I see that the problem is in ./config.guess outputting a result that Python's ./configure is not able to work with.
I don't have Apple M1 hardware, so could you run the following for me?

  • The output of /usr/bin/uname -a, guname -a
  • The output of PS4='+(${BASH_SOURCE}:${LINENO}): ${FUNCNAME[0]:+${FUNCNAME[0]}(): }' sh -x config.guess in the Python build tree (it should be somewhere under your $TEMP after a failed build; if not, download and unpack the release tarball yourself and run the command there).

I also see that the release 3.9.3 has been recalled due to having introduced an unexpected incompatibility so we should probably remove it, too.

  • Could you check if all other releases that officially support Apple Silicon -- 3.9.1+ and 3.8.10+ -- show the same problem?

@yackrru
Copy link
Contributor Author

yackrru commented Aug 3, 2021

Thank you.

The output of /usr/bin/uname -a, guname -a

The results are as follows.

  • /usr/bin/uname -a
Darwin BLMC009.local 20.6.0 Darwin Kernel Version 20.6.0: Wed Jun 23 00:26:27 PDT 2021; root:xnu-7195.141.2~5/RELEASE_ARM64_T8101 arm64
  • guname -a
Darwin BLMC009.local 20.6.0 Darwin Kernel Version 20.6.0: Wed Jun 23 00:26:27 PDT 2021; root:xnu-7195.141.2~5/RELEASE_ARM64_T8101 arm64 arm64 MacBookPro17,1 Darwin

The output of PS4='+(${BASH_SOURCE}:${LINENO}): ${FUNCNAME[0]:+${FUNCNAME[0]}(): }' sh -x config.guess in the Python build tree

I uploaded log to my gist.

Could you check if all other releases that officially support Apple Silicon -- 3.9.1+ and 3.8.10+ -- show the same problem?

I tried to install the following versions but it failed.
3.8.10, 3.8.11, 3.9.1, 3.9.2, 3.9.4, 3.9.5 and 3.9.6.

@native-api
Copy link
Member

Okay. The core reason is an old config.sub (a stock file inserted by autoconf) that doesn't support arm64. The issue has been reported in https://bugs.python.org/issue43878 and fixed in 3.10 only.

So we can selectively apply this workaroud to 3.9.1+ and 3.8.10+ only.

@native-api native-api marked this pull request as ready for review September 7, 2021 20:37
Copy link
Member

@native-api native-api left a comment

Choose a reason for hiding this comment

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

Could you change the logic to only selectively apply to the affected versions in the affected scenarios?
E.g. call this fuction not from general logic but from specific formulae.

@native-api
Copy link
Member

Another option is to instead patch the config.sub for affected versions to a newer version of it.
The patch will need to be duplicated for each affected formula though.

@yackrru
Copy link
Contributor Author

yackrru commented Sep 7, 2021

Thanks for your investigation and advices.
I changed the logic to only selectively apply to the affected versions (3.8.10+ and 3.9.1+ ~ 3.9.6).
Please re-review.

Removed redundancies, correctly handle paths with spaces
@native-api
Copy link
Member

I've made some refinements. Please check that all the changed scripts work on an Apple M1.

@yackrru
Copy link
Contributor Author

yackrru commented Sep 9, 2021

Thanks for your refinements.
I've checked that scripts work fine with revision yackrru@ff97209.

@native-api native-api merged commit 90d0d20 into pyenv:master Sep 9, 2021
@yackrru yackrru deleted the disable-coreutils-applesilicon branch September 9, 2021 13:15
native-api added a commit to native-api/pyenv that referenced this pull request Jan 8, 2022
native-api added a commit that referenced this pull request Jan 8, 2022
)

This reverts commit 90d0d20.

After further consideration, we've decided to remove this workaround:
* It only has an effect if the user has added `gnubin` from Homebrew Coreutils to PATH which is an unsupported setup
* It was intended to be applied only to a few select 3.8 and 3.9 versions that officially support Apple Silicon and only fail with Homebrew Coreutils in PATH because they have `config.*` from a too old version of Autoconf that doesn't support the Arm64 arch -- but
  * CPython devs [didn't actually fix the problem in 3.10, either, only in 3.11](#2157 (comment)), so we'd need to apply it to all 3.10 releases, too
  * users started pushing this workaround into other unrelated branches because they were using the above unsupported setup. See #2190 (review) for discussion.
@yackrru yackrru restored the disable-coreutils-applesilicon branch April 7, 2022 02:42
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

2 participants