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

HDF5: Update to 1.14.3 #7908

Merged
merged 16 commits into from Jan 8, 2024
Merged

Conversation

eschnett
Copy link
Contributor

@eschnett eschnett commented Jan 4, 2024

This includes and supersedes #7906.

@mkitti
Copy link
Contributor

mkitti commented Jan 5, 2024

@eschnett , what if broke this up and put the Fortran and C++ libraries into different JLLs? Is that very complicated?

@benlorenz
Copy link
Contributor

@eschnett the mpitrampoline builds are getting stuck again, they stop at some configure check:
https://buildkite.com/julialang/yggdrasil/builds/7605#018cdabb-0b64-4ea8-958f-c6e91e6641ed/616-6090

[18:28:57] checking for config linux-gnu... found

Looking at one of the other jobs this should probably be followed by something like compiler 'mpicc' is GNU gcc-6.1.0.

In the previous build they were stuck there for 4 hours until being killed: https://buildkite.com/julialang/yggdrasil/builds/7586#018cd787-ec93-4dc8-af99-dbf1d600aad0/620-6092

@eschnett
Copy link
Contributor Author

eschnett commented Jan 5, 2024

@benlorenz I fixed the MPItrampoline problem. CC expanded to mpicc, which tried to use CC to call the actual compiler.

@eschnett
Copy link
Contributor Author

eschnett commented Jan 5, 2024

@mkitti Separating HDF5_jll into several packages should be straightforward. I don't know how to build C++ or Fortran libraries based on existing C libraries, but we could build the full set and then simply split the build products.

Are you worried about Yggdrasil build times or about Julia loading unnecessary libraries?

@giordano
Copy link
Member

giordano commented Jan 5, 2024

Related to #7906 (comment), I looked at the log of aarch-apple-darwin and I don't see openmp being mentioned at all anywhere. None of the files in bin/ or lib/ of the corresponding artifact links to libomp.

@mkitti
Copy link
Contributor

mkitti commented Jan 6, 2024

Cross compilation to Windows almost works for C. Configure fails with the ROS3 driver enabled because it fails to detect EVP_sha256 in libcrypt for some reason:
https://github.com/HDFGroup/hdf5/blob/3d39b8595286a1cc45ca0e81ffa06dd8da0b590a/configure.ac#L3310C33-L3310C43

checking for curl/curl.h... yes
checking for openssl/evp.h... yes
checking for openssl/hmac.h... yes
checking for openssl/sha.h... yes
checking for curl_global_init in -lcurl... yes
checking for EVP_sha256 in -lcrypto... no
checking if the Read-Only S3 virtual file driver (VFD) is enabled... no
configure: error: The Read-Only S3 VFD was requested but cannot be built.
                      Please check that openssl and cURL are available on your
                      system, and/or re-configure without option
                      --enable-ros3-vfd.

From config.log:

configure:33153: checking for EVP_sha256 in -lcrypto
configure:33184: cc -o conftest.exe     -std=c99 -D_GNU_SOURCE -D__USE_MINGW_ANSI_STDIO   -D_FILE_OFFSET_BITS=64    -I/workspace/destdir/include   -L/workspace/destdir/lib conftest.c -lcrypto  -lcurl -lsz -lz -lm -L/workspace/destdir/bin -lmsmpi >&5
/opt/x86_64-w64-mingw32/bin/../lib/gcc/x86_64-w64-mingw32/6.1.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -lcrypto
collect2: error: ld returned 1 exit status
configure:33184: $? = 1
| #define HAVE_LIBCURL 1
| /* end confdefs.h.  */
|
| /* Override any GCC internal prototype to avoid an error.
|    Use char because int might match the return type of a GCC
|    builtin and then its argument prototype would still apply.  */
| char EVP_sha256 ();
| #ifdef FC_DUMMY_MAIN
| #ifndef FC_DUMMY_MAIN_EQ_F77
| #  ifdef __cplusplus
|      extern "C"
| #  endif
|    int FC_DUMMY_MAIN() { return 1; }
| #endif
| #endif
| int
| main (void)
| {
| return EVP_sha256 ();
|   ;
|   return 0;
| }
configure:33194: result: no

@mkitti
Copy link
Contributor

mkitti commented Jan 6, 2024

libcrypto seems to be in workspace/x86_64-w64-mingw32-libgfortran3-cxx11/destdir/lib64/ not workspace/x86_64-w64-mingw32-libgfortran3-cxx11/destdir/lib

sandbox:/ # ls ./workspace/x86_64-w64-mingw32-libgfortran3-cxx11/destdir/lib
cmake                             libload_time_mpi_constants.dll.a  libmbedtls.dll.a                  libnghttp2.dll.a                  libsz.dll.a
libaec.a                          libmbedcrypto.a                   libmbedx509.a                     libssh2.a                         libz.a
libaec.dll.a                      libmbedcrypto.dll.a               libmbedx509.dll.a                 libssh2.dll.a                     libz.dll.a
libcurl.dll.a                     libmbedtls.a                      libnghttp2.a                      libsz.a                           pkgconfig
sandbox:/ # ls ./workspace/x86_64-w64-mingw32-libgfortran3-cxx11/destdir/lib64/
engines-3        libcrypto.a      libcrypto.dll.a  libssl.a         libssl.dll.a     ossl-modules     pkgconfig

@mkitti
Copy link
Contributor

mkitti commented Jan 6, 2024

@eschnett

With 48e67fc I can build for Windows for C and C++. The main trick was export CFLAGS="$CLFAGS -L${prefix}/lib64" in order to detect libssl.

Let me see if I can sort out Fortran on Windows.

@eschnett
Copy link
Contributor Author

eschnett commented Jan 6, 2024

Fortran support currently still depends on H5detect. Let me see whether I can remove this completely.

@eschnett
Copy link
Contributor Author

eschnett commented Jan 6, 2024

