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<libloading::Library>,
 }
 
+#[derive(Default)]
+struct DriverInfo {
+    manifest_file: std::path::PathBuf,
+    lib_path: std::path::PathBuf,
+    entrypoint: Option<Vec<u8>>,
+    // 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<Self> {
+        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::<Table>()
+            .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<libloading::Library>,
 }
 
+#[derive(Default)]
+struct DriverInfo {
+    manifest_file: std::path::PathBuf,
+    lib_path: std::path::PathBuf,
+    entrypoint: Option<Vec<u8>>,
+    // 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<Self> {
+        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::<Table>()
+            .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:
   We also get here if `Driver` is not a table.



##########
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:
   `find_driver` is not a public method



##########
rust/core/src/driver_manager.rs:
##########
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
     _library: Option<libloading::Library>,
 }
 
+#[derive(Default)]
+struct DriverInfo {
+    manifest_file: std::path::PathBuf,

Review Comment:
   Do we really need to keep this path or do we only need it to construct a 
`DriverInfo`?



##########
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

Review Comment:
   ```suggestion
       ///      we defer to the previous logic in 
[`Self::load_dynamic_from_filename`] which will attempt to
   ```



##########
rust/core/src/driver_manager.rs:
##########
@@ -161,8 +170,94 @@ struct ManagedDriverInner {
     _library: Option<libloading::Library>,
 }
 
+#[derive(Default)]
+struct DriverInfo {
+    manifest_file: std::path::PathBuf,
+    lib_path: std::path::PathBuf,
+    entrypoint: Option<Vec<u8>>,
+    // 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<Self> {
+        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::<Table>()
+            .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:
   Should we error here on an invalid type too?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to