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.

Sign up for GitHub

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

Jump to bottom

GH-36685: [R][C++] Fix illegal opcode failure with Homebrew #36705

Merged
merged 43 commits into from
Jul 30, 2023
Merged

GH-36685: [R][C++] Fix illegal opcode failure with Homebrew #36705

Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
49e11e2
WIP: GH-36685: [R][C++] Fix illegal opcode failure with Homebrew
kou Jul 15, 2023
5610c3b
Use llvm
kou Jul 16, 2023
e267914
Revert more
kou Jul 16, 2023
175889c
Revert more
kou Jul 16, 2023
4deb1b4
Disable HOMEBREW_OPTIMIZATION_LEVEL
kou Jul 16, 2023
9b26f23
Try O2
kou Jul 16, 2023
cf2a507
Try
kou Jul 17, 2023
090340e
Use env
kou Jul 17, 2023
3f0c16b
Enable prefer config
kou Jul 18, 2023
90940a2
libc++
kou Jul 18, 2023
da8d095
Disable mimalloc
kou Jul 18, 2023
f949167
Use openssl 1.1
kou Jul 18, 2023
eb89238
Disable bzip2
kou Jul 18, 2023
a11408d
Update sed
kou Jul 18, 2023
0be0e88
Disable SIMD explicitly
kou Jul 19, 2023
d17ca72
Debug
kou Jul 20, 2023
d7865d4
Untabify
kou Jul 20, 2023
e1f7194
Use local formula
kou Jul 20, 2023
5b99278
Escape
kou Jul 20, 2023
c8be627
Restore run:
kou Jul 20, 2023
36d7248
Revert
kou Jul 20, 2023
e343cf4
Revert SIMD
kou Jul 20, 2023
958c820
Disable O2
kou Jul 21, 2023
fe911ea
Disable runtime SIMD dispatch
kou Jul 24, 2023
6e8e7a9
Save logs
kou Jul 25, 2023
4e0d1c3
Comment
kou Jul 25, 2023
e8be476
Debug
kou Jul 25, 2023
4e44372
Fix path
kou Jul 25, 2023
2e980e6
Log
kou Jul 26, 2023
4181e9f
Remove a needless code
kou Jul 26, 2023
03dc53a
Compile cpu_info.cc with SIMD flags
kou Jul 26, 2023
8391b17
Fix a typo
kou Jul 26, 2023
04fa789
Disable ENV.runtime_cpu_detection
kou Jul 26, 2023
0af2bb2
Show system information
kou Jul 26, 2023
1796028
Show CPU info
kou Jul 26, 2023
8852d71
Use AVX512 flags
kou Jul 26, 2023
e9d121e
Use RUNTIME
kou Jul 26, 2023
a363fc7
Use PARQUET_BMI2
kou Jul 26, 2023
97c5f69
Accept ARROW_HAVE_RUNTIME_BMI2
kou Jul 26, 2023
cc430b7
Remove runtime_cpu_detection
kou Jul 26, 2023
d625270
Revert
kou Jul 26, 2023
695b7b4
Disable runtime SIMD dispatch
kou Jul 26, 2023
7e688a0
Fix a typo
kou Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion dev/tasks/homebrew-formulae/apache-arrow.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ class ApacheArrow < Formula
fails_with gcc: "5"

def install
# https://github.com/Homebrew/homebrew-core/issues/76537
# This isn't for https://github.com/Homebrew/homebrew-core/issues/76537 .
Copy link
Member

Choose a reason for hiding this comment

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

Is deleting the old comment enough? Is there a reason a future reader would think this is related to 76537?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we defer this to another PR?
To be honest, I want to remove ENV.runtime_cpu_detection if Hardware::CPU.intel? entirely or remove if Hardware::CPU.intel?. If I do the change, we can remove this.
But I want to keep this in this PR to clarify we choose "1. -DARROW_RUNTIME_SIMD_LEVEL=NONE instead of "2. Remove ENV.runtime_cpu_detection if Hardware::CPU.intel?".

# This may improve performance.
ENV.runtime_cpu_detection if Hardware::CPU.intel?

# link against system libc++ instead of llvm provided libc++
Expand Down Expand Up @@ -90,6 +91,11 @@ def install
-DARROW_WITH_ZSTD=ON
-DPARQUET_BUILD_EXECUTABLES=ON
]
# Disable runtime SIMD dispatch. It may cause "illegal opcode"
# error on Intel Mac because of one-definition-rule violation.
#
# https://github.com/apache/arrow/issues/36685
args << "-DARROW_RUNTIME_SIMD_LEVEL=OFF" if OS.mac? and Hardware::CPU.intel?

system "cmake", "-S", "cpp", "-B", "build", *args, *std_cmake_args
system "cmake", "--build", "build"
Expand Down
4 changes: 3 additions & 1 deletion dev/tasks/macros.jinja
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ on:
# see https://github.com/actions/runner-images/issues/6868
brew install --overwrite python@3.11 python@3.10

set -x
ARROW_GLIB_FORMULA=$(echo ${ARROW_FORMULA} | sed -e 's/\.rb/-glib.rb/')
echo "ARROW_GLIB_FORMULA=${ARROW_GLIB_FORMULA}" >> ${GITHUB_ENV}
for formula in ${ARROW_FORMULA} ${ARROW_GLIB_FORMULA}; do
Expand All @@ -223,11 +224,12 @@ on:
# Pin the current commit in the formula to test so that
# we're not always pulling from the tip of the default branch
sed -i '' -E \
-e 's@https://github.com/apache/arrow.git"$@{{ arrow.remote }}.git", revision: "{{ arrow.head }}"@' \
-e 's@https://github.com/apache/arrow.git", branch: "main"$@{{ arrow.remote }}.git", revision: "{{ arrow.head }}"@' \
${formula}
# Sometimes crossbow gives a remote URL with .git and sometimes not.
# Make sure there's only one
sed -i '' -E -e 's@.git.git@.git@' ${formula}
cat ${formula}
cp ${formula} $(brew --repository homebrew/core)/Formula/
done
{% endmacro %}
Expand Down
20 changes: 16 additions & 4 deletions dev/tasks/r/github.macos.brew.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,33 @@ jobs:
name: "Homebrew + R package"
runs-on: macOS-11
steps:
- name: Show system information
run: |
sysctl hw.optional machdep.cpu

{{ macros.github_checkout_arrow()|indent }}

{{ macros.configure_homebrew_arrow(formula)|indent }}
- name: Install apache-arrow
env:
{{ macros.github_set_sccache_envvars()|indent(8)}}
{{ macros.github_set_sccache_envvars()|indent(8)}}
run: |

brew install sccache
# for testing
brew install minio

# TODO(ARROW-16907): apache/arrow@main seems to be installed already
# so this does nothing on a branch/PR
brew install -v --HEAD apache-arrow
brew install -v --HEAD {{ '$(brew --repository homebrew/core)/Formula/apache-arrow.rb' }}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to specify absolute path for a formula to use the local modified version. (We didn't need this with old Homebrew.)
I think that it's related to the "Using JSON files downloaded from formulae.brew.sh for package installation rather than local homebrew/core and homebrew/cask taps." change in Homebrew 4.0.0: https://brew.sh/2023/02/16/homebrew-4.0.0/

We may be able to use HOMEBREW_NO_INSTALL_FROM_API=1 instead of specifying an absolute path. But I think that the absolute path approach is straightforward.


mkdir -p homebrew-logs
cp -a ~/Library/Logs/Homebrew/apache-arrow homebrew-logs/
- name: Save logs
if: always()
uses: actions/upload-artifact@v2
with:
name: homebrew-logs
path: homebrew-logs

- uses: r-lib/actions/setup-r@v2
- name: Install dependencies
Expand Down