felipecrv commented on code in PR #2918:
URL: https://github.com/apache/arrow-adbc/pull/2918#discussion_r2127760378
##########
go/adbc/drivermgr/adbc_driver_manager.cc:
##########
@@ -114,7 +125,187 @@ struct OwnedError {
}
};
+std::filesystem::path UserConfigDir() {
+ std::filesystem::path config_dir;
+#if defined(_WIN32)
+ auto dir = std::getenv("AppData");
+ if (dir) {
+ config_dir = std::filesystem::path(dir);
+ config_dir /= "ADBC/drivers";
+ }
+#elif defined(__APPLE__)
+ auto dir = std::getenv("HOME");
+ if (dir) {
+ config_dir = std::filesystem::path(dir);
+ config_dir /= "Library/Application Support/ADBC";
+ }
+#elif defined(__linux__)
+ auto dir = std::getenv("XDG_CONFIG_HOME");
+ if (!dir) {
+ dir = std::getenv("HOME");
+ if (dir) {
+ config_dir = std::filesystem::path(dir);
+ }
+ } else {
+ config_dir = std::filesystem::path(dir);
+ }
+
+ if (!config_dir.empty()) {
+ config_dir /= "adbc";
+ }
+#endif
+
+ return config_dir;
+}
+
// Driver state
+struct DriverInfo {
+ std::string manifest_file;
+ std::string driver_name;
+ std::string lib_path;
+ std::string entrypoint;
+
+ std::string version;
+ std::string source;
+};
+
+#ifdef _WIN32
+class RegistryKey {
+ public:
+ RegistryKey(HKEY root, const std::string_view subkey) noexcept
+ : root_(root), key_(nullptr), is_open_(false) {
+ auto result = RegOpenKeyExA(root_, subkey.data(), 0, KEY_READ, &key_);
+ is_open_ = (result == ERROR_SUCCESS);
+ }
+
+ ~RegistryKey() {
+ if (is_open_ && key_ != nullptr) {
+ RegCloseKey(key_);
+ key_ = nullptr;
+ is_open_ = false;
+ }
+ }
+
+ HKEY key() const { return key_; }
+ bool is_open() const { return is_open_; }
+
+ std::string GetString(const std::string& name, const std::string&
default_value = "") {
Review Comment:
Drop the default value to make it explicit at the callsite that `""` will
return if not found and pass by value, since the function can consume the
string when returning.
```suggestion
std::string GetString(const std::string& name, std::string default_value) {
```
##########
go/adbc/drivermgr/adbc_driver_manager.cc:
##########
@@ -114,7 +125,187 @@ struct OwnedError {
}
};
+std::filesystem::path UserConfigDir() {
+ std::filesystem::path config_dir;
+#if defined(_WIN32)
+ auto dir = std::getenv("AppData");
+ if (dir) {
+ config_dir = std::filesystem::path(dir);
+ config_dir /= "ADBC/drivers";
+ }
+#elif defined(__APPLE__)
+ auto dir = std::getenv("HOME");
+ if (dir) {
+ config_dir = std::filesystem::path(dir);
+ config_dir /= "Library/Application Support/ADBC";
+ }
+#elif defined(__linux__)
+ auto dir = std::getenv("XDG_CONFIG_HOME");
+ if (!dir) {
+ dir = std::getenv("HOME");
+ if (dir) {
+ config_dir = std::filesystem::path(dir);
+ }
+ } else {
+ config_dir = std::filesystem::path(dir);
+ }
+
+ if (!config_dir.empty()) {
+ config_dir /= "adbc";
+ }
+#endif
+
+ return config_dir;
+}
+
// Driver state
+struct DriverInfo {
+ std::string manifest_file;
+ std::string driver_name;
+ std::string lib_path;
+ std::string entrypoint;
+
+ std::string version;
+ std::string source;
+};
+
+#ifdef _WIN32
+class RegistryKey {
+ public:
+ RegistryKey(HKEY root, const std::string_view subkey) noexcept
+ : root_(root), key_(nullptr), is_open_(false) {
+ auto result = RegOpenKeyExA(root_, subkey.data(), 0, KEY_READ, &key_);
+ is_open_ = (result == ERROR_SUCCESS);
Review Comment:
Better to retain the error as a member variable and define a `bool is_open()
const` that performs this comparison and a getter for the result can improve
the error handling.
##########
go/adbc/drivermgr/adbc_driver_manager.cc:
##########
@@ -114,7 +125,187 @@ struct OwnedError {
}
};
+std::filesystem::path UserConfigDir() {
+ std::filesystem::path config_dir;
+#if defined(_WIN32)
+ auto dir = std::getenv("AppData");
+ if (dir) {
+ config_dir = std::filesystem::path(dir);
+ config_dir /= "ADBC/drivers";
+ }
+#elif defined(__APPLE__)
+ auto dir = std::getenv("HOME");
+ if (dir) {
+ config_dir = std::filesystem::path(dir);
+ config_dir /= "Library/Application Support/ADBC";
+ }
+#elif defined(__linux__)
+ auto dir = std::getenv("XDG_CONFIG_HOME");
+ if (!dir) {
+ dir = std::getenv("HOME");
+ if (dir) {
+ config_dir = std::filesystem::path(dir);
+ }
+ } else {
+ config_dir = std::filesystem::path(dir);
+ }
+
+ if (!config_dir.empty()) {
+ config_dir /= "adbc";
+ }
+#endif
+
+ return config_dir;
+}
+
// Driver state
+struct DriverInfo {
+ std::string manifest_file;
+ std::string driver_name;
+ std::string lib_path;
+ std::string entrypoint;
+
+ std::string version;
+ std::string source;
+};
+
+#ifdef _WIN32
+class RegistryKey {
+ public:
+ RegistryKey(HKEY root, const std::string_view subkey) noexcept
+ : root_(root), key_(nullptr), is_open_(false) {
+ auto result = RegOpenKeyExA(root_, subkey.data(), 0, KEY_READ, &key_);
+ is_open_ = (result == ERROR_SUCCESS);
+ }
+
+ ~RegistryKey() {
+ if (is_open_ && key_ != nullptr) {
+ RegCloseKey(key_);
+ key_ = nullptr;
+ is_open_ = false;
+ }
+ }
+
+ HKEY key() const { return key_; }
+ bool is_open() const { return is_open_; }
+
+ std::string GetString(const std::string& name, const std::string&
default_value = "") {
+ if (!is_open_) return default_value;
+
+ DWORD type = REG_SZ;
+ DWORD size = 0;
+ auto result = RegQueryValueExA(key_, name.c_str(), nullptr, &type,
nullptr, &size);
+ if (result != ERROR_SUCCESS) return default_value;
+ if (type != REG_SZ) return default_value;
+
+ std::string value(size, '\0');
+ result = RegQueryValueExA(key_, name.c_str(), nullptr, &type,
+ reinterpret_cast<LPBYTE>(value.data()), &size);
+ if (result != ERROR_SUCCESS) return default_value;
+ return value;
+ }
+
+ private:
+ HKEY root_;
+ HKEY key_;
+ bool is_open_;
+};
+
+AdbcStatusCode LoadDriverFromRegistry(HKEY root, const std::string&
driver_name,
+ DriverInfo& info, struct AdbcError*
error) {
+ static const LPCSTR kAdbcDriverRegistry = "SOFTWARE\\ADBC\\Drivers";
+ RegistryKey drivers_key(root, kAdbcDriverRegistry);
+ if (!drivers_key.is_open()) {
+ return ADBC_STATUS_NOT_FOUND;
+ }
+
+ RegistryKey dkey(drivers_key.key(), driver_name);
+ if (!dkey.is_open()) {
+ return ADBC_STATUS_NOT_FOUND;
+ }
+
+ info.driver_name = dkey.GetString("name");
+ info.entrypoint = dkey.GetString("entrypoint");
+ info.version = dkey.GetString("version");
+ info.source = dkey.GetString("source");
+ info.lib_path = dkey.GetString("driver");
+ if (info.lib_path.empty()) {
+ SetError(error,
+ "[DriverManager] Driver path not found in registry for "s +
driver_name);
Review Comment:
Just in case `driver_name` is empty.
```suggestion
"[DriverManager] Driver path not found in registry for \""s +
driver_name + "\"");
```
##########
c/include/arrow-adbc/adbc_driver_manager.h:
##########
@@ -31,6 +32,17 @@ extern "C" {
#ifndef ADBC_DRIVER_MANAGER_H
#define ADBC_DRIVER_MANAGER_H
+typedef uint16_t AdbcLoadOption;
Review Comment:
Given how "option" is overloaded in ADBC already, this could be called
`AdbcLoadFlags`.
`uint32_t` is also a better default type for this.
##########
go/adbc/drivermgr/adbc_driver_manager.cc:
##########
@@ -1658,9 +2047,10 @@ const char* AdbcStatusCodeMessage(AdbcStatusCode code) {
#undef CASE
}
-AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint,
- int version, void* raw_driver, struct AdbcError*
error) {
- AdbcDriverInitFunc init_func;
+AdbcStatusCode AdbcFindLoadDriver(const char* driver_name, const char*
entrypoint,
Review Comment:
Very nice!
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -114,7 +129,175 @@ struct OwnedError {
}
};
+std::filesystem::path UserConfigDir() {
+ std::filesystem::path config_dir;
+#if defined(_WIN32)
+ auto dir = std::getenv("AppData");
Review Comment:
> ...create a Windows user with a é (e.g.)
Everyone goes through this ritual with Windows programming, huh? :)
##########
go/adbc/drivermgr/arrow-adbc/adbc_driver_manager.h:
##########
@@ -51,6 +63,52 @@ ADBC_EXPORT
AdbcStatusCode AdbcLoadDriver(const char* driver_name, const char* entrypoint,
int version, void* driver, struct AdbcError*
error);
+/// \brief Common entry point to search for and load a driver or manifest.
+///
+/// The driver manager can fill in default implementations of some ADBC
functions
+/// for drivers. Drivers must implement a minimum level of functionality for
this
+/// to be possible, however, and some functions must be implemented by the
driver.
+///
+/// This function is different from AdbcLoadDriver in that it also accepts the
name
+/// of a driver manifest file, and allows specifying options to control what
+/// directories it will search through. The behavior is as follows:
+///
+/// If the passed in driver_name is an absolute path:
+/// - If the path has a `.toml` extension, it will attempt to parse the
manifest and load
+/// the driver specified within it. Erroring if this fails.
+/// - If the path has an extension other than `.toml`, it will attempt to load
the path as
+/// a shared library. Erroring if this fails.
+///
+/// If the passed in driver_name does not have an extension and is not an
absolute path:
+/// - The load_options parameter will control whether the driver manager will
search
+/// the ADBC_CONFIG_PATH environment variable, the user configuration
directory, and/or
+/// the system level directory of /etc/adbc for either a manifest file or a
shared
+/// library.
+/// - For each path to be search, it will first look for
<path>/<driver_name>.toml. If
+/// that file exists, it will attempt to parse the manifest and load the
driver
+/// specified within it. Erroring if this fails.
+/// - If the manifest file does not exist, it will then look for
+/// <path>/<driver_name>.<extension>
+/// where <extension> is one of the following: `.so`, `.dll`, `.dylib`. If
it can load
+/// that shared library, then success is returned. Otherwise it moves to the
next
+/// directory until the search is either successful, or all directories have
been
+/// searched.
+///
+/// \param[in] driver_name An identifier for the driver (e.g. a path to a
+/// shared library on Linux or the basename of a manifest file).
+/// \param[in] entrypoint An identifier for the entrypoint (e.g. the symbol to
+/// call for AdbcDriverInitFunc on Linux). If not provided, search for an
+/// entrypoint based on the driver name.
+/// \param[in] version The ADBC revision to attempt to initialize.
+/// \param[in] load_options bit mask of AdbcLoadOption to control the
directories searched
+/// \param[out] raw_driver The table of function pointers to initialize
+/// \param[out] error An optional location to return an error message
+/// @return
+ADBC_EXPORT
+AdbcStatusCode AdbcFindLoadDriver(const char* driver_name, const char*
entrypoint,
+ const int version, const AdbcLoadOption
load_options,
+ void* driver, struct AdbcError* error);
Review Comment:
Thanks for exposing `load_options`!
--
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]
