paleolimbot commented on code in PR #2918:
URL: https://github.com/apache/arrow-adbc/pull/2918#discussion_r2155405347
##########
.github/workflows/native-unix.yml:
##########
@@ -194,6 +194,8 @@ jobs:
BUILD_DRIVER_MANAGER: "1"
BUILD_DRIVER_POSTGRESQL: "1"
BUILD_DRIVER_SQLITE: "1"
+ ADBC_DRIVER_MANAGER_USER_CONFIG_TEST: "1"
+ # ADBC_DRIVER_MANAGER_SYSTEM_CONFIG_TEST: "1"
Review Comment:
```suggestion
ADBC_DRIVER_MANAGER_USER_CONFIG_TEST: "1"
```
(unless that was intentional?)
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -631,9 +993,95 @@ static const char kDefaultEntrypoint[] = "AdbcDriverInit";
} // namespace
// Other helpers (intentionally not in an anonymous namespace so they can be
tested)
+ADBC_EXPORT
+std::filesystem::path InternalAdbcUserConfigDir() {
+ std::filesystem::path config_dir;
+#if defined(_WIN32)
+ size_t required_size;
+ _wgetenv_s(&required_size, NULL, 0, L"AppData");
+ if (required_size == 0) {
+ return config_dir;
+ }
+
+ std::wstring app_data_value;
+ app_data_value.resize(required_size);
+ _wgetenv_s(&required_size, app_data_value.data(), required_size, L"AppData");
Review Comment:
I still think you should use the same mechanism as rappdirs (R) and Python's
platformdirs (i.e., call the Windows API to get this value and fall back to the
env var) unless there's some reason that their prior art should be ignored. The
main difference as I understand it is that `AppData` will expand to either the
roaming profile (i.e., synced with a server) or the local profile, and calling
the API lets you be more explicit about that choice (I think local would be
more appropriate). This also ensures consistency if an application wants finer
grained control and uses one of those packages to reconstruct the logic you
have here.
##########
c/driver_manager/adbc_driver_manager_test.cc:
##########
@@ -332,7 +344,325 @@ TEST(AdbcDriverManagerInternal,
AdbcDriverManagerDefaultEntrypoint) {
"C:\\System32\\proprietary_engine.dll",
}) {
SCOPED_TRACE(driver);
- EXPECT_EQ("AdbcProprietaryEngineInit",
::AdbcDriverManagerDefaultEntrypoint(driver));
+ EXPECT_EQ("AdbcProprietaryEngineInit",
+ ::InternalAdbcDriverManagerDefaultEntrypoint(driver));
+ }
+
+ for (const auto& driver : {
+ "driver_example",
+ "libdriver_example.so",
+ }) {
+ SCOPED_TRACE(driver);
+ EXPECT_EQ("AdbcDriverExampleInit",
+ ::InternalAdbcDriverManagerDefaultEntrypoint(driver));
}
}
+
+TEST(AdbcDriverManagerInternal, InternalAdbcParsePath) {
+ // Test parsing a path of directories
+#ifdef __WIN32
+ static const char* const delimiter = ";";
+#else
+ static const char* const delimiter = ":";
+#endif
+
+ std::vector<std::string> paths = {
+ "/usr/lib/adbc/drivers", "/usr/local/lib/adbc/drivers",
+ "/opt/adbc/drivers", "/home/user/.config/adbc/drivers",
+#ifdef __WIN32
+ "/home/\":foo:\"/bar",
+#endif
+ };
+
+ std::ostringstream joined;
+ std::copy(paths.begin(), paths.end(),
+ std::ostream_iterator<std::string>(joined, delimiter));
+
+ auto output = InternalAdbcParsePath(joined.str());
+ EXPECT_EQ(output.size(), paths.size());
+
+ for (size_t i = 0; i < paths.size(); ++i) {
+ EXPECT_EQ(output[i].string(), paths[i]);
+ }
+}
+
+class DriverManifest : public ::testing::Test {
+ public:
+ void SetUp() override {
+ std::memset(&driver, 0, sizeof(driver));
+ std::memset(&error, 0, sizeof(error));
+
+#ifndef ADBC_DRIVER_MANAGER_TEST_LIB
+ GTEST_SKIP() << "ADBC_DRIVER_MANAGER_TEST_LIB is not defined. "
+ "This test requires a driver library to be specified.";
+#else
+ driver_path = std::filesystem::path(ADBC_DRIVER_MANAGER_TEST_LIB);
+ if (!std::filesystem::exists(driver_path)) {
+ GTEST_SKIP() << "Driver library does not exist: " << driver_path;
+ }
+
+ simple_manifest = toml::table{
+ {"name", "SQLite3"},
+ {"publisher", "arrow-adbc"},
+ {"version", "X.Y.Z"},
+ {"ADBC",
+ toml::table{
+ {"version", "1.1.0"},
+ }},
+ {"Driver",
+ toml::table{
+ {"shared",
+ toml::table{
+ {adbc::CurrentArch(), driver_path.string()},
+ }},
+ }},
+ };
+
+ auto temp_path = std::filesystem::temp_directory_path();
+ temp_path /= "adbc_driver_manager_test";
+
+ ASSERT_TRUE(std::filesystem::create_directories(temp_path));
+ temp_dir = temp_path;
+#endif
+ }
+
+ void TearDown() override {
+ if (error.release) {
+ error.release(&error);
+ }
+
+ if (driver.release) {
+ ASSERT_THAT(driver.release(&driver, &error), IsOkStatus(&error));
+ ASSERT_EQ(driver.private_data, nullptr);
+ ASSERT_EQ(driver.private_manager, nullptr);
+ }
+
+ driver_path.clear();
+ if (std::filesystem::exists(temp_dir)) {
+ std::filesystem::remove_all(temp_dir);
+ }
+ }
+
+ protected:
+ void SetConfigPath(const char* path) {
+#ifdef _WIN32
+ ASSERT_TRUE(SetEnvironmentVariable("ADBC_CONFIG_PATH", path));
+#else
+ setenv("ADBC_CONFIG_PATH", path, 1);
+#endif
+ }
+
+ void UnsetConfigPath() { SetConfigPath(""); }
+
+ struct AdbcDriver driver = {};
+ struct AdbcError error = {};
+
+ std::filesystem::path driver_path;
+ std::filesystem::path temp_dir;
+ toml::table simple_manifest;
+};
+
+TEST_F(DriverManifest, LoadDriverEnv) {
+ ASSERT_THAT(AdbcLoadDriver("sqlite", nullptr, ADBC_VERSION_1_1_0, &driver,
&error),
+ Not(IsOkStatus(&error)));
+
+ std::ofstream test_manifest_file(temp_dir / "sqlite.toml");
+ ASSERT_TRUE(test_manifest_file.is_open());
+ test_manifest_file << simple_manifest;
+ test_manifest_file.close();
+
+ SetConfigPath(temp_dir.string().c_str());
+
+ ASSERT_THAT(AdbcLoadDriver("sqlite", nullptr, ADBC_VERSION_1_1_0, &driver,
&error),
+ IsOkStatus(&error));
+
+ ASSERT_TRUE(std::filesystem::remove(temp_dir / "sqlite.toml"));
+
+ UnsetConfigPath();
+}
+
+TEST_F(DriverManifest, DisallowEnvConfig) {
+ std::ofstream test_manifest_file(temp_dir / "sqlite.toml");
+ ASSERT_TRUE(test_manifest_file.is_open());
+ test_manifest_file << simple_manifest;
+ test_manifest_file.close();
+
+ SetConfigPath(temp_dir.string().c_str());
+
+ auto load_options = ADBC_LOAD_FLAG_DEFAULT & ~ADBC_LOAD_FLAG_SEARCH_ENV;
+ ASSERT_THAT(AdbcFindLoadDriver("sqlite", nullptr, ADBC_VERSION_1_1_0,
load_options,
+ &driver, &error),
+ Not(IsOkStatus(&error)));
+
+ ASSERT_TRUE(std::filesystem::remove(temp_dir / "sqlite.toml"));
+
+ UnsetConfigPath();
+}
+
+TEST_F(DriverManifest, LoadAbsolutePath) {
+ auto filepath = temp_dir / "sqlite.toml";
+ std::ofstream test_manifest_file(filepath);
+ ASSERT_TRUE(test_manifest_file.is_open());
+ test_manifest_file << simple_manifest;
+ test_manifest_file.close();
+
+ ASSERT_THAT(AdbcLoadDriver(filepath.string().c_str(), nullptr,
ADBC_VERSION_1_1_0,
+ &driver, &error),
+ IsOkStatus(&error));
+
+ ASSERT_TRUE(std::filesystem::remove(filepath));
+}
+
+TEST_F(DriverManifest, LoadAbsolutePathNoExtension) {
+ auto filepath = temp_dir / "sqlite.toml";
+ std::ofstream test_manifest_file(filepath);
+ ASSERT_TRUE(test_manifest_file.is_open());
+ test_manifest_file << simple_manifest;
+ test_manifest_file.close();
+
+ auto noext = filepath;
+ noext.replace_extension(); // Remove the .toml extension
+ ASSERT_THAT(AdbcLoadDriver(noext.string().c_str(), nullptr,
ADBC_VERSION_1_1_0, &driver,
+ &error),
+ IsOkStatus(&error));
+
+ ASSERT_TRUE(std::filesystem::remove(filepath));
+}
+
+TEST_F(DriverManifest, LoadRelativePath) {
+ std::ofstream test_manifest_file("sqlite.toml");
+ ASSERT_TRUE(test_manifest_file.is_open());
+ test_manifest_file << simple_manifest;
+ test_manifest_file.close();
+
+ ASSERT_THAT(
+ AdbcFindLoadDriver("sqlite.toml", nullptr, ADBC_VERSION_1_1_0, 0,
&driver, &error),
+ IsStatus(ADBC_STATUS_INVALID_ARGUMENT, &error));
+
+ ASSERT_THAT(AdbcFindLoadDriver("sqlite.toml", nullptr, ADBC_VERSION_1_1_0,
+ ADBC_LOAD_FLAG_ALLOW_RELATIVE_PATHS, &driver,
&error),
+ IsOkStatus(&error));
+
+ ASSERT_TRUE(std::filesystem::remove("sqlite.toml"));
+}
+
+TEST_F(DriverManifest, ManifestMissingDriver) {
+ // Create a manifest without the "Driver" section
+ auto filepath = temp_dir / "sqlite.toml";
+ toml::table manifest_without_driver = simple_manifest;
+ manifest_without_driver.erase("Driver");
+
+ std::ofstream test_manifest_file(filepath);
+ ASSERT_TRUE(test_manifest_file.is_open());
+ test_manifest_file << manifest_without_driver;
+ test_manifest_file.close();
+
+ // Attempt to load the driver
+ ASSERT_THAT(AdbcLoadDriver(filepath.string().c_str(), nullptr,
ADBC_VERSION_1_1_0,
+ &driver, &error),
+ IsStatus(ADBC_STATUS_NOT_FOUND, &error));
+
+ ASSERT_TRUE(std::filesystem::remove(filepath));
+}
+
+TEST_F(DriverManifest, ManifestWrongArch) {
+ // Create a manifest without the "Driver" section
+ auto filepath = temp_dir / "sqlite.toml";
Review Comment:
```suggestion
auto filepath = temp_dir / "sqlite.toml";
```
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -114,7 +130,186 @@ struct OwnedError {
}
};
+#ifdef _WIN32
+using char_type = wchar_t;
+
+std::string Utf8Encode(const std::wstring& wstr) {
+ if (wstr.empty()) return std::string();
+ int size_needed = WideCharToMultiByte(
+ CP_UTF8, 0, &wstr[0], static_cast<int>(wstr.size()), NULL, 0, NULL,
NULL);
+ std::string str_to(size_needed, 0);
+ WideCharToMultiByte(CP_UTF8, 0, &wstr[0], static_cast<int>(wstr.size()),
&str_to[0],
+ size_needed, NULL, NULL);
+ return str_to;
+}
Review Comment:
I may have missed it in the tests, but are there any file paths with
non-ASCII characters in the tests?
##########
c/cmake_modules/DefineOptions.cmake:
##########
@@ -139,6 +139,12 @@ if("${CMAKE_SOURCE_DIR}" STREQUAL
"${CMAKE_CURRENT_SOURCE_DIR}")
"shared"
"static")
+ define_option(ADBC_DRIVER_MGR_TEST_MANIFEST_USER_LEVEL
Review Comment:
Other CMake variables seem to use `ADBC_DRIVER_MANAGER_...`...did you want
to use that pattern with these two as well?
--
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]