The HDF5 configure script still calls AC_RUN_IFELSE when configuring Fortran. We still need to use our magic to specify which types are available in Fortran. (We're not calling H5detect any more but are still calling two other executables.)

@mkitti
Copy link
Contributor

mkitti commented Jan 6, 2024

I removed the MSYS2 dependencies and added lib64 to CFLAGS and FCFLAGS here:
eschnett#6
Please merge.

I've posted a preview here for testing:
https://github.com/mkitti/HDF5_jll.jl

I ran into some complications with Microsoft MPI, so that is not truly enabled at the moment, but I think this is a step in the right direction.

Fortran and C++ libraries are included in these builds, meaning we can now enable all the library and executable products.

Copy link
Contributor

@mkitti mkitti left a comment

Choose a reason for hiding this comment

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

Looks good to me. The last thing I can think of is fixing the following, but I think this is a already a major leap in the right direction.

┌ Warning: Dependency("MPIPreferences") was defined as top-level but this is deprecated, use `RuntimeDependency` instead
└ @ BinaryBuilderBase ~/.julia/packages/BinaryBuilderBase/tGUXK/src/Dependencies.jl:131

@mkitti
Copy link
Contributor

mkitti commented Jan 8, 2024

I tested against the current HDF5.jl, all tests pass.

Test Summary: | Pass  Broken  Total     Time
HDF5.jl       | 1563       3   1566  2m02.9s
     Testing HDF5 tests passed 

Ping @simonbyrne , @musm

@eschnett
Copy link
Contributor Author

eschnett commented Jan 8, 2024

@mkitti The MPIPreferences warning is caused by an upstream problem, that's not something we can fix.

@@ -6,54 +6,18 @@ const YGGDRASIL_DIR = "../.."
include(joinpath(YGGDRASIL_DIR, "platforms", "mpi.jl"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the idea is that we would remove this line and replace it with a RuntimeDependency, but I'm a bit confused as well.

@giordano ?

Copy link
Member

Choose a reason for hiding this comment

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

Why replacing that line? That'd break everything. At most you want to try and change

push!(dependencies, Dependency(PackageSpec(name="MPIPreferences", uuid="3da0fdf6-3ccc-4f1b-acd9-58baa6c99267"); compat="0.1", top_level=true))
to a runtime dep, but I seem to remember that had some problems.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm starting to follow. You're saying we need to edit Yggdrasil/platforms/mpi.jl by perhaps reverting c4069bf.

Copy link
Member

Choose a reason for hiding this comment

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

Only if it solves #6417 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking we should leave this for another pull request since fixing that will probably hit multiple recipes.

I'm good to merge this as is now if Erik is good.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think #7908 (comment) was addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giordano That's the comment regarding OpenMP? What is unresolved there? I think we all agree that OpenMP isn't used and isn't mentioned, and since @mkitti removed the respective dependency all is good?

Apart from this: Yes, we can merge.

Comment on lines -424 to -429
# For OpenMP we use libomp from `LLVMOpenMP_jll` where we use LLVM as compiler (BSD
# systems), and libgomp from `CompilerSupportLibraries_jll` everywhere else.
Dependency(PackageSpec(name="CompilerSupportLibraries_jll", uuid="e66e0078-7015-5450-92f7-15fbd957f2ae");
platforms=filter(!Sys.isbsd, platforms)),
Dependency(PackageSpec(name="LLVMOpenMP_jll", uuid="1d63c593-3942-5779-bab2-d838dc0a180e");
platforms=filter(Sys.isbsd, platforms)),
Copy link
Contributor

Choose a reason for hiding this comment

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

@giordano OpenMP was removed as a dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't see that addressed in the comments after my comment, I missed it had been changed before! The diff was quite large 🥲

@giordano
Copy link
Member

giordano commented Jan 8, 2024

Are we sure this build won't break ABI on Windows compared to our v1.14.2 build which was using prebuilt binaries?

@mkitti
Copy link
Contributor

mkitti commented Jan 8, 2024

NetCDF seems to work.

(jl_EGPgvN) pkg> st
Status `C:\Users\kittisopikulm\AppData\Local\Temp\jl_EGPgvN\Project.toml`
  [30363a11] NetCDF v0.11.7
  [0234f1f7] HDF5_jll v1.14.3+0 `https://github.com/mkitti/HDF5_jll.jl#main`
  [7243133f] NetCDF_jll v400.902.208+0

julia> using NetCDF

(jl_EGPgvN) pkg> test NetCDF
     Testing NetCDF
      Status `C:\Users\kittisopikulm\AppData\Local\Temp\jl_St2oZ9\Project.toml`
  [3c3547ce] DiskArrays v0.3.22
  [59287772] Formatting v0.4.2
  [30363a11] NetCDF v0.11.7
  [7243133f] NetCDF_jll v400.902.208+0
  [9a3f8284] Random
  [8dfed614] Test
      Status `C:\Users\kittisopikulm\AppData\Local\Temp\jl_St2oZ9\Manifest.toml`
  [3c3547ce] DiskArrays v0.3.22
  [59287772] Formatting v0.4.2
  [692b3bcd] JLLWrappers v1.5.0
  [3da0fdf6] MPIPreferences v0.1.10
  [30363a11] NetCDF v0.11.7
  [6fe1bfb0] OffsetArrays v1.13.0
  [21216c6a] Preferences v1.4.1
  [6e34b625] Bzip2_jll v1.0.8+0
  [0234f1f7] HDF5_jll v1.14.3+0 `https://github.com/mkitti/HDF5_jll.jl#main`
  [94ce4f54] Libiconv_jll v1.17.0+0
  [7cb0a576] MPICH_jll v4.1.2+1
  [f1f71cc9] MPItrampoline_jll v5.3.1+1
  [9237b28f] MicrosoftMPI_jll v10.1.4+1
  [7243133f] NetCDF_jll v400.902.208+0
⌃ [fe0851c0] OpenMPI_jll v4.1.6+0
  [458c3c95] OpenSSL_jll v3.0.12+0
  [02c8fc9c] XML2_jll v2.12.2+0
  [3161d3a3] Zstd_jll v1.5.5+0
  [477f73a3] libaec_jll v1.0.6+1
  [0dad84c5] ArgTools v1.1.1
  [56f22d72] Artifacts
  [2a0f44e3] Base64
  [ade2ca70] Dates
  [f43a241f] Downloads v1.6.0
  [7b1f6079] FileWatching
  [b77e0a4c] InteractiveUtils
  [4af54fe1] LazyArtifacts
  [b27032c2] LibCURL v0.6.4
  [76f85450] LibGit2
  [8f399da3] Libdl
  [56ddb016] Logging
  [d6f4376e] Markdown
  [ca575930] NetworkOptions v1.2.0
  [44cfe95a] Pkg v1.10.0
  [de0858da] Printf
  [3fa0cd96] REPL
  [9a3f8284] Random
  [ea8e919c] SHA v0.7.0
  [9e88b42a] Serialization
  [6462fe0b] Sockets
  [fa267f1f] TOML v1.0.3
  [a4e569a6] Tar v1.10.0
  [8dfed614] Test
  [cf7118a7] UUIDs
  [4ec0a83e] Unicode
  [e66e0078] CompilerSupportLibraries_jll v1.0.5+1
  [deac9b47] LibCURL_jll v8.4.0+0
  [e37daf67] LibGit2_jll v1.6.4+0
  [29816b5a] LibSSH2_jll v1.11.0+1
  [c8ffd9c3] MbedTLS_jll v2.28.2+1
  [14a3606d] MozillaCACerts_jll v2023.1.10
  [83775a58] Zlib_jll v1.2.13+1
  [8e850ede] nghttp2_jll v1.52.0+1
  [3f19e933] p7zip_jll v17.4.0+2
        Info Packages marked with ⌃ have new versions available and may be upgradable.
Precompiling project...
  20 dependencies successfully precompiled in 11 seconds. 3 already precompiled.
     Testing Running tests...
Test Summary: | Pass  Total   Time
NetCDF        |   28     28  16.5s

@giordano
Copy link
Member

giordano commented Jan 8, 2024

Ok, that's promising, thanks!

@giordano giordano merged commit 8e03326 into JuliaPackaging:master Jan 8, 2024
35 checks passed
@eschnett eschnett deleted the eschnett/hdf5_1_14_3 branch January 12, 2024 14:11
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.

None yet

5 participants