Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade merged PR #3099: URL: https://github.com/apache/arrow-adbc/pull/3099 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on PR #3099: URL: https://github.com/apache/arrow-adbc/pull/3099#issuecomment-3063631325 @mbrobbel any further comments here? Or are we looking good? :smile: -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2201043290
##
rust/core/src/driver_manager.rs:
##
@@ -1310,3 +1645,506 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+#[cfg(target_os = "windows")]
+extern crate windows_sys as windows;
Review Comment:
updated to use `use`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
eitsupi commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2199534491
##
rust/core/src/driver_manager.rs:
##
@@ -1310,3 +1645,506 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+#[cfg(target_os = "windows")]
+extern crate windows_sys as windows;
Review Comment:
I'm not an expert on this, but at least here I think there's no need to use
`extern crate` and just `use` is sufficient.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2198840904
##
rust/core/src/driver_manager.rs:
##
@@ -1310,3 +1645,506 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+#[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;
+
+// adapted from
https://github.com/dirs-dev/dirs-sys-rs/blob/main/src/lib.rs#L150
+fn user_config_dir() -> Option {
+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 {
+#[cfg(target_os = "windows")]
Review Comment:
after a bit of research it looks like the preferred way is the `target_os`
version, so i'll standardize on that
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2198838065
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
+lib_path: std::path::PathBuf,
+entrypoint: Option>,
+// TODO: until we add more logging these are unused so we'll leave
+// them out until such time that we do so.
+// driver_name: String,
+// version: String,
+// source: String,
+}
+
+impl DriverInfo {
+fn load_driver_manifest(&self) -> Result {
+let contents =
fs::read_to_string(self.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+self.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+// leave these out until we add logging that would actually utilize
them
+// let driver_name = get_optional_key(&manifest, "name");
+// let version = get_optional_key(&manifest, "version");
+// let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v|
v.get("shared")) {
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
Review Comment:
how is the updated version?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2198836790
##
rust/core/src/driver_manager.rs:
##
@@ -184,6 +279,59 @@ impl ManagedDriver {
Ok(ManagedDriver { inner })
}
+/// Load a driver either by name, filename, path, or via locating a toml
manifest file.
+/// The `load_flags` control what directories are searched to locate a
manifest.
+/// The `entrypoint` allows customizing the name of the driver
initialization function
+/// if it is not the default `AdbcDriverInit` and isn't described in the
loaded manifest.
+/// If not provided, an entrypoint will be searched for based on the
driver's name.
+/// The `version` defines the ADBC revision to attempt to initialize.
+///
+/// The full logic used here is as follows:
+/// - if `name` has an extension: it is treated as a filename. If the
load_flags does not
+///contain `LOAD_FLAG_ALLOW_RELATIVE_PATHS`, then relative paths will
be rejected.
+///- if the extension is `toml` then we attempt to load the Driver
Manifest, otherwise
+/// we defer to the previous logic in load_dynamic_from_filename
which will attempt to
+/// load the library
+/// - if `name` does not have an extension but is an absolute path: we
first check to see
+///if there is an existing file with the same name that *does* have a
"toml" extension,
+///attempting to load that if it exists. Otherwise we just pass it to
load_dynamic_from_filename.
+/// - Finally, if there's no extension and it is not an absolute path, we
call `find_driver`
Review Comment:
how's the updated version?
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
+lib_path: std::path::PathBuf,
+entrypoint: Option>,
+// TODO: until we add more logging these are unused so we'll leave
+// them out until such time that we do so.
+// driver_name: String,
+// version: String,
+// source: String,
+}
+
+impl DriverInfo {
+fn load_driver_manifest(&self) -> Result {
+let contents =
fs::read_to_string(self.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+self.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+// leave these out until we add logging that would actually utilize
them
+// let driver_name = get_optional_key(&manifest, "name");
+// let version = get_optional_key(&manifest, "version");
+// let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v|
v.get("shared")) {
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
+.unwrap_or_default(),
+);
+}
+}
+
+if lib_path.as_os_str().is_empty() {
+return Err(Error::with_message_and_status(
+format!(
+"Manifest '{}' missing Driver.shared key of
{os}_{arch}{extra}",
Review Comment:
How is the updated verison?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2198812373
##
rust/core/src/driver_manager.rs:
##
@@ -1310,3 +1645,506 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+#[cfg(target_os = "windows")]
+extern crate windows_sys as windows;
Review Comment:
I don't know rust well enough, I just pulled this from the original
implementation that I found in `dirs`. Would i just replace this as `use
windows_sys as windows;`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2198810545
##
rust/core/Cargo.toml:
##
@@ -16,31 +16,39 @@
# under the License.
[package]
-name = "adbc_core"
+authors.workspace = true
+categories.workspace = true
description = "Public abstract API, driver manager and driver exporter"
-version.workspace = true
+documentation.workspace = true
edition.workspace = true
-rust-version.workspace = true
-authors.workspace = true
+homepage.workspace = true
+keywords.workspace = true
license.workspace = true
+name = "adbc_core"
readme = "../README.md"
-documentation.workspace = true
-homepage.workspace = true
repository.workspace = true
-keywords.workspace = true
-categories.workspace = true
+rust-version.workspace = true
+version.workspace = true
[features]
default = []
-driver_manager = ["dep:libloading"]
+driver_manager = ["dep:toml", "dep:libloading", "dep:windows-sys",
"dep:windows-registry"]
+driver_manager_test_lib = ["driver_manager"]
+driver_manager_test_manifest_user = ["driver_manager_test_lib"]
[dependencies]
arrow-array.workspace = true
arrow-schema.workspace = true
-libloading = { version = "0.8", optional = true }
+libloading = {version = "0.8", optional = true}
+toml = { version = "0.9.0", optional = true }
Review Comment:
I got it to work! huzzah!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2196745713
##
rust/core/src/driver_manager.rs:
##
@@ -184,6 +279,59 @@ impl ManagedDriver {
Ok(ManagedDriver { inner })
}
+/// Load a driver either by name, filename, path, or via locating a toml
manifest file.
+/// The `load_flags` control what directories are searched to locate a
manifest.
+/// The `entrypoint` allows customizing the name of the driver
initialization function
+/// if it is not the default `AdbcDriverInit` and isn't described in the
loaded manifest.
+/// If not provided, an entrypoint will be searched for based on the
driver's name.
+/// The `version` defines the ADBC revision to attempt to initialize.
+///
+/// The full logic used here is as follows:
+/// - if `name` has an extension: it is treated as a filename. If the
load_flags does not
+///contain `LOAD_FLAG_ALLOW_RELATIVE_PATHS`, then relative paths will
be rejected.
+///- if the extension is `toml` then we attempt to load the Driver
Manifest, otherwise
+/// we defer to the previous logic in load_dynamic_from_filename
which will attempt to
+/// load the library
+/// - if `name` does not have an extension but is an absolute path: we
first check to see
+///if there is an existing file with the same name that *does* have a
"toml" extension,
+///attempting to load that if it exists. Otherwise we just pass it to
load_dynamic_from_filename.
+/// - Finally, if there's no extension and it is not an absolute path, we
call `find_driver`
Review Comment:
Mentioning a function that's not visible in the docs could be confusing to
users.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2196740610
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
+lib_path: std::path::PathBuf,
+entrypoint: Option>,
+// TODO: until we add more logging these are unused so we'll leave
+// them out until such time that we do so.
+// driver_name: String,
+// version: String,
+// source: String,
+}
+
+impl DriverInfo {
+fn load_driver_manifest(&self) -> Result {
+let contents =
fs::read_to_string(self.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+self.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+// leave these out until we add logging that would actually utilize
them
+// let driver_name = get_optional_key(&manifest, "name");
+// let version = get_optional_key(&manifest, "version");
+// let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v|
v.get("shared")) {
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
+.unwrap_or_default(),
+);
+}
+}
+
+if lib_path.as_os_str().is_empty() {
+return Err(Error::with_message_and_status(
+format!(
+"Manifest '{}' missing Driver.shared key of
{os}_{arch}{extra}",
Review Comment:
The error states that the key is missing, but that's not true when the value
is empty i.e. it should be a different error message for that case.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2196739320
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
+lib_path: std::path::PathBuf,
+entrypoint: Option>,
+// TODO: until we add more logging these are unused so we'll leave
+// them out until such time that we do so.
+// driver_name: String,
+// version: String,
+// source: String,
+}
+
+impl DriverInfo {
+fn load_driver_manifest(&self) -> Result {
+let contents =
fs::read_to_string(self.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+self.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+// leave these out until we add logging that would actually utilize
them
+// let driver_name = get_optional_key(&manifest, "name");
+// let version = get_optional_key(&manifest, "version");
+// let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v|
v.get("shared")) {
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
Review Comment:
Ignoring invalid types could cause confusion by users, so I would suggest to
always report these as errors.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2196736783
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
Review Comment:
I often find that types that are always in a valid state are easier to use
and reason about than types that have internal invariants that always must be
checked when using them.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
eitsupi commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2196247842
##
rust/core/src/driver_manager.rs:
##
@@ -1310,3 +1645,506 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+#[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;
+
+// adapted from
https://github.com/dirs-dev/dirs-sys-rs/blob/main/src/lib.rs#L150
+fn user_config_dir() -> Option {
+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 {
+#[cfg(target_os = "windows")]
Review Comment:
As `#[cfg(target_os = "windows")]` and `#[cfg(windows)]` are mixed, it may
look better to unify one or the other.
(I did not know that we could write `#[cfg(windows)]`!)
##
.github/workflows/rust.yml:
##
@@ -127,10 +127,15 @@ jobs:
if: matrix.os == 'macos-latest'
run: |
echo "DYLD_LIBRARY_PATH=/opt/homebrew/opt/sqlite/lib:${{
github.workspace }}/local/lib:$DYLD_LIBRARY_PATH" >> "$GITHUB_ENV"
+ echo "ADBC_DRIVER_MANAGER_TEST_LIB=${{ github.workspace
}}/local/lib/libadbc_driver_sqlite.dylib" >> "$GITHUB_ENV"
- name: Set dynamic linker path
if: matrix.os == 'macos-13'
run: |
echo "DYLD_LIBRARY_PATH=/usr/local/opt/sqlite/lib:${{
github.workspace }}/local/lib:$DYLD_LIBRARY_PATH" >> "$GITHUB_ENV"
+ echo "ADBC_DRIVER_MANAGER_TEST_LIB=${{ github.workspace
}}/local/lib/libadbc_driver_sqlite.dylib" >> "$GITHUB_ENV"
+ - name: Set test env var
+if: matrix.os == 'ubuntu-latest'
Review Comment:
This may be more appropriate.
```suggestion
if: runner.os == 'Linux'
```
##
rust/core/src/driver_manager.rs:
##
@@ -1310,3 +1645,506 @@ impl Drop for ManagedStatement {
unsafe { method(statement.deref_mut(), null_mut()) };
}
}
+
+#[cfg(target_os = "windows")]
+extern crate windows_sys as windows;
Review Comment:
Is this `extern crate` necessary?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195964267
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
+lib_path: std::path::PathBuf,
+entrypoint: Option>,
+// TODO: until we add more logging these are unused so we'll leave
+// them out until such time that we do so.
+// driver_name: String,
+// version: String,
+// source: String,
+}
+
+impl DriverInfo {
+fn load_driver_manifest(&self) -> Result {
+let contents =
fs::read_to_string(self.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+self.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+// leave these out until we add logging that would actually utilize
them
+// let driver_name = get_optional_key(&manifest, "name");
+// let version = get_optional_key(&manifest, "version");
+// let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v|
v.get("shared")) {
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
Review Comment:
personally, i'm fine with ignoring the invalid type and treating it as an
empty string. thoughts?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195947326
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
+lib_path: std::path::PathBuf,
+entrypoint: Option>,
+// TODO: until we add more logging these are unused so we'll leave
+// them out until such time that we do so.
+// driver_name: String,
+// version: String,
+// source: String,
+}
+
+impl DriverInfo {
+fn load_driver_manifest(&self) -> Result {
+let contents =
fs::read_to_string(self.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+self.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+// leave these out until we add logging that would actually utilize
them
+// let driver_name = get_optional_key(&manifest, "name");
+// let version = get_optional_key(&manifest, "version");
+// let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v|
v.get("shared")) {
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
+.unwrap_or_default(),
+);
+}
+}
+
+if lib_path.as_os_str().is_empty() {
+return Err(Error::with_message_and_status(
+format!(
+"Manifest '{}' missing Driver.shared key of
{os}_{arch}{extra}",
+lib_path.display()
+),
+Status::InvalidArguments,
+));
+}
+
+let entrypoint_val = manifest
+.get("Driver")
+.and_then(Value::as_table)
+.and_then(|t| t.get("entrypoint"));
+
+if let Some(entry) = entrypoint_val {
+if !entry.is_str() {
+return Err(Error::with_message_and_status(
+"Driver entrypoint must be a string".to_string(),
Review Comment:
How? If `Driver` isn't a table then our `lib_path` would be empty and we
would have already returned that error. We can't get here if `Driver` isn't a
table.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195944656
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
+lib_path: std::path::PathBuf,
+entrypoint: Option>,
+// TODO: until we add more logging these are unused so we'll leave
+// them out until such time that we do so.
+// driver_name: String,
+// version: String,
+// source: String,
+}
+
+impl DriverInfo {
+fn load_driver_manifest(&self) -> Result {
+let contents =
fs::read_to_string(self.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+self.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+// leave these out until we add logging that would actually utilize
them
+// let driver_name = get_optional_key(&manifest, "name");
+// let version = get_optional_key(&manifest, "version");
+// let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v|
v.get("shared")) {
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
+.unwrap_or_default(),
+);
+}
+}
+
+if lib_path.as_os_str().is_empty() {
+return Err(Error::with_message_and_status(
+format!(
+"Manifest '{}' missing Driver.shared key of
{os}_{arch}{extra}",
Review Comment:
What's the benefit here of doing the match of `Some` and `None` there? It
seems like it only complicates the code or introduces duplication rather than
making it simpler. If the key exists but the value is an empty string, we want
this error to happen still. I'm probably missing what you're suggesting, can
you clarify?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195925756
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
Review Comment:
mostly just for logging so that we can know where a particular `DriverInfo`
came from. In theory it wouldn't be bad to leave it out, it would just mean
that if we use `DriverInfo` elsewhere we'd likely have to add it back.
I'll update this to remove the path from the `DriverInfo` object for now
then.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195919109
##
rust/core/src/driver_manager.rs:
##
@@ -184,6 +279,59 @@ impl ManagedDriver {
Ok(ManagedDriver { inner })
}
+/// Load a driver either by name, filename, path, or via locating a toml
manifest file.
+/// The `load_flags` control what directories are searched to locate a
manifest.
+/// The `entrypoint` allows customizing the name of the driver
initialization function
+/// if it is not the default `AdbcDriverInit` and isn't described in the
loaded manifest.
+/// If not provided, an entrypoint will be searched for based on the
driver's name.
+/// The `version` defines the ADBC revision to attempt to initialize.
+///
+/// The full logic used here is as follows:
+/// - if `name` has an extension: it is treated as a filename. If the
load_flags does not
+///contain `LOAD_FLAG_ALLOW_RELATIVE_PATHS`, then relative paths will
be rejected.
+///- if the extension is `toml` then we attempt to load the Driver
Manifest, otherwise
+/// we defer to the previous logic in load_dynamic_from_filename
which will attempt to
+/// load the library
+/// - if `name` does not have an extension but is an absolute path: we
first check to see
+///if there is an existing file with the same name that *does* have a
"toml" extension,
+///attempting to load that if it exists. Otherwise we just pass it to
load_dynamic_from_filename.
+/// - Finally, if there's no extension and it is not an absolute path, we
call `find_driver`
Review Comment:
It's not intended to be a public method, we're just describing the logic
that if given a name that doesn't have an extension, we fallback to that method.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195917720
##
rust/core/Cargo.toml:
##
@@ -16,31 +16,39 @@
# under the License.
[package]
-name = "adbc_core"
+authors.workspace = true
+categories.workspace = true
description = "Public abstract API, driver manager and driver exporter"
-version.workspace = true
+documentation.workspace = true
edition.workspace = true
-rust-version.workspace = true
-authors.workspace = true
+homepage.workspace = true
+keywords.workspace = true
license.workspace = true
+name = "adbc_core"
readme = "../README.md"
-documentation.workspace = true
-homepage.workspace = true
repository.workspace = true
-keywords.workspace = true
-categories.workspace = true
+rust-version.workspace = true
+version.workspace = true
[features]
default = []
-driver_manager = ["dep:libloading"]
+driver_manager = ["dep:toml", "dep:libloading", "dep:windows-sys",
"dep:windows-registry"]
+driver_manager_test_lib = ["driver_manager"]
+driver_manager_test_manifest_user = ["driver_manager_test_lib"]
[dependencies]
arrow-array.workspace = true
arrow-schema.workspace = true
-libloading = { version = "0.8", optional = true }
+libloading = {version = "0.8", optional = true}
+toml = { version = "0.9.0", optional = true }
Review Comment:
I think you can use https://docs.rs/toml/latest/toml/de/type.DeTable.html
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195908470
##
rust/core/Cargo.toml:
##
@@ -16,31 +16,39 @@
# under the License.
[package]
-name = "adbc_core"
+authors.workspace = true
+categories.workspace = true
description = "Public abstract API, driver manager and driver exporter"
-version.workspace = true
+documentation.workspace = true
edition.workspace = true
-rust-version.workspace = true
-authors.workspace = true
+homepage.workspace = true
+keywords.workspace = true
license.workspace = true
+name = "adbc_core"
readme = "../README.md"
-documentation.workspace = true
-homepage.workspace = true
repository.workspace = true
-keywords.workspace = true
-categories.workspace = true
+rust-version.workspace = true
+version.workspace = true
[features]
default = []
-driver_manager = ["dep:libloading"]
+driver_manager = ["dep:toml", "dep:libloading", "dep:windows-sys",
"dep:windows-registry"]
+driver_manager_test_lib = ["driver_manager"]
+driver_manager_test_manifest_user = ["driver_manager_test_lib"]
[dependencies]
arrow-array.workspace = true
arrow-schema.workspace = true
-libloading = { version = "0.8", optional = true }
+libloading = {version = "0.8", optional = true}
+toml = { version = "0.9.0", optional = true }
Review Comment:
looks like the update to 0.9.0 made it so that `toml::Value` and
`toml::Table` require the `serde` feature :frowning_face:
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195875600
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
+lib_path: std::path::PathBuf,
+entrypoint: Option>,
+// TODO: until we add more logging these are unused so we'll leave
+// them out until such time that we do so.
+// driver_name: String,
+// version: String,
+// source: String,
+}
+
+impl DriverInfo {
+fn load_driver_manifest(&self) -> Result {
+let contents =
fs::read_to_string(self.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+self.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+// leave these out until we add logging that would actually utilize
them
+// let driver_name = get_optional_key(&manifest, "name");
+// let version = get_optional_key(&manifest, "version");
+// let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v|
v.get("shared")) {
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
+.unwrap_or_default(),
+);
+}
+}
+
+if lib_path.as_os_str().is_empty() {
+return Err(Error::with_message_and_status(
+format!(
+"Manifest '{}' missing Driver.shared key of
{os}_{arch}{extra}",
Review Comment:
We also get here if the key exists but the value is an empty string. Instead
of using the empty `lib_path` you could match both `Some` and `None` in
`manifest.get("Driver")`.
##
rust/core/Cargo.toml:
##
@@ -16,31 +16,39 @@
# under the License.
[package]
-name = "adbc_core"
+authors.workspace = true
+categories.workspace = true
description = "Public abstract API, driver manager and driver exporter"
-version.workspace = true
+documentation.workspace = true
edition.workspace = true
-rust-version.workspace = true
-authors.workspace = true
+homepage.workspace = true
+keywords.workspace = true
license.workspace = true
+name = "adbc_core"
readme = "../README.md"
-documentation.workspace = true
-homepage.workspace = true
repository.workspace = true
-keywords.workspace = true
-categories.workspace = true
+rust-version.workspace = true
+version.workspace = true
[features]
default = []
-driver_manager = ["dep:libloading"]
+driver_manager = ["dep:toml", "dep:libloading", "dep:windows-sys",
"dep:windows-registry"]
+driver_manager_test_lib = ["driver_manager"]
+driver_manager_test_manifest_user = ["driver_manager_test_lib"]
[dependencies]
arrow-array.workspace = true
arrow-schema.workspace = true
-libloading = { version = "0.8", optional = true }
+libloading = {version = "0.8", optional = true}
+toml = { version = "0.9.0", optional = true }
Review Comment:
This crate has a [default](https://docs.rs/crate/toml/latest/features)
`serde` feature.
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+struct DriverInfo {
+manifest_file: std::path::PathBuf,
+lib_path: std::path::PathBuf,
+entrypoint: Option>,
+// TODO: until we add more logging these are unused so we'll leave
+// them out until such time that we do so.
+// driver_name: String,
+// version: String,
+// source: String,
+}
+
+impl DriverInfo {
+fn load_driver_manifest(&self) -> Result {
+let contents =
fs::read_to_string(self.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+self.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+// leave these out until we add logging that would actually utilize
them
+// let driver_name =
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195632771
##
rust/core/src/driver_manager.rs:
##
@@ -316,6 +493,98 @@ impl Driver for ManagedDriver {
}
}
+#[cfg(windows)]
+fn load_driver_from_registry(
+root: &windows_registry::Key,
+driver_name: &OsStr,
+entrypoint: Option<&[u8]>,
+) -> Result {
+const ADBC_DRIVER_REGISTRY: &str = "SOFTWARE\\ADBC\\Drivers";
+let drivers_key = root.open(ADBC_DRIVER_REGISTRY)?;
+
+Ok(DriverInfo {
+driver_name: drivers_key.get_string("name").unwrap_or_default(),
+entrypoint: entrypoint
+.or_else(|| drivers_key.get_string("entrypoint").map(|e|
e.into_bytes())),
+version: drivers_key.get_string("version").unwrap_or_default(),
+source: drivers_key.get_string("source").unwrap_or_default(),
+lib_path:
PathBuf::from(drivers_key.get_string("driver").unwrap_or_default()),
+})
+}
+
+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: &DriverInfo) -> Result {
+let contents =
fs::read_to_string(info.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+info.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+let driver_name = get_optional_key(&manifest, "name");
+let version = get_optional_key(&manifest, "version");
+let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v| v.get("shared"))
{
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
+.unwrap_or_default(),
+);
+}
+}
+
+if lib_path.as_os_str().is_empty() {
+return Err(Error::with_message_and_status(
+format!(
+"Manifest '{}' missing appropriate path for current arch",
+lib_path.display()
+),
+Status::InvalidArguments,
+));
+}
+
+let entrypoint = manifest
+.get("Driver")
+.and_then(Value::as_table)
+.and_then(|t| t.get("entrypoint"))
+.and_then(Value::as_str)
Review Comment:
modified the logic to error on a non-string type
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195632175
##
rust/core/src/driver_manager.rs:
##
@@ -316,6 +493,98 @@ impl Driver for ManagedDriver {
}
}
+#[cfg(windows)]
+fn load_driver_from_registry(
+root: &windows_registry::Key,
+driver_name: &OsStr,
+entrypoint: Option<&[u8]>,
+) -> Result {
+const ADBC_DRIVER_REGISTRY: &str = "SOFTWARE\\ADBC\\Drivers";
+let drivers_key = root.open(ADBC_DRIVER_REGISTRY)?;
+
+Ok(DriverInfo {
+driver_name: drivers_key.get_string("name").unwrap_or_default(),
+entrypoint: entrypoint
+.or_else(|| drivers_key.get_string("entrypoint").map(|e|
e.into_bytes())),
+version: drivers_key.get_string("version").unwrap_or_default(),
+source: drivers_key.get_string("source").unwrap_or_default(),
+lib_path:
PathBuf::from(drivers_key.get_string("driver").unwrap_or_default()),
+})
+}
+
+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: &DriverInfo) -> Result {
+let contents =
fs::read_to_string(info.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+info.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+let driver_name = get_optional_key(&manifest, "name");
+let version = get_optional_key(&manifest, "version");
+let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v| v.get("shared"))
{
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
+.unwrap_or_default(),
+);
+}
+}
+
+if lib_path.as_os_str().is_empty() {
+return Err(Error::with_message_and_status(
+format!(
+"Manifest '{}' missing appropriate path for current arch",
Review Comment:
updated the error message
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195630646
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,20 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+#[allow(dead_code)]
Review Comment:
updated to remove the fields but commented them out so we don't forget about
them
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195631533
##
rust/core/src/driver_manager.rs:
##
@@ -184,6 +205,41 @@ 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 load_from_name(
+name: impl AsRef,
+entrypoint: Option<&[u8]>,
+version: AdbcVersion,
Review Comment:
added to the docs
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195607298
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,20 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+#[allow(dead_code)]
Review Comment:
the object itself isn't public, so I guess we should just remove them for
now until they are used elsewhere, likely when we add more logging stuff.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195607298
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,20 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+#[allow(dead_code)]
Review Comment:
the object itself isn't public, so I guess we should just remove them for
now until they are used elsewhere
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195531070
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,20 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+#[allow(dead_code)]
Review Comment:
If we aren't using those fields maybe we should 1) add `pub` methods that
return them 2) mark them `pub` or 3) remove them?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195209863
##
rust/core/src/driver_manager.rs:
##
@@ -184,6 +205,41 @@ 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 load_from_name(
+name: impl AsRef,
+entrypoint: Option<&[u8]>,
+version: AdbcVersion,
+load_flags: LoadFlags,
+) -> Result {
+let driver_path = Path::new(name.as_ref());
+let allow_relative = load_flags & LOAD_FLAG_ALLOW_RELATIVE_PATHS != 0;
+if let Some(ext) = driver_path.extension() {
+if !allow_relative && driver_path.is_relative() {
+return Err(Error::with_message_and_status(
+"Relative paths are not allowed",
+Status::InvalidArguments,
+));
+}
+
+if ext == "toml" {
+Self::load_from_manifest(driver_path, entrypoint, version)
+} else {
+Self::load_dynamic_from_filename(driver_path, entrypoint,
version)
+}
+} else if driver_path.is_absolute() {
+let toml_path = driver_path.with_extension("toml");
+if toml_path.is_file() {
+return Self::load_from_manifest(&toml_path, entrypoint,
version);
+}
+
+Self::load_dynamic_from_filename(driver_path, entrypoint, version)
+} else {
+Self::find_driver(driver_path, entrypoint, version, load_flags)
+}
Review Comment:
I'll update the doc string. I don't think that adding new methods would
significantly improve understanding the logic. But i'll take a look
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2195205169
##
rust/core/src/driver_manager.rs:
##
@@ -161,8 +170,20 @@ struct ManagedDriverInner {
_library: Option,
}
+#[derive(Default)]
+#[allow(dead_code)]
Review Comment:
We needed this because we aren't directly reading version/source/publisher
anywhere, so the compiler got mad without this.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2194367106
##
rust/core/src/driver_manager.rs:
##
@@ -316,6 +493,98 @@ impl Driver for ManagedDriver {
}
}
+#[cfg(windows)]
+fn load_driver_from_registry(
+root: &windows_registry::Key,
+driver_name: &OsStr,
+entrypoint: Option<&[u8]>,
+) -> Result {
+const ADBC_DRIVER_REGISTRY: &str = "SOFTWARE\\ADBC\\Drivers";
+let drivers_key = root.open(ADBC_DRIVER_REGISTRY)?;
+
+Ok(DriverInfo {
+driver_name: drivers_key.get_string("name").unwrap_or_default(),
+entrypoint: entrypoint
+.or_else(|| drivers_key.get_string("entrypoint").map(|e|
e.into_bytes())),
+version: drivers_key.get_string("version").unwrap_or_default(),
+source: drivers_key.get_string("source").unwrap_or_default(),
+lib_path:
PathBuf::from(drivers_key.get_string("driver").unwrap_or_default()),
+})
+}
+
+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: &DriverInfo) -> Result {
+let contents =
fs::read_to_string(info.manifest_file.as_path()).map_err(|e| {
+Error::with_message_and_status(
+format!(
+"Could not read manifest '{}': {e}",
+info.manifest_file.display()
+),
+Status::InvalidArguments,
+)
+})?;
+
+let manifest = contents
+.parse::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
+
+let driver_name = get_optional_key(&manifest, "name");
+let version = get_optional_key(&manifest, "version");
+let source = get_optional_key(&manifest, "source");
+let (os, arch, extra) = arch_triplet();
+
+let mut lib_path = PathBuf::default();
+if let Some(driver) = manifest.get("Driver").and_then(|v| v.get("shared"))
{
+if driver.is_str() {
+lib_path = PathBuf::from(driver.as_str().unwrap_or_default());
+} else if driver.is_table() {
+lib_path = PathBuf::from(
+driver
+.get(format!("{os}_{arch}{extra}"))
+.and_then(Value::as_str)
+.unwrap_or_default(),
+);
+}
+}
+
+if lib_path.as_os_str().is_empty() {
+return Err(Error::with_message_and_status(
+format!(
+"Manifest '{}' missing appropriate path for current arch",
+lib_path.display()
+),
+Status::InvalidArguments,
+));
+}
+
+let entrypoint = manifest
+.get("Driver")
+.and_then(Value::as_table)
+.and_then(|t| t.get("entrypoint"))
+.and_then(Value::as_str)
Review Comment:
Don't we want to error on an invalid type?
##
rust/core/src/driver_manager.rs:
##
@@ -316,6 +493,98 @@ impl Driver for ManagedDriver {
}
}
+#[cfg(windows)]
+fn load_driver_from_registry(
+root: &windows_registry::Key,
+driver_name: &OsStr,
+entrypoint: Option<&[u8]>,
+) -> Result {
+const ADBC_DRIVER_REGISTRY: &str = "SOFTWARE\\ADBC\\Drivers";
+let drivers_key = root.open(ADBC_DRIVER_REGISTRY)?;
+
+Ok(DriverInfo {
+driver_name: drivers_key.get_string("name").unwrap_or_default(),
+entrypoint: entrypoint
+.or_else(|| drivers_key.get_string("entrypoint").map(|e|
e.into_bytes())),
+version: drivers_key.get_string("version").unwrap_or_default(),
+source: drivers_key.get_string("source").unwrap_or_default(),
+lib_path:
PathBuf::from(drivers_key.get_string("driver").unwrap_or_default()),
+})
+}
+
+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: &DriverInfo) -> Result {
Review Comment:
Maybe this should be a method of `DriverInfo`?
##
rust/core/src/driver_manager.rs:
##
@@ -316,6 +493,98 @@ impl Driver for ManagedDriver {
}
}
+#[cfg(windows)]
+fn load_driver_from_registry(
+root: &windows_registry::Key,
+driver_name: &OsStr,
+entrypoint: Option<&[u8]>,
+) -> Result {
+const ADBC_DRIVER_REGISTRY: &str = "SOFTWARE\\ADBC\\Drivers";
+let drivers_key = root.open(ADBC_DRIVER_REGISTRY)?;
+
+Ok(DriverInfo {
+driver_name: drivers_key.get_string("name").unwrap_or_default(),
+entrypoint: entrypoint
+.or_else(|| drivers_key.get_string("entrypoint").map(|e|
e.into_bytes())),
+version: drivers_key.get_string("version").unwrap_or_default(),
+source: drivers_key.get_string("source").unwrap_or_default(),
+lib_path:
PathBuf::from(drivers_key.get_string("driver").unwrap_or_defau
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
eitsupi commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2193952055
##
rust/core/Cargo.toml:
##
@@ -16,31 +16,41 @@
# under the License.
[package]
-name = "adbc_core"
+authors.workspace = true
+categories.workspace = true
description = "Public abstract API, driver manager and driver exporter"
-version.workspace = true
+documentation.workspace = true
edition.workspace = true
-rust-version.workspace = true
-authors.workspace = true
+homepage.workspace = true
+keywords.workspace = true
license.workspace = true
+name = "adbc_core"
readme = "../README.md"
-documentation.workspace = true
-homepage.workspace = true
repository.workspace = true
-keywords.workspace = true
-categories.workspace = true
+rust-version.workspace = true
+version.workspace = true
[features]
default = []
-driver_manager = ["dep:libloading"]
+driver_manager = ["dep:toml", "dep:libloading", "dep:windows-sys",
"dep:windows-registry"]
+driver_manager_test_lib = ["driver_manager"]
+driver_manager_test_manifest_user = ["driver_manager_test_lib"]
[dependencies]
arrow-array.workspace = true
arrow-schema.workspace = true
-libloading = { version = "0.8", optional = true }
+libloading = {version = "0.8", optional = true}
+toml = {version = "0.8.23", default-features = false, features = [
+"parse", "display",
+], optional = true}
[dev-dependencies]
arrow-select.workspace = true
+tempfile = "3.20.0"
[package.metadata.docs.rs]
all-features = true
+
+[target.windows.dependencies]
Review Comment:
Maybe this looks better?
https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies
```suggestion
[target.'cfg(windows)'.dependencies]
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
amoeba commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2193189638
##
ci/scripts/rust_test.sh:
##
@@ -26,5 +26,6 @@ export
LD_LIBRARY_PATH="${cpp_libs_dir}/lib:${LD_LIBRARY_PATH:-}"
export DYLD_LIBRARY_PATH="${cpp_libs_dir}/lib:${DYLD_LIBRARY_PATH:-}"
pushd "${source_dir}"
+export
ADBC_DRIVER_MANAGER_TEST_LIB="${cpp_libs_dir}/lib/libadbc_driver_sqlite.so"
Review Comment:
```suggestion
case "$(uname)" in
Linux) EXT="so" ;;
Darwin) EXT="dylib" ;;
MINGW*|MSYS*) EXT="dll" ;;
esac
export
ADBC_DRIVER_MANAGER_TEST_LIB="${cpp_libs_dir}/lib/libadbc_driver_sqlite.${EXT:-}"
```
##
ci/scripts/rust_test.sh:
##
@@ -26,5 +26,6 @@ export
LD_LIBRARY_PATH="${cpp_libs_dir}/lib:${LD_LIBRARY_PATH:-}"
export DYLD_LIBRARY_PATH="${cpp_libs_dir}/lib:${DYLD_LIBRARY_PATH:-}"
pushd "${source_dir}"
+export
ADBC_DRIVER_MANAGER_TEST_LIB="${cpp_libs_dir}/lib/libadbc_driver_sqlite.so"
Review Comment:
CI isn't exercising this script but do we want to try to stay as
crossplatform as possible?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2193016267
##
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::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
Review Comment:
given we have three people in favor of removing serde and using the original
logic, I'm gonna switch back to that unless someone objects.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
phillipleblanc commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2191548877
##
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::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
Review Comment:
In general, I favor libraries being thoughtful about which dependencies they
require to work. I'd also lean towards dropping serde to keep the original
parsing logic.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
lidavidm commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2191260835
##
rust/core/src/manifest.rs:
##
@@ -0,0 +1,391 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Driver manifest.
+
+use std::{
+collections::HashMap,
+fmt::Display,
+marker::PhantomData,
+path::{Path, PathBuf},
+str::FromStr,
+};
+
+use serde::de::{self, MapAccess, Visitor};
+use serde::{Deserialize, Deserializer};
+use toml::Value;
+use void::Void;
+
+use crate::{
+error::{Error, Status},
+options::AdbcVersion,
+};
+
+#[derive(Clone, Debug, Deserialize, PartialEq)]
+pub struct Manifest {
+/// Driver display name
+name: Option,
+
+/// Publisher Identifier
+publisher: Option,
+
+/// Driver license
+#[serde(default, deserialize_with = "de_from_str_opt")]
+license: Option,
+
+/// Driver Version
+version: Option,
+
+/// Source of the driver, e.g. a URL to the source code repository
+source: Option,
+
+/// ADBC
+#[serde(rename = "ADBC")]
+adbc: Option,
+
+/// Driver
+#[serde(rename = "Driver")]
+driver: Driver,
+
+/// Additional Fields
+#[serde(flatten)]
+additional: HashMap,
+}
+
+impl Manifest {
+pub fn name(&self) -> Option<&str> {
+self.name.as_deref()
+}
+
+pub fn publisher(&self) -> Option<&str> {
+self.publisher.as_deref()
+}
+
+pub fn license(&self) -> Option<&spdx::Expression> {
+self.license.as_ref()
+}
+
+pub fn version(&self) -> Option<&semver::Version> {
+self.version.as_ref()
+}
+
+pub fn source(&self) -> Option<&str> {
+self.source.as_deref()
+}
+
+pub fn adbc(&self) -> Option<&ADBC> {
+self.adbc.as_ref()
+}
+
+pub fn driver(&self) -> &Driver {
+&self.driver
+}
+
+pub fn additional(&self) -> &HashMap {
+&self.additional
+}
+}
+
+/// Deserialize an `Option` using the [`FromStr`] impl of `T`
+fn de_from_str_opt<'de, D, T: FromStr>(deserializer: D) ->
Result, D::Error>
+where
+D: Deserializer<'de>,
+{
+Optiondeserialize(deserializer).and_then(|value| {
+value
+.as_deref()
+.map(|value| T::from_str(value).map_err(de::Error::custom))
+.transpose()
+})
+}
+
+impl FromStr for Manifest {
+type Err = Error;
+
+fn from_str(input: &str) -> Result {
+toml::from_str(input)
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))
+}
+}
+
+fn string_or_struct<'de, T, D>(deserializer: D) -> Result
+where
+T: Deserialize<'de> + FromStr,
+D: Deserializer<'de>,
+{
+// visitor that forwards string types to T's `FromStr` impl
+// and forwards map types to T's `Deserialize` impl. The `PhantomData`
+// is to keep the compiler from complaining about T being an unused
+// generic type parameter. We need T in order to know the Value type
+// for the Visitor impl.
+struct StringOrStruct(PhantomData T>);
+
+impl<'de, T> Visitor<'de> for StringOrStruct
+where
+T: Deserialize<'de> + FromStr,
+{
+type Value = T;
+
+fn expecting(&self, formatter: &mut std::fmt::Formatter) ->
std::fmt::Result {
+formatter.write_str("string or map")
+}
+
+fn visit_str(self, value: &str) -> Result
+where
+E: de::Error,
+{
+Ok(FromStr::from_str(value).unwrap())
+}
+
+fn visit_map(self, map: M) -> Result
+where
+M: MapAccess<'de>,
+{
+// `MapAccessDeserializer` is a wrapper that turns a `MapAccess`
+// into a `Deserializer`, allowing it to be used as the input to
T's
+// `Deserialize` impl. T then deserializes itself using the entries
+// from the map visitor.
+
Deserialize::deserialize(de::value::MapAccessDeserializer::new(map))
+}
+}
+deserializer.deserialize_any(StringOrStruct(PhantomData))
+}
+
+/// ADBC Information
+#[derive(Clone, Debug, Deserialize, PartialEq)]
+pub struct ADBC {
+/// Supported ADBC spec version
+#[serde(deserialize
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
lidavidm commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2191205764
##
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::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
Review Comment:
I suppose I favor fewer dependencies where possible. Also I don't think the
manifest struct should be exported in the first place.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on PR #3099: URL: https://github.com/apache/arrow-adbc/pull/3099#issuecomment-3046508053 I've also updated this to have the same windows registry logic that the C++ adbc_driver_manager has. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
felipecrv commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2190793924
##
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 {
+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:
Ha! Thanks for not adding the dependency. Just wanted to make sure you
checked the state-of-the-art for inspiration.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2190789902
##
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::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
Review Comment:
I've updated this to use the serde implementation that @mbrobbel suggested.
While it does make the implementation of `load_driver_manifest` in
`driver_manager.rs` much simpler, it is at the expense of the complexity of the
`manifest.rs` file itself and custom serde etc.
I'll let all of you decide what makes the most sense here as I'm not as
familiar with the benefits or drawbacks here. @phillipleblanc @lidavidm either
of you have an opinion here also?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2190488905
##
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 {
+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:
where do you think I figured out how to do it? :smile: Added a comment to
reference `dirs`. I didn't want to rely on an archived repo/dependency
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2190476059
##
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 {
+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 {
+#[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 {
+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'
+
+
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2190422881
##
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:
maybe just `load_from_name`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2190373384
##
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::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
Review Comment:
I don't think we can put the manifest handling behind a feature flag as it's
necessary for the driver_manager itself (or maybe we just make the
driver_manager feature depend on the manifest feature?)
I'll poke around with this, in the meantime you two (@felipecrv @mbrobbel)
can fight over whether or not to use `serde` as I don't know the lib enough to
know the pros/cons :smile: I'll go with whichever wins :)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2189337450
##
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::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
Review Comment:
> Is that maybe overkill given that we aren't looking to provide full
manifest interactions, just a simple parse to get the shared lib path?
By providing a `Manifest` definition (behind a feature flag) users can
easily use the other fields, because even if we don't use those other fields
here now, others might.
> How do I modify that to express that the `shared` member could be _either_
a string or a `map` that maps platform triples to a path?
https://serde.rs/string-or-struct.html
> 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.
`serde` doesn't include other data formats (`Deserializer` implementations)?
> better abstractions
Can you elaborate?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
phillipleblanc commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2187837806
##
rust/core/src/driver_manager.rs:
##
@@ -276,6 +351,23 @@ impl ManagedDriver {
Ok(database)
}
+
+fn load_driver_from_manifest(
+driver_path: &Path,
+entrypoint: Option<&[u8]>,
+version: AdbcVersion,
+) -> Result {
+let mut info = DriverInfo {
+manifest_file: driver_path.to_path_buf(),
+driver_name: String::new(),
+lib_path: PathBuf::new(),
+entrypoint: entrypoint.map(|e| e.to_vec()),
+version: String::new(),
+source: String::new(),
Review Comment:
You can also write the type name, i.e. `DriverInfo::default()` - this is
also a pedantic clippy lint:
https://rust-lang.github.io/rust-clippy/master/index.html#default_trait_access
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
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::()
+.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 {
+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 {
+#[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)
+})
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
paleolimbot commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2186039421
##
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 {
+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 {
+#[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 {
+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'
+
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2185659695
##
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::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
Review Comment:
Thanks for that! I was looking through it and just have two questions:
1. Is that maybe overkill given that we aren't looking to provide full
manifest interactions, just a simple parse to get the shared lib path?
2. How do I modify that to express that the `shared` member could be
*either* a string or a `map` that maps platform triples to a
path?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
mbrobbel commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2185124655
##
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::()
+.map_err(|e| Error::with_message_and_status(e.to_string(),
Status::InvalidArguments))?;
Review Comment:
I opened https://github.com/apache/arrow-adbc/pull/3101 with an alternative
approach to handle `Manifest` deserialization.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
lidavidm commented on code in PR #3099:
URL: https://github.com/apache/arrow-adbc/pull/3099#discussion_r2184115938
##
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:
Shouldn't this be something like `load_dynamic_from_name`?
##
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(
+name: impl AsRef,
+entrypoint: Option<&[u8]>,
+version: AdbcVersion,
+load_flags: LoadFlags,
+) -> Result {
+let driver_path = Path::new(name.as_ref());
+let allow_relative = load_flags & LOAD_FLAG_ALLOW_RELATIVE_PATHS != 0;
+let ext = driver_path.extension();
+if ext.is_some() {
Review Comment:
```suggestion
if let Some(ext) = driver_path.extension() {
```
There's generally little reason to use is_some.
##
rust/core/src/driver_manager.rs:
##
@@ -276,6 +351,23 @@ impl ManagedDriver {
Ok(database)
}
+
+fn load_driver_from_manifest(
+driver_path: &Path,
+entrypoint: Option<&[u8]>,
+version: AdbcVersion,
+) -> Result {
+let mut info = DriverInfo {
+manifest_file: driver_path.to_path_buf(),
+driver_name: String::new(),
+lib_path: PathBuf::new(),
+entrypoint: entrypoint.map(|e| e.to_vec()),
+version: String::new(),
+source: String::new(),
Review Comment:
I think it works to just write `Default::default()` instead of explicitly
calling `new`
##
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:
Instead of the extra dependency, could we just return a 3-tuple and concat
at runtime? (Or return a newtype 3-tuple with a custom Display to do the concat)
##
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<()> {
Review Comment:
I think it might be more idiomatic to take info by value and return it by
value (rather than requiring a mutable reference)
##
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<()> {
Review Comment:
You can then write
```rust
let info = load_driver_manifest(DriverInfo { ... })?;
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat(rust/core): add function to load driver manifests [arrow-adbc]
zeroshade commented on PR #3099: URL: https://github.com/apache/arrow-adbc/pull/3099#issuecomment-3033882449 CC @phillipleblanc -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
