Install FMT and download ccache on MacOS #10933
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
majetideepak
force-pushed
the
fix-fmt-ci
branch
from
3043891
to
19cc4d0
Compare
czentgr
reviewed
Sep 5, 2024
| @@ -35,7 +35,7 @@ PYTHON_VENV=${PYHTON_VENV:-"${SCRIPTDIR}/../.venv"} | |||
| NPROC=$(getconf _NPROCESSORS_ONLN) | |||
|
|
|||
| DEPENDENCY_DIR=${DEPENDENCY_DIR:-$(pwd)} | |||
| MACOS_VELOX_DEPS="bison boost double-conversion flex fmt gflags glog googletest icu4c libevent libsodium lz4 lzo openssl protobuf@21 simdjson snappy thrift xz xsimd zstd" | |||
| MACOS_VELOX_DEPS="bison boost double-conversion flex gflags glog googletest icu4c libevent libsodium lz4 lzo openssl protobuf@21 simdjson snappy thrift xz xsimd zstd" | |||
There was a problem hiding this comment.
This will not be enough. Problem is that line 39 installs ccache. This brings in fmt11 as well. And since we have the ordering issues will still use fmt11 regardless of whether or not it is installed in the CI script.
There was a problem hiding this comment.
good point! Let's remove ccache for now and add it back in #10920
assignUser
reviewed
Sep 5, 2024
.github/workflows/macos.yml
Outdated
| @@ -93,12 +93,6 @@ jobs: | |||
| - name: Build | |||
| run: | | |||
| cmake --build _build/$BUILD_TYPE -j $NJOBS | |||
| ccache -s | |||
|
|
|||
| - uses: assignUser/stash/save@v1 | |||
There was a problem hiding this comment.
just do if: false and also add it to the restore step
assignUser
approved these changes
Sep 5, 2024
majetideepak
force-pushed
the
fix-fmt-ci
branch
2 times, most recently
from
5c9f8dc
to
4fe1473
Compare
|
@assignUser thanks for the feedback! |
majetideepak
force-pushed
the
fix-fmt-ci
branch
from
4fe1473
to
d4d75a3
Compare
czentgr
approved these changes
Sep 5, 2024
Why the bot will squash them anyway before pushing? |
majetideepak
force-pushed
the
fix-fmt-ci
branch
from
d4d75a3
to
33da18b
Compare
majetideepak
changed the title
|
@czentgr, @assignUser suggested we could download ccache instead. I implemented this. |
kgpai
approved these changes
Sep 5, 2024
|
@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
czentgr
reviewed
Sep 5, 2024
| @@ -77,6 +77,9 @@ function install_build_prerequisites { | |||
| python3 -m venv ${PYTHON_VENV} | |||
| fi | |||
| source ${PYTHON_VENV}/bin/activate; pip3 install cmake-format regex pyyaml | |||
| wget -O ccache.tar.gz https://github.com/ccache/ccache/releases/download/v4.10.2/ccache-4.10.2-darwin.tar.gz | |||
| tar -xf ccache.tar.gz | |||
| mv ccache-4.10.2-darwin/ccache /usr/local/bin/ | |||
There was a problem hiding this comment.
Mmhh, this might be restricted. For the same reason we want INSTALL_PREFIX on macOS.
There was a problem hiding this comment.
Should be okay. We install minio here as well.
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
yingsu00
added a commit
to yingsu00/velox
that referenced
this pull request
Sep 13, 2024
In "Install FMT and download ccache on MacOS (facebookincubator#10933)", wget was used to download ccache in setup-macos.sh. However, wget is not pre-installed on MacOs by default, and the following error was thrown: + wget -O ccache.tar.gz https://github.com/ccache/ccache/releases/download/v4.10.2/ccache-4.10.2-darwin.tar.gz ../scripts/setup-macos.sh: line 80: wget: command not found + tar -xf ccache.tar.gz tar: Error opening archive: Failed to open 'ccache.tar.gz' This commit changes wget to curl, which is guaranteed to be pre-installed on MacOS.
yingsu00
added a commit
to yingsu00/velox
that referenced
this pull request
Sep 13, 2024
In "Install FMT and download ccache on MacOS (facebookincubator#10933)", wget was used to download ccache in setup-macos.sh. However, wget is not pre-installed on MacOs by default, and the following error was thrown: + wget -O ccache.tar.gz https://github.com/ccache/ccache/releases/download/v4.10.2/ccache-4.10.2-darwin.tar.gz ../scripts/setup-macos.sh: line 80: wget: command not found + tar -xf ccache.tar.gz tar: Error opening archive: Failed to open 'ccache.tar.gz' This commit changes wget to curl, which is guaranteed to be pre-installed on MacOS.
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 13, 2024
Summary: In "Install FMT and download ccache on MacOS (#10933)", wget was used to download ccache in setup-macos.sh. However, wget is not pre-installed on MacOs by default, and the following error was thrown: ``` + wget -O ccache.tar.gz https://github.com/ccache/ccache/releases/download/v4.10.2/ccache-4.10.2-darwin.tar.gz ../scripts/setup-macos.sh: line 80: wget: command not found + tar -xf ccache.tar.gz tar: Error opening archive: Failed to open 'ccache.tar.gz' ``` This commit changes wget to curl, which is guaranteed to be pre-installed on MacOS. Pull Request resolved: #10995 Reviewed By: pedroerp Differential Revision: D62646279 Pulled By: kevinwilfong fbshipit-source-id: aaa2365ae013bb012f25556d4741e3a08ea7ef82
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Brew fmt11 is not supported.
ccache brings in fmt11 from brew.
The brew fmt11 headers are taking precedence over bundled headers.
Bundle fmt and download prebuilt ccache that is statically linked.
We will fix the header include issue in #10920.
Resolves #10936