You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "eitsupi (via GitHub)" <gi...@apache.org> on 2025/07/13 15:08:20 UTC
[PR] ci(rust): enable tests on windows [arrow-adbc]
eitsupi opened a new pull request, #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148
Close #3122
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067402766
So I guess you can dynamically link Go libraries on Windows, but not statically.
--
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] ci(rust): enable tests on windows [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067176647
Welp, looks like I gotta fix the windows build code for the 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
Re: [PR] ci(rust): enable tests on windows [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067179987
Yea, let's temporarily exclude windows while I fix the windows build so we can merge 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] fix(rust/core): fix build errror on windows and enable ci for windows [arrow-adbc]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3069854492
@zeroshade I was not able to fix the adbc_snowflake build error today. Also, I am currently skipping some tests of driver_manager.
Feel free to edit this PR as you did yesterday.
(However, since we are making a lot of progress compared to yesterday, I feel it is better to merge them anyway.)
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067399702
Okay, so I've almost got this except for one weird linking issue with the rust adbc_snowflake crate on windows:
```shell
nowflake-0.natvis" "/NATVIS:C:\\Users\\zotth\\AppData\\Local\\Temp\\rustcbQbqD9\\adbc_snowflake-1.natvis"
= note: some arguments are omitted. use `--verbose` to show all linker arguments
= note: legacy_stdio_definitions.lib(legacy_stdio_definitions.obj) : warning LNK4078: multiple '.drectve' sections found with different attributes (00100A00)␍
snowflake.lib(go.o) : fatal error LNK1223: invalid or corrupt file: file contains invalid .pdata contributions␍
```
I've hit a wall here. Any ideas @eitsupi @kou @lidavidm @paleolimbot ?
--
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] ci(rust): enable tests on windows [arrow-adbc]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067179314
CI is apparently working now, but the build is failing, so do I need to temporarily exclude `windows-latest` from the matrix so it can be merged?
--
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] fix(rust/core): fix build errror on windows and enable ci for windows [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3069886339
I agree. Let's merge what we have and then open a new PR to continue fixing the windows tests.
--
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] fix(rust/core): fix build errror on windows and enable ci for windows [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148
--
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] fix(rust/core): fix build errror on windows and enable ci for windows [arrow-adbc]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3068387382
Some tests are failing on Windows:
```
running 14 tests
test driver_manager::tests::test_get_search_paths ... ok
test driver_manager::tests::test_default_entrypoint ... FAILED
test driver_manager::tests::test_load_driver_env ... FAILED
test driver_manager::tests::test_load_absolute_path ... ok
test driver_manager::tests::test_load_absolute_path_no_ext ... ok
test driver_manager::tests::test_disallow_env_config ... FAILED
test driver_manager::tests::test_load_relative_path ... ok
test driver_manager::tests::test_load_driver_env_multiple_paths ... ok
test driver_manager::tests::test_load_non_ascii_path ... ok
test driver_manager::tests::test_manifest_missing_driver ... ok
test driver_manager::tests::test_manifest_wrong_arch ... ok
test driver_manager::tests::test_manifest_user_config ... FAILED
test ffi::types::tests::test_error_roundtrip ... ok
test ffi::types::tests::test_partitions_roundtrip ... ok
failures:
---- driver_manager::tests::test_default_entrypoint stdout ----
thread 'driver_manager::tests::test_default_entrypoint' panicked at core\src\driver_manager.rs:1887:13:
assertion `left == right` failed
left: "AdbcLibadbcDriverSqliteInit"
right: "AdbcDriverSqliteInit"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- driver_manager::tests::test_load_driver_env stdout ----
thread 'driver_manager::tests::test_load_driver_env' panicked at core\src\driver_manager.rs:1921:9:
assertion `left == right` failed
left: Internal
right: NotFound
---- driver_manager::tests::test_disallow_env_config stdout ----
thread 'driver_manager::tests::test_disallow_env_config' panicked at core\src\driver_manager.rs:1995:9:
assertion `left == right` failed
left: Internal
right: NotFound
---- driver_manager::tests::test_manifest_user_config stdout ----
thread 'driver_manager::tests::test_manifest_user_config' panicked at core\src\driver_manager.rs:2123:9:
assertion `left == right` failed
left: Internal
right: NotFound
failures:
driver_manager::tests::test_default_entrypoint
driver_manager::tests::test_disallow_env_config
driver_manager::tests::test_load_driver_env
driver_manager::tests::test_manifest_user_config
test result: FAILED. 10 passed; 4 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.21s
```
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067226007
@eitsupi I've worked out the necessary changes to get it to build on windows. Should I push it to this PR or do you want to get this merged and I'll make a follow-up PR to enable the windows build with my changes?
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067229445
That's so fast! Either is fine with me.
(I'm going to sleep now so I won't be able to reply for a while, so please feel free to edit this PR etc.)
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#discussion_r2203988524
##########
rust/driver/snowflake/build.rs:
##########
@@ -50,7 +51,14 @@ fn bundled() -> Result<(), Box<dyn Error>> {
println!("cargo:rustc-link-lib=static=snowflake");
// Link other dependencies.
- println!("cargo:rustc-link-lib=resolv");
+ #[cfg(not(target_os = "windows"))]
+ {
+ println!("cargo:rustc-link-lib=resolv");
+ }
Review Comment:
```suggestion
#[cfg(not(target_os = "windows"))]
println!("cargo:rustc-link-lib=resolv");
```
##########
rust/driver/snowflake/build.rs:
##########
@@ -50,7 +51,14 @@ fn bundled() -> Result<(), Box<dyn Error>> {
println!("cargo:rustc-link-lib=static=snowflake");
// Link other dependencies.
- println!("cargo:rustc-link-lib=resolv");
+ #[cfg(not(target_os = "windows"))]
+ {
+ println!("cargo:rustc-link-lib=resolv");
+ }
+ #[cfg(target_os = "windows")]
+ {
+ println!("cargo:rustc-link-lib=legacy_stdio_definitions");
+ }
Review Comment:
```suggestion
#[cfg(target_os = "windows")]
println!("cargo:rustc-link-lib=legacy_stdio_definitions");
```
##########
rust/driver/snowflake/build.rs:
##########
@@ -70,7 +78,10 @@ fn linked() -> Result<(), Box<dyn Error>> {
if let Some(path) = option_env!("ADBC_SNOWFLAKE_GO_LIB_DIR") {
println!("cargo:rustc-link-search={path}");
}
-
+ #[cfg(target_os = "windows")]
+ {
+ println!("cargo:rustc-link-lib=legacy_stdio_definitions");
+ }
Review Comment:
```suggestion
#[cfg(target_os = "windows")]
println!("cargo:rustc-link-lib=legacy_stdio_definitions");
```
##########
rust/core/src/driver_manager.rs:
##########
@@ -609,15 +618,27 @@ fn load_driver_from_registry(
entrypoint: Option<&[u8]>,
) -> Result<DriverInfo> {
const ADBC_DRIVER_REGISTRY: &str = "SOFTWARE\\ADBC\\Drivers";
- let drivers_key = root.open(ADBC_DRIVER_REGISTRY)?;
+ let drivers_key = root
+ .open(ADBC_DRIVER_REGISTRY)
+ .and_then(|k| k.open(driver_name.to_str().unwrap_or_default()))
+ .map_err(|e| {
+ Error::with_message_and_status(
+ format!("Failed to open registry key: {e}"),
+ Status::NotFound,
+ )
+ })?;
+
+ let entrypoint_val = match drivers_key.get_string("entrypoint") {
+ Ok(s) => Some(s.into_bytes()),
+ Err(_) => None,
+ };
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()),
+ entrypoint: match entrypoint_val {
+ Some(e) => Some(e),
+ None => entrypoint.map(|s| s.to_vec()),
+ },
Review Comment:
You could use https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.or_else
##########
rust/core/src/driver_manager.rs:
##########
@@ -609,15 +618,27 @@ fn load_driver_from_registry(
entrypoint: Option<&[u8]>,
) -> Result<DriverInfo> {
const ADBC_DRIVER_REGISTRY: &str = "SOFTWARE\\ADBC\\Drivers";
- let drivers_key = root.open(ADBC_DRIVER_REGISTRY)?;
+ let drivers_key = root
+ .open(ADBC_DRIVER_REGISTRY)
+ .and_then(|k| k.open(driver_name.to_str().unwrap_or_default()))
+ .map_err(|e| {
+ Error::with_message_and_status(
+ format!("Failed to open registry key: {e}"),
+ Status::NotFound,
+ )
+ })?;
+
+ let entrypoint_val = match drivers_key.get_string("entrypoint") {
+ Ok(s) => Some(s.into_bytes()),
+ Err(_) => None,
+ };
Review Comment:
You could use https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.ok
##########
rust/core/src/driver_manager.rs:
##########
@@ -1658,25 +1679,25 @@ impl Drop for ManagedStatement {
}
}
-#[cfg(target_os = "windows")]
-use windows_sys as windows;
-
#[cfg(target_os = "windows")]
mod target_windows {
+ #[cfg(target_os = "windows")]
Review Comment:
We can remove this here because the `mod` has the same cfg attribute
```suggestion
```
--
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] ci(rust): enable tests on windows [arrow-adbc]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067177835
Apparently, `use windows_sys as windows` doesn't work!
Sorry...
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067407103
*sigh* well, at least they found the issue and are fixing it, even though it'll be a while before we can use that fix. I guess we can disable the bundled feature on windows for now so that it doesn't do the static link
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067185968
> let's temporarily exclude windows while I fix the windows build so we can merge this
Thanks!
I've excluded windows for now and opened a follow up issue #3149.
--
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] fix(rust/core): fix build errror on windows and enable ci for windows [arrow-adbc]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3069908817
Thanks!
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#discussion_r2203636913
##########
rust/driver/snowflake/build.rs:
##########
@@ -70,7 +78,7 @@ fn linked() -> Result<(), Box<dyn Error>> {
if let Some(path) = option_env!("ADBC_SNOWFLAKE_GO_LIB_DIR") {
println!("cargo:rustc-link-search={path}");
}
-
+
Review Comment:
nit: trailing whitespace?
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3067402334
It's https://go-review.googlesource.com/c/go/+/678795
--
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] ci(rust): add a job for tests on windows (excluded for now) [arrow-adbc]
Posted by "eitsupi (via GitHub)" <gi...@apache.org>.
eitsupi commented on PR #3148:
URL: https://github.com/apache/arrow-adbc/pull/3148#issuecomment-3068206012
Thanks all! I'll take a look the build error.
--
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