Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix building with MSYS2 distributed MinGW hdf5. #229

Merged
merged 12 commits into from
Jun 10, 2023

Conversation

Berrysoft
Copy link
Contributor

@Berrysoft Berrysoft commented Jan 26, 2023

  • Allow minor version to be 14, because the version of MSYS2 distributed MinGW hdf5 is 1.14. Maybe need: Support hdf5 1.14.0 #227
  • Add is_msvc to build.rs to determine target enviroment from env var instead of cfg!, to make cross-compile right. Here the term "cross-compile" also means targeting GNU with MSVC toolchain (which is very common).
  • The dll name of hdf5 is libhdf5-0.dll or libhdf5-310.dll (see this commit: msys2/MINGW-packages@205f329). I'm afraid we need to update the dll name every time MSYS2 updates it. I suggest using pkgconf to detect it in the future.
  • Limit msvc_dll_indirection to MSVC only.
  • The tests have passed on my machine with MSVC toolchain, targeting GNU, and latest MSYS2.

@mulimoen
Copy link
Collaborator

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

@Berrysoft
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@mulimoen
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

The new test passed.

@mulimoen
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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

@Berrysoft
Copy link
Contributor Author

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.

Copy link
Collaborator

@mulimoen mulimoen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments, otherwise this PR is in great shape. Would you mind adding to the changelog?

hdf5-sys/build.rs Outdated Show resolved Hide resolved
hdf5-sys/build.rs Show resolved Hide resolved
@aldanor
Copy link
Owner

aldanor commented Jun 9, 2023

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
Copy link
Contributor Author

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

@aldanor
Copy link
Owner

aldanor commented Jun 10, 2023

@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
Copy link
Contributor Author

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

@aldanor
Copy link
Owner

aldanor commented Jun 10, 2023

@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
Copy link
Owner

aldanor commented Jun 10, 2023

Interestingly enough, this mingw run doesn't hit this problem HDFGroup/hdf5#3091

@Berrysoft
Copy link
Contributor Author

I cannot reproduce this problem on my device either.

@mulimoen
Copy link
Collaborator

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

@aldanor aldanor merged commit 0276592 into aldanor:master Jun 10, 2023
@aldanor
Copy link
Owner

aldanor commented Jun 10, 2023

Now merged, thanks @Berrysoft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants