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

arrow: Fix cross-compiling errors on macos #2798

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 24 additions & 0 deletions cmake/toolchains/darwin-arm64.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
# ┃ ██████ ██████ ██████ █ █ █ █ █ █▄ ▀███ █ ┃
# ┃ ▄▄▄▄▄█ █▄▄▄▄▄ ▄▄▄▄▄█ ▀▀▀▀▀█▀▀▀▀▀ █ ▀▀▀▀▀█ ████████▌▐███ ███▄ ▀█ █ ▀▀▀▀▀ ┃
# ┃ █▀▀▀▀▀ █▀▀▀▀▀ █▀██▀▀ ▄▄▄▄▄ █ ▄▄▄▄▄█ ▄▄▄▄▄█ ████████▌▐███ █████▄ █ ▄▄▄▄▄ ┃
# ┃ █ ██████ █ ▀█▄ █ ██████ █ ███▌▐███ ███████▄ █ ┃
# ┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┫
# ┃ Copyright (c) 2017, the Perspective Authors. ┃
# ┃ ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ ┃
# ┃ This file is part of the Perspective library, distributed under the terms ┃
# ┃ of the [Apache License 2.0](https://www.apache.org/licenses/LICENSE-2.0). ┃
# ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

# Minimal toolchain file to ensure that Arrow's bundled dependency builds use
# the correct architecture for Mac builds.

# (Arrow does not as of Oct 2024 pass on CMAKE_OSX_ARCHITECTURES to its
# dependencies if it is set, but does pass on a toolchain file)

set(CMAKE_OSX_ARCHITECTURES "arm64" CACHE STRING "Target architecture for Mac builds")
Copy link
Contributor Author

@tomjakubowski tomjakubowski Oct 22, 2024

Choose a reason for hiding this comment

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

don't think this needs the CACHE etc. If it's confusing I'm happy to try removing it

# Arrow uses this to choose a `-march` flag
set(CMAKE_SYSTEM_PROCESSOR "arm64")
# Prevent cmake from overwriting CMAKE_SYSTEM_PROCESSOR
# https://github.com/apache/arrow/issues/44448#issuecomment-2418649378
set(CMAKE_SYSTEM_NAME "Darwin")
24 changes: 24 additions & 0 deletions cmake/toolchains/darwin-x86_64.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
# ┃ ██████ ██████ ██████ █ █ █ █ █ █▄ ▀███ █ ┃
# ┃ ▄▄▄▄▄█ █▄▄▄▄▄ ▄▄▄▄▄█ ▀▀▀▀▀█▀▀▀▀▀ █ ▀▀▀▀▀█ ████████▌▐███ ███▄ ▀█ █ ▀▀▀▀▀ ┃
# ┃ █▀▀▀▀▀ █▀▀▀▀▀ █▀██▀▀ ▄▄▄▄▄ █ ▄▄▄▄▄█ ▄▄▄▄▄█ ████████▌▐███ █████▄ █ ▄▄▄▄▄ ┃
# ┃ █ ██████ █ ▀█▄ █ ██████ █ ███▌▐███ ███████▄ █ ┃
# ┣━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┫
# ┃ Copyright (c) 2017, the Perspective Authors. ┃
# ┃ ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ ┃
# ┃ This file is part of the Perspective library, distributed under the terms ┃
# ┃ of the [Apache License 2.0](https://www.apache.org/licenses/LICENSE-2.0). ┃
# ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

# Minimal toolchain file to ensure that Arrow's bundled dependency builds use
# the correct architecture for Mac builds.

# (Arrow does not as of Oct 2024 pass on CMAKE_OSX_ARCHITECTURES to its
# dependencies if it is set, but does pass on a toolchain file)

set(CMAKE_OSX_ARCHITECTURES "x86_64" CACHE STRING "Target architecture for Mac builds")
# Arrow uses this to choose a `-march` flag
set(CMAKE_SYSTEM_PROCESSOR "x86_64")
# Prevent cmake from overwriting CMAKE_SYSTEM_PROCESSOR
# https://github.com/apache/arrow/issues/44448#issuecomment-2418649378
set(CMAKE_SYSTEM_NAME "Darwin")
22 changes: 18 additions & 4 deletions rust/perspective-server/build/psp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,24 @@ pub fn cmake_build() -> Result<Option<PathBuf>, std::io::Error> {
let profile = std::env::var("PROFILE").unwrap();
dst.always_configure(true);
dst.define("CMAKE_BUILD_TYPE", profile.as_str());
if std::env::var("PSP_ARCH").as_deref() == Ok("x86_64") {
dst.define("CMAKE_OSX_ARCHITECTURES", "x86_64");
} else if std::env::var("PSP_ARCH").as_deref() == Ok("arm64") {
dst.define("CMAKE_OSX_ARCHITECTURES", "arm64");

if cfg!(target_os = "macos") {
// Set CMAKE_OSX_ARCHITECTURES et al. for Mac builds. Arrow does not forward on
// CMAKE_OSX_ARCHITECTURES and but it does forward on a
// CMAKE_TOOLCHAIN_FILE. In Conda builds, the environment sets
// `CMAKE_ARGS` up with various toolchain arguments. This block may need
// to be patched out or adjusted for Conda.
Copy link
Contributor Author

@tomjakubowski tomjakubowski Oct 22, 2024

Choose a reason for hiding this comment

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

by this I mean this block may eventually need some if !conda_build style guards

Copy link
Contributor Author

@tomjakubowski tomjakubowski Oct 22, 2024

Choose a reason for hiding this comment

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

funny enough, the conda 3.1.1 macos cross-compiling builds were apparently unaffected -- their import tests succeeded. https://github.com/conda-forge/perspective-feedstock/runs/31853180964

I'm not exactly sure why they were unaffected, but have some leads. conda-build passes its toolchain arguments to us (in CMAKE_ARGS) with compilers/tools with arm64 in the name, -DCMAKE_C_COMPILER=arm64-…-cc -DCMAKE_LIBTOOL=arm64-…-cc, which I presume is a compiler toolchain which targets arm64 without needing any other flags, and doesn't set CMAKE_OSX_ARCHITECTURE itself. And Arrow does forward those specific flags on to its external dependencies

let toolchain_file = match std::env::var("PSP_ARCH").as_deref() {
Ok("x86_64") => "./cmake/toolchains/darwin-x86_64.cmake",
Ok("aarch64") => "./cmake/toolchains/darwin-arm64.cmake",
arch @ Ok(_) | arch @ Err(_) => {
panic!("Unknown PSP_ARCH value: {:?}", arch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this might be the wrong behavior if PSP_ARCH isn't set (which I think is the case for local dev). I guess it should just not set a toolchain file when PSP_ARCH is unset? (but perhaps still error + bail out when PSP_ARCH is an unknown value)

},
};
dst.define(
"CMAKE_TOOLCHAIN_FILE",
std::fs::canonicalize(toolchain_file).expect("Failed to canonicalize toolchain file."),
);
}

if std::env::var("TARGET")
Expand Down
Loading