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

Set icu4c path in CMAKE_PREFIX_PATH only when it's not there #691

Closed
wants to merge 5 commits into from
Closed

Set icu4c path in CMAKE_PREFIX_PATH only when it's not there #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b58dbb8
Set icu4c in CMAKE_PREFIX_PATH only when it's not there
mshang816 Dec 2, 2021
8575601
Fix a format error
mshang816 Dec 2, 2021
022bd1a
Update circle ci config to pick up local brew packages
mshang816 Dec 2, 2021
df25da5
Fix another format error
mshang816 Dec 2, 2021
5a0dc1b
Export PATH env variable properly for all commands
mshang816 Dec 2, 2021
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
7 changes: 5 additions & 2 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,12 @@ jobs:
command: |
set -xu
if [[ ! -e ~/deps ]]; then
export PATH=~/deps/bin:${PATH}
mkdir ~/deps ~/deps-src
curl -L https://github.com/Homebrew/brew/tarball/master | tar xz --strip 1 -C ~/deps
PATH=~/deps/bin:${PATH} DEPENDENCY_DIR=~/deps-src INSTALL_PREFIX=~/deps PROMPT_ALWAYS_RESPOND=n ./scripts/setup-macos.sh
DEPENDENCY_DIR=~/deps-src INSTALL_PREFIX=~/deps PROMPT_ALWAYS_RESPOND=n ./scripts/setup-macos.sh
# Calculate the prefix path before we delete brew's repos and taps.
echo "$(pwd)/deps;$(brew --prefix [email protected]);$(brew --prefix icu4c)" > ~/deps/PREFIX_PATH
rm -rf ~/deps/.git ~/deps/Library/Taps/ # Reduce cache size by 70%.
fi
- save_cache:
Expand All @@ -88,7 +91,7 @@ jobs:
mkdir -p .ccache
export CCACHE_DIR=$(pwd)/.ccache
ccache -sz -M 5Gi
cmake -B _build/debug -GNinja -DTREAT_WARNINGS_AS_ERRORS=1 -DENABLE_ALL_WARNINGS=1 -DCMAKE_BUILD_TYPE=Debug -DCMAKE_PREFIX_PATH=~/deps -DCMAKE_CXX_COMPILER_LAUNCHER=ccache
cmake -B _build/debug -GNinja -DTREAT_WARNINGS_AS_ERRORS=1 -DENABLE_ALL_WARNINGS=1 -DCMAKE_BUILD_TYPE=Debug -DCMAKE_PREFIX_PATH=$(cat ~/deps/PREFIX_PATH) -DCMAKE_CXX_COMPILER_LAUNCHER=ccache
ninja -C _build/debug
ccache -s
no_output_timeout: 1h
Expand Down
6 changes: 5 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,11 @@ find_package(ZLIB)
find_library(SNAPPY snappy)

if(CMAKE_SYSTEM_NAME MATCHES "Darwin")
set(CMAKE_PREFIX_PATH "/usr/local/opt/icu4c" ${CMAKE_PREFIX_PATH})
if(NOT CMAKE_PREFIX_PATH MATCHES ".*\/icu4c.*")
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should not add paths that were not created by the CMake script / part of the repo to the prefix path in our CMake script, but I can't find any discussion of best practices around this.

If you want to keep this default, at least append rather than prepend the path; that way it will not override developer-provided paths.

# icu4c is not found in CMAKE_PREFIX_PATH, defaults to
# "/usr/local/opt/icu4c" for MacOS
set(CMAKE_PREFIX_PATH "/usr/local/opt/icu4c" ${CMAKE_PREFIX_PATH})
endif()
find_package(ICU REQUIRED)
include_directories(${ICU_INCLUDE_DIRS})
link_directories("${ICU_INCLUDE_DIRS}/../lib")
Expand Down