aldanor / hdf5-rust

HDF5 for Rust
https://docs.rs/hdf5
Apache License 2.0
310 stars 84 forks source link

Fix building with MSYS2 distributed MinGW hdf5. #229

Closed Berrysoft closed 1 year ago

Berrysoft commented 1 year ago
mulimoen commented 1 year ago

Do you have a way of including this build in the CI?

Berrysoft commented 1 year ago

I will try to include it. And by the way, cargo-clippy fails because of uninlined_format_args. Maybe you need to turn it off because it's very annoying:)

Berrysoft commented 1 year ago

@mulimoen I've added a job to test this crate with MSYS2.

mulimoen commented 1 year ago

The clippy lints should be fixed in a separate PR, I think clippy supports rust_version which would prevent MSRV trouble in the future. We need a bump in MSRV for that...

Berrysoft commented 1 year ago

The package updated just now, and the dll name now changes with the soversion. Should we use pkgconf in that case, instead of hard-coding the dll name manually?

Berrysoft commented 1 year ago

The new test passed.

mulimoen commented 1 year ago

I have not used a windows system in ages. I think you are a better judge of whether pkgconf will be more or less suprising for the developers using this library

Berrysoft commented 1 year ago

MSYS2 provides pkgconf of course, and vcpkg, the Microsoft-banded C/C++ package manager, also provides pkgconf affairs. I have also noticed that some other bindings, for example, libgit2 and openssl also use pkgconf to find libs.

openssl-sys only uses pkgconf for Windows GNU target. I think I can follow this approach.

Berrysoft commented 1 year ago

I added pkg-config support for all windows target because the gnu and msvc targets all supports pkg-config, and (maybe a bug?) they cannot be dealt separately. (When targeting GNU, a MSVC target build script will also be built and executed.)

And for Windows target, I choose to trust pkg-config result and not checking the dll, because at first we wanted to avoid hard-coding the dll names with pkg-config, which could not provide the dll name either.

mulimoen commented 1 year ago

We know hdf5 localisation is difficult, especially on windows. Thanks a lot for doing this the proper way

Berrysoft commented 1 year ago

Pkg-config for Windows support added. Now it should work with pkg-config, and if pkg-config is not installed, HDF5_DIR would also work (if dll name hadn't been changed by MSYS2).

pkg-config gives many paths like C:\msys64\mingw64\lib\..\bin, so we need to canonicalize the path before querying it in PATH. The paths in PATH should also be canonicalized when comparing because canonicalize would turn a path to a UNC path on Windows.

aldanor commented 1 year ago

If there's still interest in moving this forward, it would have to be rebased on the latest master, now that 1.14 support is in and all the build failures etc have been hopefully resolved and fixed.

Berrysoft commented 1 year ago

I've merged master but need approval to run CI.

aldanor commented 1 year ago

@Berrysoft There's changelog conflicts so you might want to quickly rebase again.

It looks like your build ran on CI and failed: https://github.com/aldanor/hdf5-rust/actions/runs/5227987760/jobs/9440874079?pr=229

Because the version is 1.14.1.2 (I think we do parse 1.14.1-2, like it looks on arch, but not this format)

Berrysoft commented 1 year ago

I solved the conflicts and updated the regex. Now it should parse both kind of versions.

aldanor commented 1 year ago

@mulimoen Looks good? Any comments from your side? Seems like mingw build now runs green again 1.14.1.2.

// @Berrysoft not to be picky, minor nit: in PRs you'd normally rebase your branch against the main one as opposed to merging, otherwise you end up with all those merge commits in main. No big deal though, I'll just squash-merge it.

aldanor commented 1 year ago

Interestingly enough, this mingw run doesn't hit this problem https://github.com/HDFGroup/hdf5/issues/3091

Berrysoft commented 1 year ago

I cannot reproduce this problem on my device either.

mulimoen commented 1 year ago

Weird with the 1.14.1-2 static build, can't see any obvious differences in the build flags. Looks good to go!

aldanor commented 1 year ago

Now merged, thanks @Berrysoft