You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2025/08/20 21:20:21 UTC
[PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
zeroshade opened a new pull request, #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320
Updates the driver manager logic to handle virtual environments used by venv and conda.
venv will set a `VIRTUAL_ENV` environment variable while conda will set `CONDA_PREFIX`. If these environment variables are set, then they are added to the search list of `get_search_paths` / `GetSearchPaths` when the `LOAD_FLAG_SEARCH_ENV` flag is set.
Also updates the docs to mention 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2289335204
##########
docs/source/format/driver_manifests.rst:
##########
@@ -431,9 +431,14 @@ Unix-like Platforms
For Unix-like platforms, (e.g. Linux, macOS), the driver manager will search the following directories based on the options provided, in
the given order:
-#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then the environment variable ``ADBC_CONFIG_PATH`` will be searched
+#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then environment variables will be searched in this order
* ``ADBC_CONFIG_PATH`` is a colon-separated list of directories to search for ``${name}.toml``
+ * ``VIRTUAL_ENV`` is used by `venv <https://docs.python.org/3/library/venv.html>`__ and if set
+ ``$VIRTUAL_ENV/etc/adbc`` will be searched for ``${name}.toml``
+ * ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ when using a ``conda``
+ environment (by using ``conda activate env-name``), and if set then ``$CONDA_PREFIX/etc/adbc``
+ will be searched for ``${name}.toml``
Review Comment:
I think it'd be OK to leave out the details to make this shorter:
```suggestion
* ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ and if set then
``$CONDA_PREFIX/etc/adbc`` will be searched for ``${name}.toml``
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2289335204
##########
docs/source/format/driver_manifests.rst:
##########
@@ -431,9 +431,14 @@ Unix-like Platforms
For Unix-like platforms, (e.g. Linux, macOS), the driver manager will search the following directories based on the options provided, in
the given order:
-#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then the environment variable ``ADBC_CONFIG_PATH`` will be searched
+#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then environment variables will be searched in this order
* ``ADBC_CONFIG_PATH`` is a colon-separated list of directories to search for ``${name}.toml``
+ * ``VIRTUAL_ENV`` is used by `venv <https://docs.python.org/3/library/venv.html>`__ and if set
+ ``$VIRTUAL_ENV/etc/adbc`` will be searched for ``${name}.toml``
+ * ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ when using a ``conda``
+ environment (by using ``conda activate env-name``), and if set then ``$CONDA_PREFIX/etc/adbc``
+ will be searched for ``${name}.toml``
Review Comment:
I think it'd be OK to omit the details to make this shorter:
```suggestion
* ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ and if set
``$CONDA_PREFIX/etc/adbc`` will be searched for ``${name}.toml``
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2292445995
##########
c/driver_manager/CMakeLists.txt:
##########
Review Comment:
We may need similar checks in other builds? (R driver manager?)
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -1362,6 +1414,22 @@ AdbcStatusCode AdbcDriverManagerDatabaseSetLoadFlags(struct AdbcDatabase* databa
return ADBC_STATUS_OK;
}
+AdbcStatusCode AdbcDriverManagerDatabaseSetAdditionalSearchPathList(
+ struct AdbcDatabase* database, const char* path_list, struct AdbcError* error) {
+ if (database->private_driver) {
+ SetError(error, "Cannot SetAdditionalSearchPathList after AdbcDatabaseInit");
+ return ADBC_STATUS_INVALID_STATE;
+ }
+
+ TempDatabase* args = reinterpret_cast<TempDatabase*>(database->private_data);
+ if (path_list) {
+ args->additional_search_path_list = path_list;
Review Comment:
IMO this isn't safe. We should store an allocated std::string.
##########
docs/source/format/driver_manifests.rst:
##########
Review Comment:
Should we note that CONDA_PREFIX is only searched if configured at build time?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3221367375
@zeroshade can you please comment the `#endif`s like `#endif // _WIN32` to make the conditional logic easier to follow?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2304221213
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -273,14 +273,39 @@ AdbcStatusCode LoadDriverManifest(const std::filesystem::path& driver_manifest,
return ADBC_STATUS_OK;
}
+std::vector<std::filesystem::path> GetEnvPaths(const char_type* env_var) {
+#ifdef _WIN32
+ size_t required_size;
+
+ _wgetenv_s(&required_size, NULL, 0, env_var);
+ if (required_size == 0) {
+ return {};
+ }
+
+ std::wstring path_var;
+ path_var.resize(required_size);
+ _wgetenv_s(&required_size, path_var.data(), required_size, env_var);
+ auto path = Utf8Encode(path_var);
+#else
+ const char* path_var = std::getenv(env_var);
+ if (!path_var) {
+ return {};
+ }
+ std::string path(path_var);
+#endif
+ return InternalAdbcParsePath(path);
+}
+
std::vector<std::filesystem::path> GetSearchPaths(const AdbcLoadFlags levels) {
std::vector<std::filesystem::path> paths;
if (levels & ADBC_LOAD_FLAG_SEARCH_ENV) {
+#ifdef _WIN32
+ static const wchar_t* env_var = L"ADBC_CONFIG_PATH";
+#else
+ static const char* env_var = "ADBC_CONFIG_PATH";
+#endif
Review Comment:
```suggestion
#endif // _WIN32
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3208799435
@paleolimbot Thanks for calling this out. I think what's proposed here would be problematic as the default search behavior for driver shared libraries. What makes it reasonable (at least in my thinking) is that it's not that; it's the search behavior for TOML manifests. The TOML manifests are fundamentally an optional affordance for portability and user convenience. I expect that the vast majority of developers building apps that use the driver managers will skip using manifests and instead have the driver managers search directly for driver shared libraries. With all things concerning the manifests, we're operating within a small corner of the overall space, and in this small corner the objective is to afford portability and convenience. In that context, this proposed behavior seems well aligned with that goal.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3217320086
@zeroshade please let me know if my docs updates in dc5144133c3b7c5f440bdc3006a645d036e66deb seem correct.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3221405625
> @zeroshade can you please comment the #endifs like #endif // _WIN32 to make the conditional logic easier to follow?
done!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2289341054
##########
docs/source/format/driver_manifests.rst:
##########
@@ -452,9 +456,15 @@ Windows
Things are slightly different on Windows, where the driver manager will also search for driver information in the registry just as
would happen for ODBC drivers. The search for a manifest on Windows would be the following:
-#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then the environment variable ``ADBC_CONFIG_PATH`` will be searched
+#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then the environment variables will be searched in this order
* ``ADBC_CONFIG_PATH`` is a semicolon-separated list of directories to search for ``${name}.toml``
+ * ``VIRTUAL_ENV`` is used by `venv <https://docs.python.org/3/library/venv.html>`__ and if set
+ ``$VIRTUAL_ENV/etc/adbc`` will be searched for ``${name}.toml``
+ * ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ when using a ``conda``
+ environment (by using ``conda activate env-name``), and if set then ``$CONDA_PREFIX/etc/adbc``
+ will be searched for ``${name}.toml``
Review Comment:
Same here
```suggestion
* ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ and if set
``$CONDA_PREFIX/etc/adbc`` will be searched for ``${name}.toml``
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2294308983
##########
docs/source/format/driver_manifests.rst:
##########
Review Comment:
updated with a note for 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3212473961
> For example, if I install the PyPI adbc_driver_manager package into my Conda environment, driver loading won't search my conda environment by default? It seems cleaner to me to just put the conda stuff alongside the venv stuff in the Python package.
Under Conda you can't assume you'll only be used by Python, so relying entirely on the Python-side code won't work IMO
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2289349730
##########
docs/source/format/driver_manifests.rst:
##########
@@ -431,9 +431,13 @@ Unix-like Platforms
For Unix-like platforms, (e.g. Linux, macOS), the driver manager will search the following directories based on the options provided, in
the given order:
-#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then the environment variable ``ADBC_CONFIG_PATH`` will be searched
+#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then environment variables will be searched in this order
* ``ADBC_CONFIG_PATH`` is a colon-separated list of directories to search for ``${name}.toml``
+ * ``VIRTUAL_ENV`` is used by `venv <https://docs.python.org/3/library/venv.html>`__ and if set
+ ``$VIRTUAL_ENV/etc/adbc`` will be searched for ``${name}.toml``
+ * ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ and if set then
Review Comment:
tiniest nit
```suggestion
* ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ and if set
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3208158101
Looks like there are a bunch of other assorted other mentions of `ADBC_CONFIG_PATH` that will need replacing: https://github.com/search?q=repo%3Aapache%2Farrow-adbc+%22ADBC_CONFIG_PATH%22&type=code
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3222175623
In the parts of the code added/changed here, we're careful to use `wchar_t` for env vars on Windows, but higher up in `adbc_driver_manager.cc` there's an unconditional
```cpp
const char* env_path = std::getenv("ADBC_CONFIG_PATH");
```
Does that need to be changed?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2289335204
##########
docs/source/format/driver_manifests.rst:
##########
@@ -431,9 +431,14 @@ Unix-like Platforms
For Unix-like platforms, (e.g. Linux, macOS), the driver manager will search the following directories based on the options provided, in
the given order:
-#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then the environment variable ``ADBC_CONFIG_PATH`` will be searched
+#. If the ``LOAD_FLAG_SEARCH_ENV`` load option is set, then environment variables will be searched in this order
* ``ADBC_CONFIG_PATH`` is a colon-separated list of directories to search for ``${name}.toml``
+ * ``VIRTUAL_ENV`` is used by `venv <https://docs.python.org/3/library/venv.html>`__ and if set
+ ``$VIRTUAL_ENV/etc/adbc`` will be searched for ``${name}.toml``
+ * ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ when using a ``conda``
+ environment (by using ``conda activate env-name``), and if set then ``$CONDA_PREFIX/etc/adbc``
+ will be searched for ``${name}.toml``
Review Comment:
I think it'd be OK to leave out the details to make this shorter:
```suggestion
* ``CONDA_PREFIX`` is used by `conda <https://anaconda.org/anaconda/conda>`__ and if set
``$CONDA_PREFIX/etc/adbc`` will be searched for ``${name}.toml``
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3208843653
I think we're all in agreement that if you're using `adbc_driver_manager` from Python in a virtual environment that it makes sense to load a TOML manifest from the virtual environment, and if you're using `adbc_driver_manager` from a conda environment that it makes sense to load it from the conda prefix. My point is that putting that logic in a C header that could (is!) be used in an application completely unrelated to Python, Conda, or virtual environments is a crude way to make that happen.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3211614395
@amoeba correct, that's the current logic. Installing the PyPI adbc_driver_manager package would not perform automatic checking of your conda env, (but it would recognize a venv), and installing it from conda would then recognize the conda_prefix env var and check for it.
I was following @paleolimbot's suggestion of only checking `CONDA_PREFIX` if built for conda. I'm on the fence personally as to which makes more sense.... I'll leave that to you guys to duke out and I'll just implement whatever is decided.
@ianmcook you're right, I didn't update the docs to fit the new behavior yet. Thanks for taking on the updates there. you should be able to just push to this PR any doc updates you make.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2294513456
##########
c/driver_manager/CMakeLists.txt:
##########
Review Comment:
That's very kind of you (there are almost no conda R users so even though this is great I'm not particularly worried about it).
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2296305582
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -446,14 +489,22 @@ struct ManagedLibrary {
return ADBC_STATUS_NOT_FOUND;
}
- AdbcStatusCode FindDriver(const std::filesystem::path& driver_path,
- const AdbcLoadFlags load_options, DriverInfo& info,
- struct AdbcError* error) {
+ AdbcStatusCode FindDriver(
+ const std::filesystem::path& driver_path, const AdbcLoadFlags load_options,
+ const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info,
+ struct AdbcError* error) {
if (driver_path.empty()) {
SetError(error, "Driver path is empty");
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ {
+ auto status = SearchPaths(driver_path, additional_search_paths, info, error);
+ if (status == ADBC_STATUS_OK) {
+ return status;
+ }
+ }
+
Review Comment:
In my docs update, I wrote that:
- `ADBC_CONFIG_PATH` comes before the user-added search paths
- `$CONDA_PREFIX/etc/adbc` comes after the user-added search paths
After you update the implementation, please confirm that that's correct!
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2296245301
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -446,14 +489,22 @@ struct ManagedLibrary {
return ADBC_STATUS_NOT_FOUND;
}
- AdbcStatusCode FindDriver(const std::filesystem::path& driver_path,
- const AdbcLoadFlags load_options, DriverInfo& info,
- struct AdbcError* error) {
+ AdbcStatusCode FindDriver(
+ const std::filesystem::path& driver_path, const AdbcLoadFlags load_options,
+ const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info,
+ struct AdbcError* error) {
if (driver_path.empty()) {
SetError(error, "Driver path is empty");
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ {
+ auto status = SearchPaths(driver_path, additional_search_paths, info, error);
+ if (status == ADBC_STATUS_OK) {
+ return status;
+ }
+ }
+
Review Comment:
As this is currently implemented, if the user adds additional search paths, are those searched _before_ `ADBC_CONFIG_PATH`?
I don't think that's the behavior we want. We want them to be searched _after_ `ADBC_CONFIG_PATH` but before anything else, correct?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2298823073
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -446,14 +489,22 @@ struct ManagedLibrary {
return ADBC_STATUS_NOT_FOUND;
}
- AdbcStatusCode FindDriver(const std::filesystem::path& driver_path,
- const AdbcLoadFlags load_options, DriverInfo& info,
- struct AdbcError* error) {
+ AdbcStatusCode FindDriver(
+ const std::filesystem::path& driver_path, const AdbcLoadFlags load_options,
+ const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info,
+ struct AdbcError* error) {
if (driver_path.empty()) {
SetError(error, "Driver path is empty");
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ {
+ auto status = SearchPaths(driver_path, additional_search_paths, info, error);
+ if (status == ADBC_STATUS_OK) {
+ return status;
+ }
+ }
+
Review Comment:
Easy but hacky way to do it: If user adds additional search paths AND `LOAD_FLAG_SEARCH_ENV ` flag is set, then add `$ADBC_CONFIG_PATH` to the beginning of the search paths.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2296250174
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -446,14 +489,22 @@ struct ManagedLibrary {
return ADBC_STATUS_NOT_FOUND;
}
- AdbcStatusCode FindDriver(const std::filesystem::path& driver_path,
- const AdbcLoadFlags load_options, DriverInfo& info,
- struct AdbcError* error) {
+ AdbcStatusCode FindDriver(
+ const std::filesystem::path& driver_path, const AdbcLoadFlags load_options,
+ const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info,
+ struct AdbcError* error) {
if (driver_path.empty()) {
SetError(error, "Driver path is empty");
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ {
+ auto status = SearchPaths(driver_path, additional_search_paths, info, error);
+ if (status == ADBC_STATUS_OK) {
+ return status;
+ }
+ }
+
Review Comment:
Do we? I thought we wanted the application defined search paths to be searched first? Though I guess it makes sense to search after the env var now that I think of it... I'll fix 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3218438491
Also pre-commit indicates adbc_driver_manager.h needs to be synced to the go/ tree.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2296512433
##########
docs/source/format/driver_manifests.rst:
##########
@@ -351,7 +351,8 @@ to control which directories will be searched for manifests, with the behavior b
The type :c:type:`AdbcLoadFlags` is a set of bitflags to control the directories to be searched. The flags are
- * :c:macro:`ADBC_LOAD_FLAG_SEARCH_ENV` - search the environment variable ``ADBC_CONFIG_PATH``
+ * :c:macro:`ADBC_LOAD_FLAG_SEARCH_ENV` - search the directory paths in the environment variable
+ ``ADBC_CONFIG_PATH`` and (when built or installed with conda) search in the conda environment
Review Comment:
```suggestion
* :c:macro:`ADBC_LOAD_FLAG_SEARCH_ENV` - search the directory paths in the environment variable
``ADBC_CONFIG_PATH`` and (when built or installed with conda) search in the conda environment
```
I think these need to be indended for reST to recognize it as part of the same list
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2298717117
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -446,14 +489,22 @@ struct ManagedLibrary {
return ADBC_STATUS_NOT_FOUND;
}
- AdbcStatusCode FindDriver(const std::filesystem::path& driver_path,
- const AdbcLoadFlags load_options, DriverInfo& info,
- struct AdbcError* error) {
+ AdbcStatusCode FindDriver(
+ const std::filesystem::path& driver_path, const AdbcLoadFlags load_options,
+ const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info,
+ struct AdbcError* error) {
if (driver_path.empty()) {
SetError(error, "Driver path is empty");
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ {
+ auto status = SearchPaths(driver_path, additional_search_paths, info, error);
+ if (status == ADBC_STATUS_OK) {
+ return status;
+ }
+ }
+
Review Comment:
Currently the `$CONDA_PREFIX/etc/adbc` path (if it exists) will be listed as part of the `ENV` search paths, so it's difficult to insert the user-added paths before that but after the `ADBC_CONFIG_PATH` paths without a refactor. I'll see what I can do, but it's gonna be weird....
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2298823073
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -446,14 +489,22 @@ struct ManagedLibrary {
return ADBC_STATUS_NOT_FOUND;
}
- AdbcStatusCode FindDriver(const std::filesystem::path& driver_path,
- const AdbcLoadFlags load_options, DriverInfo& info,
- struct AdbcError* error) {
+ AdbcStatusCode FindDriver(
+ const std::filesystem::path& driver_path, const AdbcLoadFlags load_options,
+ const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info,
+ struct AdbcError* error) {
if (driver_path.empty()) {
SetError(error, "Driver path is empty");
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ {
+ auto status = SearchPaths(driver_path, additional_search_paths, info, error);
+ if (status == ADBC_STATUS_OK) {
+ return status;
+ }
+ }
+
Review Comment:
Easy but hacky way to do it: If user adds additional search paths AND `LOAD_FLAG_SEARCH_ENV` flag is set, then add `$ADBC_CONFIG_PATH` to the beginning of the search paths.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2304219713
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -273,14 +273,39 @@ AdbcStatusCode LoadDriverManifest(const std::filesystem::path& driver_manifest,
return ADBC_STATUS_OK;
}
+std::vector<std::filesystem::path> GetEnvPaths(const char_type* env_var) {
+#ifdef _WIN32
+ size_t required_size;
+
+ _wgetenv_s(&required_size, NULL, 0, env_var);
+ if (required_size == 0) {
+ return {};
+ }
+
+ std::wstring path_var;
+ path_var.resize(required_size);
+ _wgetenv_s(&required_size, path_var.data(), required_size, env_var);
+ auto path = Utf8Encode(path_var);
+#else
+ const char* path_var = std::getenv(env_var);
+ if (!path_var) {
+ return {};
+ }
+ std::string path(path_var);
+#endif
Review Comment:
```suggestion
#endif // _WIN32
```
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3211124401
> But conda is language-agnostic so I think there's a stronger case for keeping CONDA_PREFIX in the C driver manager.
Agreed (as long as it's not checked by default when built for things that are not conda!)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2298830020
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -446,14 +489,22 @@ struct ManagedLibrary {
return ADBC_STATUS_NOT_FOUND;
}
- AdbcStatusCode FindDriver(const std::filesystem::path& driver_path,
- const AdbcLoadFlags load_options, DriverInfo& info,
- struct AdbcError* error) {
+ AdbcStatusCode FindDriver(
+ const std::filesystem::path& driver_path, const AdbcLoadFlags load_options,
+ const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info,
+ struct AdbcError* error) {
if (driver_path.empty()) {
SetError(error, "Driver path is empty");
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ {
+ auto status = SearchPaths(driver_path, additional_search_paths, info, error);
+ if (status == ADBC_STATUS_OK) {
+ return status;
+ }
+ }
+
Review Comment:
Ah, the way you did it looks much cleaner :)
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3221555182
@zeroshade can't this be simplified by moving:
1. search `ADBC_CONFIG_PATHS`
2. search runtime application-defined paths
3. search `CONDA_PREFIX/etc/adbc`
all outside the `#ifdef _WIN32` block (above it)?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2294308460
##########
c/driver_manager/CMakeLists.txt:
##########
Review Comment:
updated the R `Makevars` and `Makevars.win` to check for `CONDA_BUILD` appropriately
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -1362,6 +1414,22 @@ AdbcStatusCode AdbcDriverManagerDatabaseSetLoadFlags(struct AdbcDatabase* databa
return ADBC_STATUS_OK;
}
+AdbcStatusCode AdbcDriverManagerDatabaseSetAdditionalSearchPathList(
+ struct AdbcDatabase* database, const char* path_list, struct AdbcError* error) {
+ if (database->private_driver) {
+ SetError(error, "Cannot SetAdditionalSearchPathList after AdbcDatabaseInit");
+ return ADBC_STATUS_INVALID_STATE;
+ }
+
+ TempDatabase* args = reinterpret_cast<TempDatabase*>(database->private_data);
+ if (path_list) {
+ args->additional_search_path_list = path_list;
Review Comment:
good point, fixed
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3211609602
Thanks for doing this @zeroshade. I'll propose some docs improvements. In this PR in its current state, it looks like the Python driver manager docs don't tell the user that it detects venv environments, and they make it seem like all the driver managers will always detect conda environments. I'll work to fix 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2296830059
##########
docs/source/format/driver_manifests.rst:
##########
@@ -351,7 +351,8 @@ to control which directories will be searched for manifests, with the behavior b
The type :c:type:`AdbcLoadFlags` is a set of bitflags to control the directories to be searched. The flags are
- * :c:macro:`ADBC_LOAD_FLAG_SEARCH_ENV` - search the environment variable ``ADBC_CONFIG_PATH``
+ * :c:macro:`ADBC_LOAD_FLAG_SEARCH_ENV` - search the directory paths in the environment variable
+ ``ADBC_CONFIG_PATH`` and (when built or installed with conda) search in the conda environment
Review Comment:
Fixed in e32e72a
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3211598479
Thanks @zeroshade. So with this, to get automatic conda environment detection in my adbc_driver_manager Python package build, am I restricted to installing the conda package? For example, if I install the PyPI adbc_driver_manager package into my Conda environment, driver loading won't search my conda environment by default? It seems cleaner to me to just put the conda stuff alongside the venv stuff in the Python package.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3212613321
Reconciling the comments above: How about we:
- Search in `CONDA_PREFIX` when the driver manager was built by conda (which this already does).
- Also add `CONDA_PREFIX` to the search paths in the Python driver manager (coming after `VIRTUAL_ENV` in precedence) regardless of whether it was built by conda.
Does that work for everyone?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2298823073
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -446,14 +489,22 @@ struct ManagedLibrary {
return ADBC_STATUS_NOT_FOUND;
}
- AdbcStatusCode FindDriver(const std::filesystem::path& driver_path,
- const AdbcLoadFlags load_options, DriverInfo& info,
- struct AdbcError* error) {
+ AdbcStatusCode FindDriver(
+ const std::filesystem::path& driver_path, const AdbcLoadFlags load_options,
+ const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info,
+ struct AdbcError* error) {
if (driver_path.empty()) {
SetError(error, "Driver path is empty");
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ {
+ auto status = SearchPaths(driver_path, additional_search_paths, info, error);
+ if (status == ADBC_STATUS_OK) {
+ return status;
+ }
+ }
+
Review Comment:
Easy but hacky way to do it: If user adds additional search paths AND `LOAD_FLAG_SEARCH_ENV` flag is set AND `ADBC_CONFIG_PATH` env var is set, then add `$ADBC_CONFIG_PATH` to the beginning of the user-added search paths.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on code in PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#discussion_r2298823073
##########
c/driver_manager/adbc_driver_manager.cc:
##########
@@ -446,14 +489,22 @@ struct ManagedLibrary {
return ADBC_STATUS_NOT_FOUND;
}
- AdbcStatusCode FindDriver(const std::filesystem::path& driver_path,
- const AdbcLoadFlags load_options, DriverInfo& info,
- struct AdbcError* error) {
+ AdbcStatusCode FindDriver(
+ const std::filesystem::path& driver_path, const AdbcLoadFlags load_options,
+ const std::vector<std::filesystem::path>& additional_search_paths, DriverInfo& info,
+ struct AdbcError* error) {
if (driver_path.empty()) {
SetError(error, "Driver path is empty");
return ADBC_STATUS_INVALID_ARGUMENT;
}
+ {
+ auto status = SearchPaths(driver_path, additional_search_paths, info, error);
+ if (status == ADBC_STATUS_OK) {
+ return status;
+ }
+ }
+
Review Comment:
Easy but hacky way to do it: If user adds additional search paths AND `LOAD_FLAG_SEARCH_ENV` flag is set, then add `$ADBC_CONFIG_PATH` to the beginning of the user-added search paths.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3211532970
@paleolimbot How's the updated version?
I've put the checks for `CONDA_PREFIX` behind the existence of the `CONDA_BUILD` env var at compile time (since it seems all conda build utilities will set that env var during build) in both the C and Rust driver managers.
I've then added the `additional_search_path_list` argument to `AdbcFindLoadDriver` and added an appropriate setter for `AdbcDatabase` to manipulate it. Finally, I updated the python driver manager to check if it's in a virtual environment, and if it is it will add the virtual environment prefix +'/etc/adbc' to the search path list. (And updated the R implementation accordingly).
Thoughts?
@lidavidm @ianmcook @amoeba Did I cover everything?
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(c/driver_manager,rust/driver_manager): handle virtual environments in driver manager [arrow-adbc]
Posted by "ianmcook (via GitHub)" <gi...@apache.org>.
ianmcook commented on PR #3320:
URL: https://github.com/apache/arrow-adbc/pull/3320#issuecomment-3208928098
Ah I see what you mean. Another way to implement this would be to add a config function in the C driver manager that allows wrappers to add paths to the manifest search paths at runtime. Then for example the Python driver manager could check if `VIRTUAL_ENV` is set and add it to the search paths if it is.
But conda is language-agnostic so I think there's a stronger case for keeping `CONDA_PREFIX` in the C driver manager.
--
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-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org