From 9d6d9a253b462a6be6b5faa38ddb7d4f26238d4a Mon Sep 17 00:00:00 2001 From: Benjamin Winger Date: Tue, 20 Jun 2023 12:18:43 -0400 Subject: [PATCH] Get the rust api building on windows Currently building in release mode works fine, but debug mode requires overriding CFLAGS and CXXFLAGS to include /MDd, which is done in the rusttest makefile target. Also made the number of threads passed to make in build.rs configurable --- .github/workflows/ci-workflow.yml | 7 ++ Makefile | 15 +++- tools/rust_api/Cargo.toml | 2 + tools/rust_api/build.rs | 113 ++++++++++++++++++++++++----- tools/rust_api/src/database.rs | 19 ++++- tools/rust_api/src/query_result.rs | 7 +- 6 files changed, 138 insertions(+), 25 deletions(-) diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml index bbfd1a360f..ed59939e12 100644 --- a/.github/workflows/ci-workflow.yml +++ b/.github/workflows/ci-workflow.yml @@ -122,6 +122,13 @@ jobs: call "C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Auxiliary\Build\vcvars64.bat" make test NUM_THREADS=18 + - name: Rust test + shell: cmd + run: | + call "C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Auxiliary\Build\vcvars64.bat" + set OPENSSL_DIR=C:\Program Files\OpenSSL-Win64 + make rusttest NUM_THREADS=18 + clang-formatting-check: name: clang-format check runs-on: kuzu-self-hosted-testing diff --git a/Makefile b/Makefile index 9c385e1147..be75b7ec57 100644 --- a/Makefile +++ b/Makefile @@ -53,12 +53,23 @@ arrow: cmake $(FORCE_COLOR) $(SANITIZER_FLAG) $(GENERATOR) -DCMAKE_BUILD_TYPE=Release .. && \ cmake --build . --config Release -- -j $(NUM_THREADS) +arrow-debug: +ifeq ($(OS),Windows_NT) + $(call mkdirp,external/build) && cd external/build && \ + cmake $(FORCE_COLOR) $(SANITIZER_FLAG) $(GENERATOR) -DCMAKE_BUILD_TYPE=Debug .. && \ + cmake --build . --config Debug -- -j $(NUM_THREADS) +else + $(call mkdirp,external/build) && cd external/build && \ + cmake $(FORCE_COLOR) $(SANITIZER_FLAG) $(GENERATOR) -DCMAKE_BUILD_TYPE=Release .. && \ + cmake --build . --config Release -- -j $(NUM_THREADS) +endif + release: arrow $(call mkdirp,build/release) && cd build/release && \ cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Release ../.. && \ cmake --build . --config Release -- -j $(NUM_THREADS) -debug: arrow +debug: arrow-debug $(call mkdirp,build/debug) && cd build/debug && \ cmake $(GENERATOR) $(FORCE_COLOR) $(SANITIZER_FLAG) -DCMAKE_BUILD_TYPE=Debug ../.. && \ cmake --build . --config Debug -- -j $(NUM_THREADS) @@ -111,6 +122,8 @@ rusttest: ifeq ($(OS),Windows_NT) cd $(ROOT_DIR)/tools/rust_api && \ set KUZU_TESTING=1 && \ + set CFLAGS=/MDd && \ + set CXXFLAGS=/MDd /std:c++20 && \ cargo test -- --test-threads=1 else cd $(ROOT_DIR)/tools/rust_api && \ diff --git a/tools/rust_api/Cargo.toml b/tools/rust_api/Cargo.toml index 9d54f219af..924dd0241e 100644 --- a/tools/rust_api/Cargo.toml +++ b/tools/rust_api/Cargo.toml @@ -22,6 +22,8 @@ time = "0.3" [build-dependencies] cxx-build = "1.0" num_cpus = "1.0" +[target.'cfg(windows)'.build-dependencies] +which = "4" [dev-dependencies] tempdir = "0.3" diff --git a/tools/rust_api/build.rs b/tools/rust_api/build.rs index 268a9e5b7c..6e675aa8c0 100644 --- a/tools/rust_api/build.rs +++ b/tools/rust_api/build.rs @@ -9,18 +9,44 @@ fn link_mode() -> &'static str { } } -fn main() -> Result<(), Box> { +fn main() { // There is a kuzu-src symlink pointing to the root of the repo since Cargo // only looks at the files within the rust project when packaging crates. // Using a symlink the library can both be built in-source and from a crate. - let kuzu_root = Path::new(&std::env::var("CARGO_MANIFEST_DIR").unwrap()).join("kuzu-src"); - let target = env::var("PROFILE")?; + let kuzu_root = { + let root = Path::new(&std::env::var("CARGO_MANIFEST_DIR").unwrap()).join("kuzu-src"); + if root.is_dir() { + root + } else { + // If the path is not directory, this is probably an in-source build on windows where the + // symlink is unreadable. + Path::new(&std::env::var("CARGO_MANIFEST_DIR").unwrap()).join("../..") + } + }; + // Windows fails to link on windows unless CFLAGS and CXXFLAGS are overridden + // If they are, assume the user knows what they are doing. Otherwise just link against the + // release version of kuzu even in debug mode. + let target = if cfg!(windows) && std::env::var("CXXFLAGS").is_err() { + "release".to_string() + } else { + env::var("PROFILE").unwrap() + }; let kuzu_cmake_root = kuzu_root.join(format!("build/{target}")); let mut command = std::process::Command::new("make"); + let threads = env::var("NUM_THREADS") + .map(|x| x.parse::()) + .unwrap_or_else(|_| Ok(num_cpus::get())) + .unwrap(); command - .args(&[target, format!("NUM_THREADS={}", num_cpus::get())]) + .args(&[&target, &format!("NUM_THREADS={}", threads)]) .current_dir(&kuzu_root); - let make_status = command.status()?; + let make_status = command.status().unwrap_or_else(|_| { + panic!( + "Running make {} on {}/Makefile failed!", + &target, + &kuzu_root.display() + ) + }); assert!(make_status.success()); let kuzu_lib_path = kuzu_cmake_root.join("src"); @@ -58,20 +84,67 @@ fn main() -> Result<(), Box> { println!("cargo:rustc-link-lib={}=kuzu", link_mode()); if link_mode() == "static" { - println!("cargo:rustc-link-lib=dylib=stdc++"); + if cfg!(windows) { + if target == "debug" { + println!("cargo:rustc-link-lib=dylib=msvcrtd"); + } else { + println!("cargo:rustc-link-lib=dylib=msvcrt"); + } + println!("cargo:rustc-link-lib=dylib=shell32"); + println!("cargo:rustc-link-lib=dylib=ole32"); + } else { + println!("cargo:rustc-link-lib=dylib=stdc++"); + } println!("cargo:rustc-link-lib=static=arrow_bundled_dependencies"); - // Dependencies of arrow's bundled dependencies + // arrow's bundled dependencies link against openssl when it's on the system, whether + // requested or not. // Only seems to be necessary when building tests. - // This will probably not work on windows/macOS - // openssl-sys has better cross-platform logic, but just using that doesn't work. if env::var("KUZU_TESTING").is_ok() { - println!("cargo:rustc-link-lib=dylib=ssl"); - println!("cargo:rustc-link-lib=dylib=crypto"); + if cfg!(windows) { + // Find openssl library relative to the path of the openssl executable + // Or fall back to OPENSSL_DIR + #[cfg(windows)] + { + let openssl_dir = if let Ok(mut path) = which::which("openssl") { + path.pop(); + path.pop(); + path + } else if let Ok(path) = env::var("OPENSSL_CONF") { + Path::new(&path) + .parent() + .unwrap() + .parent() + .unwrap() + .to_path_buf() + } else if let Ok(path) = env::var("OPENSSL_DIR") { + Path::new(&path).to_path_buf() + } else { + panic!( + "OPENSSL_DIR must be set if the openssl library cannot be found \ + using the path of the openssl executable" + ) + }; + println!( + "cargo:rustc-link-search=native={}/lib", + openssl_dir.display() + ); + } + println!("cargo:rustc-link-lib=dylib=libssl"); + println!("cargo:rustc-link-lib=dylib=libcrypto"); + } else { + println!("cargo:rustc-link-lib=dylib=ssl"); + println!("cargo:rustc-link-lib=dylib=crypto"); + } } - println!("cargo:rustc-link-lib=static=parquet"); - println!("cargo:rustc-link-lib=static=arrow"); + if cfg!(windows) { + println!("cargo:rustc-link-lib=static=parquet_static"); + println!("cargo:rustc-link-lib=static=arrow_static"); + } else { + println!("cargo:rustc-link-lib=static=parquet"); + println!("cargo:rustc-link-lib=static=arrow"); + } println!("cargo:rustc-link-lib=static=utf8proc"); println!("cargo:rustc-link-lib=static=antlr4_cypher"); @@ -83,11 +156,13 @@ fn main() -> Result<(), Box> { println!("cargo:rerun-if-changed=include/kuzu_rs.h"); println!("cargo:rerun-if-changed=include/kuzu_rs.cpp"); - cxx_build::bridge("src/ffi.rs") - .file("src/kuzu_rs.cpp") - .flag_if_supported("-std=c++20") - .includes(include_paths) - .compile("kuzu_rs"); + let mut build = cxx_build::bridge("src/ffi.rs"); + build.file("src/kuzu_rs.cpp").includes(include_paths); - Ok(()) + if cfg!(windows) { + build.flag("/std:c++20"); + } else { + build.flag_if_supported("-std=c++20"); + } + build.compile("kuzu_rs"); } diff --git a/tools/rust_api/src/database.rs b/tools/rust_api/src/database.rs index 4680d6972f..f533ea8817 100644 --- a/tools/rust_api/src/database.rs +++ b/tools/rust_api/src/database.rs @@ -81,9 +81,20 @@ mod tests { let result: Error = Database::new("", 0) .expect_err("An empty string should not be a valid database path!") .into(); - assert_eq!( - result.to_string(), - "Failed to create directory due to: filesystem error: cannot create directory: No such file or directory []" - ); + if cfg!(windows) { + assert_eq!( + result.to_string(), + "Failed to create directory due to: create_directory: The system cannot find the path specified.: \"\"" + ); + } else if cfg!(linux) { + assert_eq!( + result.to_string(), + "Failed to create directory due to: filesystem error: cannot create directory: No such file or directory []" + ); + } else { + assert!(result + .to_string() + .starts_with("Failed to create directory due to")); + } } } diff --git a/tools/rust_api/src/query_result.rs b/tools/rust_api/src/query_result.rs index 032caf7ac4..9ee92dc27f 100644 --- a/tools/rust_api/src/query_result.rs +++ b/tools/rust_api/src/query_result.rs @@ -213,7 +213,12 @@ mod tests { CSVOptions::default().delimiter(','), )?; let data = std::fs::read_to_string(path.join("output.csv"))?; - assert_eq!(data, "Alice,25\n"); + if cfg!(windows) { + // Windows translates the newlines automatically in text mode + assert_eq!(data, "Alice,25\r\n"); + } else { + assert_eq!(data, "Alice,25\n"); + } temp_dir.close()?; Ok(()) }