felipecrv commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2186066404
##########
rust/core/src/driver_manager.rs:
##########
@@ -1310,3 +1461,489 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+const fn current_arch() -> &'static str {
+ #[cfg(target_arch = "x86_64")]
+ const ARCH: &str = "amd64";
+ #[cfg(target_arch = "aarch64")]
+ const ARCH: &str = "arm64";
+ #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))]
+ const ARCH: &str = std::env::consts::ARCH;
+
+ #[cfg(target_os = "macos")]
+ const OS: &str = "osx";
+ #[cfg(not(target_os = "macos"))]
+ const OS: &str = std::env::consts::OS;
+
+ #[cfg(target_env = "musl")]
+ const EXTRA: &str = "_musl";
+ #[cfg(all(target_os = "windows", target_env = "gnu"))]
+ const EXTRA: &str = "_mingw";
+ #[cfg(not(any(target_env = "musl", all(target_os = "windows", target_env =
"gnu"))))]
+ const EXTRA: &str = "";
+
+ concat!(OS, "_", ARCH, EXTRA)
Review Comment:
I agree. Just call it `arch_triplet() -> (&'static str, &'static str,
&'static str)`
##########
rust/core/src/driver_manager.rs:
##########
@@ -316,6 +408,65 @@ impl Driver for ManagedDriver {
}
}
+fn get_optional_key(manifest: &Table, key: &str) -> String {
+ manifest
+ .get(key)
+ .and_then(Value::as_str)
+ .unwrap_or_default()
+ .to_string()
+}
+
+fn load_driver_manifest(info: &mut DriverInfo) -> Result<()> {
+ let contents =
fs::read_to_string(info.manifest_file.as_path()).map_err(|e| {
+ let file = info.manifest_file.to_string_lossy();
+ Error::with_message_and_status(
+ format!("Could not read manifest '{file}': {e}"),
+ Status::InvalidArguments,
+ )
+ })?;
+
+ let manifest = contents
+ .parse::<Table>()
+ .map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
Review Comment:
PLEASE let's not include `serde`. TOML was chosen as the format for
manifests and that's the only ser/de format that the driver manager will need.
@paleolimbot 💯. Better error messages, less binary code bloat, better
abstractions... etc.
##########
rust/core/src/driver_manager.rs:
##########
@@ -1310,3 +1461,489 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+const fn current_arch() -> &'static str {
+ #[cfg(target_arch = "x86_64")]
+ const ARCH: &str = "amd64";
+ #[cfg(target_arch = "aarch64")]
+ const ARCH: &str = "arm64";
+ #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))]
+ const ARCH: &str = std::env::consts::ARCH;
+
+ #[cfg(target_os = "macos")]
+ const OS: &str = "osx";
+ #[cfg(not(target_os = "macos"))]
+ const OS: &str = std::env::consts::OS;
+
+ #[cfg(target_env = "musl")]
+ const EXTRA: &str = "_musl";
+ #[cfg(all(target_os = "windows", target_env = "gnu"))]
+ const EXTRA: &str = "_mingw";
+ #[cfg(not(any(target_env = "musl", all(target_os = "windows", target_env =
"gnu"))))]
+ const EXTRA: &str = "";
+
+ concat!(OS, "_", ARCH, EXTRA)
+}
+
+#[cfg(target_os = "windows")]
+extern crate windows_sys as windows;
+
+#[cfg(target_os = "windows")]
+mod target_windows {
+ use std::ffi::c_void;
+ use std::ffi::OsString;
+ use std::os::windows::ffi::OsStringExt;
+ use std::path::PathBuf;
+ use std::slice;
+
+ use super::windows::Win32::UI::Shell;
+
+ fn user_config_dir() -> Option<PathBuf> {
+ unsafe {
+ let mut path_ptr: windows::core::PWSTR = std::ptr::null_mut();
+ let result = Shell::SHGetKnownFolderPath(
+ Shell::FOLDERID_LocalAppData,
+ 0,
+ std::ptr::null_mut(),
+ &mut path_ptr,
+ );
+
+ if result == 0 {
+ let len = windows::Win32::Globalization::lstrlenW(path_ptr) as
usize;
+ let path = slice::from_raw_parts(path_ptr, len);
+ let ostr: OsString = OsStringExt::from_wide(path);
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ Some(PathBuf::from(ostr))
+ } else {
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ None
+ }
+ }
+ }
+}
+
+fn user_config_dir() -> Option<PathBuf> {
+ #[cfg(target_os = "windows")]
+ {
+ use target_windows::user_config_dir;
+ user_config_dir().and_then(|path| {
+ path.push("ADBC");
+ path.push("drivers");
+ Some(path)
+ })
+ }
+
+ #[cfg(target_os = "macos")]
+ {
+ env::var_os("HOME").map(PathBuf::from).and_then(|mut path| {
+ path.push("Library");
+ path.push("Application Support");
+ path.push("ADBC");
+ Some(path)
+ })
+ }
+
+ #[cfg(all(unix, not(target_os = "macos")))]
+ {
+ env::var_os("XDG_CONFIG_HOME")
+ .map(PathBuf::from)
+ .or_else(|| {
+ env::var_os("HOME").map(|home| {
+ let mut path = PathBuf::from(home);
+ path.push(".config");
+ path
+ })
+ })
+ .and_then(|mut path| {
+ path.push("adbc");
+ Some(path)
+ })
+ }
+}
+
+fn get_search_paths(lvls: LoadFlags) -> Vec<PathBuf> {
+ let mut result = Vec::new();
+ if lvls & LOAD_FLAG_SEARCH_ENV != 0 {
+ env::var_os("ADBC_CONFIG_PATH").and_then(|paths| {
+ for p in env::split_paths(&paths) {
+ result.push(p);
+ }
+ Some(())
+ });
+ }
+
+ if lvls & LOAD_FLAG_SEARCH_USER != 0 {
+ if let Some(path) = user_config_dir() {
+ if path.exists() {
+ result.push(path);
+ }
+ }
+ }
+
+ // system level for windows is to search the registry keys
+ #[cfg(not(windows))]
+ if lvls & LOAD_FLAG_SEARCH_SYSTEM != 0 {
+ let system_config_dir = PathBuf::from("/etc/adbc");
+ if system_config_dir.exists() {
+ result.push(system_config_dir);
+ }
+ }
+
+ result
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ use crate::LOAD_FLAG_DEFAULT;
+ use tempfile::Builder;
+
+ fn simple_manifest() -> toml::Table {
+ // if this test is enabled, we expect the env var
ADBC_DRIVER_MANAGER_TEST_LIB
+ // to be defined.
+ let driver_path =
+ PathBuf::from(env::var_os("ADBC_DRIVER_MANAGER_TEST_LIB").expect(
+ "ADBC_DRIVER_MANAGER_TEST_LIB must be set for driver manager
manifest tests",
+ ));
+
+ assert!(
+ driver_path.exists(),
+ "ADBC_DRIVER_MANAGER_TEST_LIB path does not exist: {}",
+ driver_path.display()
+ );
+
+ let arch = current_arch();
+ format!(
+ r#"
+ name = 'SQLite3'
+ publisher = 'arrow-adbc'
+ version = '1.0.0'
+
+ [ADBC]
+ version = '1.1.0'
+
+ [Driver]
+ [Driver.shared]
+ {arch} = {driver_path:?}
+ "#
+ )
+ .parse::<toml::Table>()
+ .unwrap()
+ }
+
+ fn write_manifest_to_tempfile(p: PathBuf, tbl: toml::Table) ->
(tempfile::TempDir, PathBuf) {
+ let tmp_dir = Builder::new()
+ .prefix("adbc_tests")
+ .tempdir()
+ .expect("Failed to create temporary directory for driver manager
manifest test");
+
+ let manifest_path = tmp_dir.path().join(p);
+ if let Some(parent) = manifest_path.parent() {
+ std::fs::create_dir_all(parent)
+ .expect("Failed to create parent directory for manifest");
+ }
+
+ std::fs::write(&manifest_path, toml::to_string_pretty(&tbl).unwrap())
+ .expect("Failed to write driver manager manifest to temporary
file");
+
+ (tmp_dir, manifest_path)
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_driver_env() {
+ // ensure that we fail without the env var set
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV
+ )
+ .is_err());
Review Comment:
yes. unwrap in tests is better.
##########
rust/core/src/driver_manager.rs:
##########
@@ -1310,3 +1461,489 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+const fn current_arch() -> &'static str {
+ #[cfg(target_arch = "x86_64")]
+ const ARCH: &str = "amd64";
+ #[cfg(target_arch = "aarch64")]
+ const ARCH: &str = "arm64";
+ #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))]
+ const ARCH: &str = std::env::consts::ARCH;
+
+ #[cfg(target_os = "macos")]
+ const OS: &str = "osx";
+ #[cfg(not(target_os = "macos"))]
+ const OS: &str = std::env::consts::OS;
+
+ #[cfg(target_env = "musl")]
+ const EXTRA: &str = "_musl";
+ #[cfg(all(target_os = "windows", target_env = "gnu"))]
+ const EXTRA: &str = "_mingw";
+ #[cfg(not(any(target_env = "musl", all(target_os = "windows", target_env =
"gnu"))))]
+ const EXTRA: &str = "";
+
+ concat!(OS, "_", ARCH, EXTRA)
+}
+
+#[cfg(target_os = "windows")]
+extern crate windows_sys as windows;
+
+#[cfg(target_os = "windows")]
+mod target_windows {
+ use std::ffi::c_void;
+ use std::ffi::OsString;
+ use std::os::windows::ffi::OsStringExt;
+ use std::path::PathBuf;
+ use std::slice;
+
+ use super::windows::Win32::UI::Shell;
+
+ fn user_config_dir() -> Option<PathBuf> {
+ unsafe {
+ let mut path_ptr: windows::core::PWSTR = std::ptr::null_mut();
+ let result = Shell::SHGetKnownFolderPath(
+ Shell::FOLDERID_LocalAppData,
+ 0,
+ std::ptr::null_mut(),
+ &mut path_ptr,
+ );
+
+ if result == 0 {
+ let len = windows::Win32::Globalization::lstrlenW(path_ptr) as
usize;
+ let path = slice::from_raw_parts(path_ptr, len);
+ let ostr: OsString = OsStringExt::from_wide(path);
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ Some(PathBuf::from(ostr))
+ } else {
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ None
+ }
+ }
+ }
+}
Review Comment:
Have you compared this implementation with the one from `dirs`?
https://github.com/dirs-dev/dirs-sys-rs/blob/main/src/lib.rs#L150
##########
rust/core/src/driver_manager.rs:
##########
@@ -276,6 +351,23 @@ impl ManagedDriver {
Ok(database)
}
+
+ fn load_driver_from_manifest(
Review Comment:
`load_from_manifest`
##########
rust/core/src/driver_manager.rs:
##########
@@ -184,6 +200,65 @@ impl ManagedDriver {
Ok(ManagedDriver { inner })
}
+ /// Load a driver either by name, filename, path, or via locating a toml
manifest file.
+ /// The LoadFlags control what directories are searched to locate a
manifest.
+ pub fn find_load_from_name(
Review Comment:
~`load_dynamic_from_name` to be similar to `load_dynamic_from_filename`~
Maybe we should drop the `dynamic_` from all these function names. An actual
"load" really is dynamic, "load_static" is the weird one: it doesn't warrant us
making every other method contain the word "dynamic".
##########
rust/core/src/lib.rs:
##########
@@ -532,3 +532,14 @@ pub struct PartitionedResult {
/// The number of rows affected if known, else -1.
pub rows_affected: i64,
}
+
+pub type LoadFlags = u32;
+
+pub const LOAD_FLAG_SEARCH_ENV: LoadFlags = 1 << 1;
+pub const LOAD_FLAG_SEARCH_USER: LoadFlags = 1 << 2;
+pub const LOAD_FLAG_SEARCH_SYSTEM: LoadFlags = 1 << 3;
+pub const LOAD_FLAG_ALLOW_RELATIVE_PATHS: LoadFlags = 1 << 4;
+pub const LOAD_FLAG_DEFAULT: LoadFlags = LOAD_FLAG_SEARCH_ENV
+ | LOAD_FLAG_SEARCH_USER
+ | LOAD_FLAG_SEARCH_SYSTEM
+ | LOAD_FLAG_ALLOW_RELATIVE_PATHS;
Review Comment:
`const` are more commonly declared at the top of source files, so it's weird
to see these at the very bottom. Can you move them up? :)
##########
rust/core/src/driver_manager.rs:
##########
@@ -1310,3 +1461,489 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+const fn current_arch() -> &'static str {
+ #[cfg(target_arch = "x86_64")]
+ const ARCH: &str = "amd64";
+ #[cfg(target_arch = "aarch64")]
+ const ARCH: &str = "arm64";
+ #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))]
+ const ARCH: &str = std::env::consts::ARCH;
+
+ #[cfg(target_os = "macos")]
+ const OS: &str = "osx";
+ #[cfg(not(target_os = "macos"))]
+ const OS: &str = std::env::consts::OS;
+
+ #[cfg(target_env = "musl")]
+ const EXTRA: &str = "_musl";
+ #[cfg(all(target_os = "windows", target_env = "gnu"))]
+ const EXTRA: &str = "_mingw";
+ #[cfg(not(any(target_env = "musl", all(target_os = "windows", target_env =
"gnu"))))]
+ const EXTRA: &str = "";
+
+ concat!(OS, "_", ARCH, EXTRA)
+}
+
+#[cfg(target_os = "windows")]
+extern crate windows_sys as windows;
+
+#[cfg(target_os = "windows")]
+mod target_windows {
+ use std::ffi::c_void;
+ use std::ffi::OsString;
+ use std::os::windows::ffi::OsStringExt;
+ use std::path::PathBuf;
+ use std::slice;
+
+ use super::windows::Win32::UI::Shell;
+
+ fn user_config_dir() -> Option<PathBuf> {
+ unsafe {
+ let mut path_ptr: windows::core::PWSTR = std::ptr::null_mut();
+ let result = Shell::SHGetKnownFolderPath(
+ Shell::FOLDERID_LocalAppData,
+ 0,
+ std::ptr::null_mut(),
+ &mut path_ptr,
+ );
+
+ if result == 0 {
+ let len = windows::Win32::Globalization::lstrlenW(path_ptr) as
usize;
+ let path = slice::from_raw_parts(path_ptr, len);
+ let ostr: OsString = OsStringExt::from_wide(path);
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ Some(PathBuf::from(ostr))
+ } else {
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ None
+ }
+ }
+ }
+}
+
+fn user_config_dir() -> Option<PathBuf> {
+ #[cfg(target_os = "windows")]
+ {
+ use target_windows::user_config_dir;
+ user_config_dir().and_then(|path| {
+ path.push("ADBC");
+ path.push("drivers");
+ Some(path)
+ })
+ }
+
+ #[cfg(target_os = "macos")]
+ {
+ env::var_os("HOME").map(PathBuf::from).and_then(|mut path| {
+ path.push("Library");
+ path.push("Application Support");
+ path.push("ADBC");
+ Some(path)
+ })
+ }
+
+ #[cfg(all(unix, not(target_os = "macos")))]
+ {
+ env::var_os("XDG_CONFIG_HOME")
+ .map(PathBuf::from)
+ .or_else(|| {
+ env::var_os("HOME").map(|home| {
+ let mut path = PathBuf::from(home);
+ path.push(".config");
+ path
+ })
+ })
+ .and_then(|mut path| {
+ path.push("adbc");
+ Some(path)
+ })
+ }
+}
+
+fn get_search_paths(lvls: LoadFlags) -> Vec<PathBuf> {
+ let mut result = Vec::new();
+ if lvls & LOAD_FLAG_SEARCH_ENV != 0 {
+ env::var_os("ADBC_CONFIG_PATH").and_then(|paths| {
+ for p in env::split_paths(&paths) {
+ result.push(p);
+ }
+ Some(())
+ });
+ }
+
+ if lvls & LOAD_FLAG_SEARCH_USER != 0 {
+ if let Some(path) = user_config_dir() {
+ if path.exists() {
+ result.push(path);
+ }
+ }
+ }
+
+ // system level for windows is to search the registry keys
+ #[cfg(not(windows))]
+ if lvls & LOAD_FLAG_SEARCH_SYSTEM != 0 {
+ let system_config_dir = PathBuf::from("/etc/adbc");
+ if system_config_dir.exists() {
+ result.push(system_config_dir);
+ }
+ }
+
+ result
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ use crate::LOAD_FLAG_DEFAULT;
+ use tempfile::Builder;
+
+ fn simple_manifest() -> toml::Table {
+ // if this test is enabled, we expect the env var
ADBC_DRIVER_MANAGER_TEST_LIB
+ // to be defined.
+ let driver_path =
+ PathBuf::from(env::var_os("ADBC_DRIVER_MANAGER_TEST_LIB").expect(
+ "ADBC_DRIVER_MANAGER_TEST_LIB must be set for driver manager
manifest tests",
+ ));
+
+ assert!(
+ driver_path.exists(),
+ "ADBC_DRIVER_MANAGER_TEST_LIB path does not exist: {}",
+ driver_path.display()
+ );
+
+ let arch = current_arch();
+ format!(
+ r#"
+ name = 'SQLite3'
+ publisher = 'arrow-adbc'
+ version = '1.0.0'
+
+ [ADBC]
+ version = '1.1.0'
+
+ [Driver]
+ [Driver.shared]
+ {arch} = {driver_path:?}
+ "#
+ )
+ .parse::<toml::Table>()
+ .unwrap()
+ }
+
+ fn write_manifest_to_tempfile(p: PathBuf, tbl: toml::Table) ->
(tempfile::TempDir, PathBuf) {
+ let tmp_dir = Builder::new()
+ .prefix("adbc_tests")
+ .tempdir()
+ .expect("Failed to create temporary directory for driver manager
manifest test");
+
+ let manifest_path = tmp_dir.path().join(p);
+ if let Some(parent) = manifest_path.parent() {
+ std::fs::create_dir_all(parent)
+ .expect("Failed to create parent directory for manifest");
+ }
+
+ std::fs::write(&manifest_path, toml::to_string_pretty(&tbl).unwrap())
+ .expect("Failed to write driver manager manifest to temporary
file");
+
+ (tmp_dir, manifest_path)
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_driver_env() {
+ // ensure that we fail without the env var set
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV
+ )
+ .is_err());
+
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ env::set_var(
+ "ADBC_CONFIG_PATH",
+ manifest_path.parent().unwrap().as_os_str(),
+ );
+
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV,
+ )
+ .is_ok());
+
+ env::remove_var("ADBC_CONFIG_PATH");
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_driver_env_multiple_paths() {
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ let path_os_string =
+ env::join_paths([Path::new("/home"), Path::new(""),
manifest_path.as_path()]).unwrap();
+ env::set_var("ADBC_CONFIG_PATH", path_os_string);
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV,
+ )
+ .is_ok());
+
+ env::remove_var("ADBC_CONFIG_PATH");
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_non_ascii_path() {
+ let p = PathBuf::from("majestik møøse/sqlite.toml");
+ let (tmp_dir, manifest_path) = write_manifest_to_tempfile(p,
simple_manifest());
+
+ env::set_var(
+ "ADBC_CONFIG_PATH",
+ manifest_path.parent().unwrap().as_os_str(),
+ );
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV,
+ )
+ .is_ok());
+
+ env::remove_var("ADBC_CONFIG_PATH");
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_disallow_env_config() {
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ env::set_var(
+ "ADBC_CONFIG_PATH",
+ manifest_path.parent().unwrap().as_os_str(),
+ );
+
+ let load_flags = LOAD_FLAG_DEFAULT & !LOAD_FLAG_SEARCH_ENV;
+ assert!(
+ ManagedDriver::find_load_from_name("sqlite", None,
AdbcVersion::V100, load_flags)
+ .is_err_and(|e| e.status == Status::NotFound)
+ );
+
+ env::remove_var("ADBC_CONFIG_PATH");
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_absolute_path() {
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ assert!(ManagedDriver::find_load_from_name(
+ manifest_path,
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_DEFAULT,
+ )
+ .is_ok());
+
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_absolute_path_no_ext() {
+ let (tmp_dir, mut manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ manifest_path.set_extension("");
+ assert!(ManagedDriver::find_load_from_name(
+ manifest_path,
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_DEFAULT,
+ )
+ .is_ok());
Review Comment:
Better to replace all `assert!(expr.is.ok())` with `expr.unwrap()`.
`unwrap()` will panic on error like `assert!` would do on a `false` boolean,
but with a better error message.
##########
rust/core/src/driver_manager.rs:
##########
@@ -1310,3 +1461,489 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+const fn current_arch() -> &'static str {
+ #[cfg(target_arch = "x86_64")]
+ const ARCH: &str = "amd64";
+ #[cfg(target_arch = "aarch64")]
+ const ARCH: &str = "arm64";
+ #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))]
+ const ARCH: &str = std::env::consts::ARCH;
+
+ #[cfg(target_os = "macos")]
+ const OS: &str = "osx";
+ #[cfg(not(target_os = "macos"))]
+ const OS: &str = std::env::consts::OS;
+
+ #[cfg(target_env = "musl")]
+ const EXTRA: &str = "_musl";
+ #[cfg(all(target_os = "windows", target_env = "gnu"))]
+ const EXTRA: &str = "_mingw";
+ #[cfg(not(any(target_env = "musl", all(target_os = "windows", target_env =
"gnu"))))]
+ const EXTRA: &str = "";
+
+ concat!(OS, "_", ARCH, EXTRA)
+}
+
+#[cfg(target_os = "windows")]
+extern crate windows_sys as windows;
+
+#[cfg(target_os = "windows")]
+mod target_windows {
+ use std::ffi::c_void;
+ use std::ffi::OsString;
+ use std::os::windows::ffi::OsStringExt;
+ use std::path::PathBuf;
+ use std::slice;
+
+ use super::windows::Win32::UI::Shell;
+
+ fn user_config_dir() -> Option<PathBuf> {
+ unsafe {
+ let mut path_ptr: windows::core::PWSTR = std::ptr::null_mut();
+ let result = Shell::SHGetKnownFolderPath(
+ Shell::FOLDERID_LocalAppData,
+ 0,
+ std::ptr::null_mut(),
+ &mut path_ptr,
+ );
+
+ if result == 0 {
+ let len = windows::Win32::Globalization::lstrlenW(path_ptr) as
usize;
+ let path = slice::from_raw_parts(path_ptr, len);
+ let ostr: OsString = OsStringExt::from_wide(path);
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ Some(PathBuf::from(ostr))
+ } else {
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ None
+ }
+ }
+ }
+}
+
+fn user_config_dir() -> Option<PathBuf> {
+ #[cfg(target_os = "windows")]
+ {
+ use target_windows::user_config_dir;
+ user_config_dir().and_then(|path| {
+ path.push("ADBC");
+ path.push("drivers");
+ Some(path)
+ })
+ }
+
+ #[cfg(target_os = "macos")]
+ {
+ env::var_os("HOME").map(PathBuf::from).and_then(|mut path| {
+ path.push("Library");
+ path.push("Application Support");
+ path.push("ADBC");
+ Some(path)
+ })
+ }
+
+ #[cfg(all(unix, not(target_os = "macos")))]
+ {
+ env::var_os("XDG_CONFIG_HOME")
+ .map(PathBuf::from)
+ .or_else(|| {
+ env::var_os("HOME").map(|home| {
+ let mut path = PathBuf::from(home);
+ path.push(".config");
+ path
+ })
+ })
+ .and_then(|mut path| {
+ path.push("adbc");
+ Some(path)
+ })
+ }
+}
+
+fn get_search_paths(lvls: LoadFlags) -> Vec<PathBuf> {
+ let mut result = Vec::new();
+ if lvls & LOAD_FLAG_SEARCH_ENV != 0 {
+ env::var_os("ADBC_CONFIG_PATH").and_then(|paths| {
+ for p in env::split_paths(&paths) {
+ result.push(p);
+ }
+ Some(())
+ });
+ }
+
+ if lvls & LOAD_FLAG_SEARCH_USER != 0 {
+ if let Some(path) = user_config_dir() {
+ if path.exists() {
+ result.push(path);
+ }
+ }
+ }
+
+ // system level for windows is to search the registry keys
+ #[cfg(not(windows))]
+ if lvls & LOAD_FLAG_SEARCH_SYSTEM != 0 {
+ let system_config_dir = PathBuf::from("/etc/adbc");
+ if system_config_dir.exists() {
+ result.push(system_config_dir);
+ }
+ }
+
+ result
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ use crate::LOAD_FLAG_DEFAULT;
+ use tempfile::Builder;
+
+ fn simple_manifest() -> toml::Table {
+ // if this test is enabled, we expect the env var
ADBC_DRIVER_MANAGER_TEST_LIB
+ // to be defined.
+ let driver_path =
+ PathBuf::from(env::var_os("ADBC_DRIVER_MANAGER_TEST_LIB").expect(
+ "ADBC_DRIVER_MANAGER_TEST_LIB must be set for driver manager
manifest tests",
+ ));
+
+ assert!(
+ driver_path.exists(),
+ "ADBC_DRIVER_MANAGER_TEST_LIB path does not exist: {}",
+ driver_path.display()
+ );
+
+ let arch = current_arch();
+ format!(
+ r#"
+ name = 'SQLite3'
+ publisher = 'arrow-adbc'
+ version = '1.0.0'
+
+ [ADBC]
+ version = '1.1.0'
+
+ [Driver]
+ [Driver.shared]
+ {arch} = {driver_path:?}
+ "#
+ )
+ .parse::<toml::Table>()
+ .unwrap()
+ }
+
+ fn write_manifest_to_tempfile(p: PathBuf, tbl: toml::Table) ->
(tempfile::TempDir, PathBuf) {
+ let tmp_dir = Builder::new()
+ .prefix("adbc_tests")
+ .tempdir()
+ .expect("Failed to create temporary directory for driver manager
manifest test");
+
+ let manifest_path = tmp_dir.path().join(p);
+ if let Some(parent) = manifest_path.parent() {
+ std::fs::create_dir_all(parent)
+ .expect("Failed to create parent directory for manifest");
+ }
+
+ std::fs::write(&manifest_path, toml::to_string_pretty(&tbl).unwrap())
+ .expect("Failed to write driver manager manifest to temporary
file");
+
+ (tmp_dir, manifest_path)
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_driver_env() {
+ // ensure that we fail without the env var set
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV
+ )
+ .is_err());
+
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ env::set_var(
+ "ADBC_CONFIG_PATH",
+ manifest_path.parent().unwrap().as_os_str(),
+ );
+
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV,
+ )
+ .is_ok());
Review Comment:
💯
##########
rust/core/src/driver_manager.rs:
##########
@@ -1310,3 +1461,489 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+const fn current_arch() -> &'static str {
+ #[cfg(target_arch = "x86_64")]
+ const ARCH: &str = "amd64";
+ #[cfg(target_arch = "aarch64")]
+ const ARCH: &str = "arm64";
+ #[cfg(not(any(target_arch = "x86_64", target_arch = "aarch64")))]
+ const ARCH: &str = std::env::consts::ARCH;
+
+ #[cfg(target_os = "macos")]
+ const OS: &str = "osx";
+ #[cfg(not(target_os = "macos"))]
+ const OS: &str = std::env::consts::OS;
+
+ #[cfg(target_env = "musl")]
+ const EXTRA: &str = "_musl";
+ #[cfg(all(target_os = "windows", target_env = "gnu"))]
+ const EXTRA: &str = "_mingw";
+ #[cfg(not(any(target_env = "musl", all(target_os = "windows", target_env =
"gnu"))))]
+ const EXTRA: &str = "";
+
+ concat!(OS, "_", ARCH, EXTRA)
+}
+
+#[cfg(target_os = "windows")]
+extern crate windows_sys as windows;
+
+#[cfg(target_os = "windows")]
+mod target_windows {
+ use std::ffi::c_void;
+ use std::ffi::OsString;
+ use std::os::windows::ffi::OsStringExt;
+ use std::path::PathBuf;
+ use std::slice;
+
+ use super::windows::Win32::UI::Shell;
+
+ fn user_config_dir() -> Option<PathBuf> {
+ unsafe {
+ let mut path_ptr: windows::core::PWSTR = std::ptr::null_mut();
+ let result = Shell::SHGetKnownFolderPath(
+ Shell::FOLDERID_LocalAppData,
+ 0,
+ std::ptr::null_mut(),
+ &mut path_ptr,
+ );
+
+ if result == 0 {
+ let len = windows::Win32::Globalization::lstrlenW(path_ptr) as
usize;
+ let path = slice::from_raw_parts(path_ptr, len);
+ let ostr: OsString = OsStringExt::from_wide(path);
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ Some(PathBuf::from(ostr))
+ } else {
+ windows::Win32::System::Com::CoTaskMemFree(path_ptr as *const
c_void);
+ None
+ }
+ }
+ }
+}
+
+fn user_config_dir() -> Option<PathBuf> {
+ #[cfg(target_os = "windows")]
+ {
+ use target_windows::user_config_dir;
+ user_config_dir().and_then(|path| {
+ path.push("ADBC");
+ path.push("drivers");
+ Some(path)
+ })
+ }
+
+ #[cfg(target_os = "macos")]
+ {
+ env::var_os("HOME").map(PathBuf::from).and_then(|mut path| {
+ path.push("Library");
+ path.push("Application Support");
+ path.push("ADBC");
+ Some(path)
+ })
+ }
+
+ #[cfg(all(unix, not(target_os = "macos")))]
+ {
+ env::var_os("XDG_CONFIG_HOME")
+ .map(PathBuf::from)
+ .or_else(|| {
+ env::var_os("HOME").map(|home| {
+ let mut path = PathBuf::from(home);
+ path.push(".config");
+ path
+ })
+ })
+ .and_then(|mut path| {
+ path.push("adbc");
+ Some(path)
+ })
+ }
+}
+
+fn get_search_paths(lvls: LoadFlags) -> Vec<PathBuf> {
+ let mut result = Vec::new();
+ if lvls & LOAD_FLAG_SEARCH_ENV != 0 {
+ env::var_os("ADBC_CONFIG_PATH").and_then(|paths| {
+ for p in env::split_paths(&paths) {
+ result.push(p);
+ }
+ Some(())
+ });
+ }
+
+ if lvls & LOAD_FLAG_SEARCH_USER != 0 {
+ if let Some(path) = user_config_dir() {
+ if path.exists() {
+ result.push(path);
+ }
+ }
+ }
+
+ // system level for windows is to search the registry keys
+ #[cfg(not(windows))]
+ if lvls & LOAD_FLAG_SEARCH_SYSTEM != 0 {
+ let system_config_dir = PathBuf::from("/etc/adbc");
+ if system_config_dir.exists() {
+ result.push(system_config_dir);
+ }
+ }
+
+ result
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ use crate::LOAD_FLAG_DEFAULT;
+ use tempfile::Builder;
+
+ fn simple_manifest() -> toml::Table {
+ // if this test is enabled, we expect the env var
ADBC_DRIVER_MANAGER_TEST_LIB
+ // to be defined.
+ let driver_path =
+ PathBuf::from(env::var_os("ADBC_DRIVER_MANAGER_TEST_LIB").expect(
+ "ADBC_DRIVER_MANAGER_TEST_LIB must be set for driver manager
manifest tests",
+ ));
+
+ assert!(
+ driver_path.exists(),
+ "ADBC_DRIVER_MANAGER_TEST_LIB path does not exist: {}",
+ driver_path.display()
+ );
+
+ let arch = current_arch();
+ format!(
+ r#"
+ name = 'SQLite3'
+ publisher = 'arrow-adbc'
+ version = '1.0.0'
+
+ [ADBC]
+ version = '1.1.0'
+
+ [Driver]
+ [Driver.shared]
+ {arch} = {driver_path:?}
+ "#
+ )
+ .parse::<toml::Table>()
+ .unwrap()
+ }
+
+ fn write_manifest_to_tempfile(p: PathBuf, tbl: toml::Table) ->
(tempfile::TempDir, PathBuf) {
+ let tmp_dir = Builder::new()
+ .prefix("adbc_tests")
+ .tempdir()
+ .expect("Failed to create temporary directory for driver manager
manifest test");
+
+ let manifest_path = tmp_dir.path().join(p);
+ if let Some(parent) = manifest_path.parent() {
+ std::fs::create_dir_all(parent)
+ .expect("Failed to create parent directory for manifest");
+ }
+
+ std::fs::write(&manifest_path, toml::to_string_pretty(&tbl).unwrap())
+ .expect("Failed to write driver manager manifest to temporary
file");
+
+ (tmp_dir, manifest_path)
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_driver_env() {
+ // ensure that we fail without the env var set
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV
+ )
+ .is_err());
+
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ env::set_var(
+ "ADBC_CONFIG_PATH",
+ manifest_path.parent().unwrap().as_os_str(),
+ );
+
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV,
+ )
+ .is_ok());
+
+ env::remove_var("ADBC_CONFIG_PATH");
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_driver_env_multiple_paths() {
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ let path_os_string =
+ env::join_paths([Path::new("/home"), Path::new(""),
manifest_path.as_path()]).unwrap();
+ env::set_var("ADBC_CONFIG_PATH", path_os_string);
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV,
+ )
+ .is_ok());
+
+ env::remove_var("ADBC_CONFIG_PATH");
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_non_ascii_path() {
+ let p = PathBuf::from("majestik møøse/sqlite.toml");
+ let (tmp_dir, manifest_path) = write_manifest_to_tempfile(p,
simple_manifest());
+
+ env::set_var(
+ "ADBC_CONFIG_PATH",
+ manifest_path.parent().unwrap().as_os_str(),
+ );
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_SEARCH_ENV,
+ )
+ .is_ok());
+
+ env::remove_var("ADBC_CONFIG_PATH");
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_disallow_env_config() {
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ env::set_var(
+ "ADBC_CONFIG_PATH",
+ manifest_path.parent().unwrap().as_os_str(),
+ );
+
+ let load_flags = LOAD_FLAG_DEFAULT & !LOAD_FLAG_SEARCH_ENV;
+ assert!(
+ ManagedDriver::find_load_from_name("sqlite", None,
AdbcVersion::V100, load_flags)
+ .is_err_and(|e| e.status == Status::NotFound)
+ );
+
+ env::remove_var("ADBC_CONFIG_PATH");
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_absolute_path() {
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ assert!(ManagedDriver::find_load_from_name(
+ manifest_path,
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_DEFAULT,
+ )
+ .is_ok());
+
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_absolute_path_no_ext() {
+ let (tmp_dir, mut manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
simple_manifest());
+
+ manifest_path.set_extension("");
+ assert!(ManagedDriver::find_load_from_name(
+ manifest_path,
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_DEFAULT,
+ )
+ .is_ok());
+
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_load_relative_path() {
+ std::fs::write(
+ PathBuf::from("sqlite.toml"),
+ toml::to_string_pretty(&simple_manifest()).unwrap(),
+ )
+ .expect("Failed to write driver manager manifest to file");
+
+ assert!(
+ ManagedDriver::find_load_from_name("sqlite.toml", None,
AdbcVersion::V100, 0)
+ .is_err_and(|e| e.status == Status::InvalidArguments)
+ );
+
+ assert!(ManagedDriver::find_load_from_name(
+ "sqlite.toml",
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_ALLOW_RELATIVE_PATHS,
+ )
+ .is_ok());
+
+ std::fs::remove_file("sqlite.toml")
+ .expect("Failed to remove temporary driver manifest file");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_manifest_missing_driver() {
+ let mut manifest_without_driver = simple_manifest();
+ manifest_without_driver.remove("Driver");
+
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
manifest_without_driver);
+
+ assert!(ManagedDriver::find_load_from_name(
+ manifest_path,
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_DEFAULT
+ )
+ .is_err_and(|e| e.status == Status::InvalidArguments));
+
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(not(feature = "driver_manager_test_lib"), ignore)]
+ fn test_manifest_wrong_arch() {
+ let mut manifest_wrong_arch = simple_manifest();
+ manifest_wrong_arch
+ .get_mut("Driver")
+ .unwrap()
+ .get_mut("shared")
+ .unwrap()
+ .as_table_mut()
+ .unwrap()
+ .clear();
+ manifest_wrong_arch
+ .get_mut("Driver")
+ .unwrap()
+ .get_mut("shared")
+ .unwrap()
+ .as_table_mut()
+ .unwrap()
+ .insert("non-existing".into(), "path/to/bad/driver.so".into());
+
+ let (tmp_dir, manifest_path) =
+ write_manifest_to_tempfile(PathBuf::from("sqlite.toml"),
manifest_wrong_arch);
+
+ assert!(ManagedDriver::find_load_from_name(
+ manifest_path,
+ None,
+ AdbcVersion::V100,
+ LOAD_FLAG_DEFAULT
+ )
+ .is_err_and(|e| e.status == Status::InvalidArguments));
+
+ tmp_dir
+ .close()
+ .expect("Failed to close/remove temporary directory");
+ }
+
+ #[test]
+ #[cfg_attr(
+ not(all(
+ feature = "driver_manager_test_lib",
+ feature = "driver_manager_test_manifest_user"
+ )),
+ ignore
+ )]
+ fn test_manifest_user_config() {
+ assert!(ManagedDriver::find_load_from_name(
+ "adbc-test-sqlite",
+ None,
+ AdbcVersion::V110,
+ LOAD_FLAG_DEFAULT
+ )
+ .is_err());
Review Comment:
`.unwrap_err()` without the assert works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
