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

Fix macOS build failure by adding export ARCHFLAGS="-arch x86_64" #1168

Merged
merged 1 commit into from
Nov 1, 2020

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Oct 31, 2020

This is an attempt to fix macOS build with system default python.

See giampaolo/psutil#1832 for similar issues.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@yongtang yongtang force-pushed the macos-fix branch 2 times, most recently from 269ac8b to 9c73384 Compare October 31, 2020 20:41
@yongtang yongtang marked this pull request as ready for review October 31, 2020 22:21
Copy link
Member

@kvignesh1420 kvignesh1420 left a comment

Choose a reason for hiding this comment

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

@yongtang here are the things I have observed:

  • Since you added /usr/bin to $PATH (export PATH=/usr/bin:$PATH) the github action is using python 3.8.2. However, the one readily available is 3.8.6 in /usr/local/bin.
+ which python3
/usr/local/bin/python3
+ python3 --version
+ python3 -c 'import site; print(site.getsitepackages())'
Python 3.8.6

Based on #1167.

  • Also, I am not sure why the requirements.txt modification is required, as this PR: [github-action] fix macOS task #1167 passes without those changes. Is this because users have python3.7 as system default as of now? Also, this change might affect users trying to install the lint dependencies while using python3.8.

Please let me know if I am missing something here. Thanks.

See giampaolo/psutil#1832

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang changed the title Fix macOS build failure by skipping requirement.txt Fix macOS build failure by adding export ARCHFLAGS="-arch x86_64" Nov 1, 2020
@yongtang
Copy link
Member Author

yongtang commented Nov 1, 2020

@kvignesh1420 As mentioned in #1167 (comment) the system default python3 (comes with macOS itself, not with Python.org download, brew, or pyenv, etc) is 3.8.2.

In GitHub Actions machines, brew was used to install additional versions of python3. So that is why you see /usr/local/bin/python3 as 3.8.6 (through brew), and /usr/bin/python3 as 3.8.2 (macOS system default).

If your macOS machine only comes from a clean OS install (no brew), then you will notice there is only /usr/bin/python3 (no /usr/local/bin/python3 existence)

Since there are too many ways to install python3, our README.md is trying to stay with system default python (user does not need brew or download Python3 from python.org). If instead we support python3 with brew install, users may very well ask for additional support with other python installations as well (e.g., download from python.org). We want to avoid ending up testing too many scenarios.

Thus export PATH=/usr/bin:$PATH is needed to bring system default python back on GitHub Action's machines.

Now there is a small glitch on system default python 3.8.2: In order to make our lint work, bazel will try to install regex package which needs to compile C/C++ code.

However, as of today the latest clang with XCode will try to build with options of -arch arm64 -arch x86_64. Obviously this is Apple's pre-support for the upcoming Apple Silicon (ARM based processor) macBook that is expected to release mid-november. However, this will cause a bug similar to giampaolo/psutil#1832

So some additional fix to avoid installation of regex is needed. In the early attempt I added additional conditions in requirements.txt to skip the regex pip package.

After looking through giampaolo/psutil#1832 it looks like merely adding export ARCHFLAGS="-arch x86_64" will be enough.

Note this measure is only temporarily. I expect Apple's macOS 11.0 (Big Sur) will fully address the issue mentioned above. (I am guessing Big Sur will be released mid-november as well when Apple Silicon ARM macBook is released).

Please take a look at the updated PR and see if that helps.

Copy link
Member

@kvignesh1420 kvignesh1420 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 info @yongtang. It makes more sense now. I will close #1167 in favour of this.

@kvignesh1420 kvignesh1420 merged commit 96c9bb9 into tensorflow:master Nov 1, 2020
@yongtang yongtang deleted the macos-fix branch November 1, 2020 21:08
i-ony pushed a commit to i-ony/io that referenced this pull request Feb 8, 2021
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.

2 participants