Skip to content

Conversation

hmaarrfk
Copy link
Contributor

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Mar 21, 2025

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). This parser is not currently used by conda-forge, but may be in the future. We are collecting information to see which recipes are compatible with grayskull.
  • ℹ️ The recipe is not parsable by parser conda-recipe-manager. The recipe can only be automatically migrated to the new v1 format if it is parseable by conda-recipe-manager.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13983615571. Examine the logs at this URL for more detail.

@hmaarrfk hmaarrfk force-pushed the fix_url_cmake branch 2 times, most recently from 52e40d4 to 5334645 Compare March 21, 2025 01:56
recipe/meta.yaml Outdated
{% set maj_min_ver = ".".join(version.split(".")[:2]) %}
{% set build = 5 %}
{% set version = "1.14.6" %}
{% set maj_min_patch_ver = "_".join(version.split(".")) %}
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be easier to just use {{ version|replace(".", "_") }} in the url, rather than introduce another variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
{% set maj_min_patch_ver = "_".join(version.split(".")) %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but you also need 1_14 and not just 1_14_6 this kind of churn isn't necessary.

Lets focus on the cmake imporvements and the version update.

@hmaarrfk hmaarrfk mentioned this pull request Mar 21, 2025
5 tasks
Comment on lines -119 to -124
{% set hdf5_unix_cmds = h5_compilers + [
"h5redeploy",
] %}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i believe that this is an automake style tool

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint --conda-forge . from the recipe directory. You can also examine the workflow logs for more detail.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13983965316. Examine the logs at this URL for more detail.

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Mar 21, 2025

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). This parser is not currently used by conda-forge, but may be in the future. We are collecting information to see which recipes are compatible with grayskull.
  • ℹ️ The recipe is not parsable by parser conda-recipe-manager. The recipe can only be automatically migrated to the new v1 format if it is parseable by conda-recipe-manager.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/16837980984. Examine the logs at this URL for more detail.

@hmaarrfk
Copy link
Contributor Author

fingers crossed i skipped enough tests for linux.

Then it is off to osx cross compilation issues.

@hmaarrfk
Copy link
Contributor Author

@ajelenak is there some existing HDF5 build constants that you can point me to in order to bypass the checks for type sizes for OSX + arm64

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Apr 4, 2025

@ajelenak @conda-forge/hdf5 i'm not sure if anybody else can jump in here. I mostly have trouble ensuring that the constants I define are getting passed through t the compilation settings.

I would love to release 1.14.6 in April, but I'm not sure I will have the investigate this.

LMK if anybody can help, otherwise the cmake transition may have to wait till 1.15 and we should merge #254 first.

@ajelenak
Copy link

ajelenak commented Apr 4, 2025

@hmaarrfk Where do you have the most problems? Specific platforms? Compilers? Modifying so many tests does not seem right. Is there a common underlying problem?

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Apr 4, 2025

Hi @ajelenak thanks for chiming in.

Where do you have the most problems?

The bigges problems are getting through the cross compilation settings for OSX ARM. We build using a x86-64 runner, and target ARM. without emulation, alot of the "discovery code" that detects the word sizes just fails.

I don't understand the build system enough to create our own overrides.

Modifying so many tests does not seem right. Is there a common underlying problem?

Generally speaking:

  1. Underpowered runners
  2. Parallel tests
  3. Timeouts
  4. Architecture Emulation
  5. Buggy emulation of advanced instructions

make it so that high performance scientific code fails tests. Each skipped test is thematically related to one of the 5 points above and the tests were historically skipped on conda-forge. I've just had to port over the skips to cmake (I still have some skips to port).

@ajelenak
Copy link

ajelenak commented Apr 4, 2025

It looks like it may take a while to address these kinds of problems. If you have a quicker option to release 1.14.6, I suggest to use it.

recipe/build.sh Outdated
export H5_PAC_FORTRAN_NATIVE_INTEGER_KIND=" 4"
export H5_PAC_FORTRAN_NATIVE_REAL_SIZEOF=" 4"
export H5_PAC_FORTRAN_NATIVE_REAL_KIND=" 4"
export H5_PAC_FORTRAN_NATIVE_DOUBLE_SIZEOF=" 8"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajelenak are you saying that this kind of setting of environment variable does not ovewrite

these definitions?
https://github.com/HDFGroup/hdf5/blob/407caf1259dd03685b411f78321f823abd2666a6/config/cmake/HDF5UseFortran.cmake#L424

Copy link

Choose a reason for hiding this comment

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

I don't know, have to ask our CMake expert. I have only set CMake variables on the command line (-D option).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be very helpful in terms of helping the community improve the state of HDF5 support

Copy link

Choose a reason for hiding this comment

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

Setting env. variables is not going to affect those CMake variables. Options are setting them on the command line or in a custom cacheinit.cmake file which needs to be specified on the command line.

I know this is not the right place to ask but... is it possible to build conda packages somewhere else? I can build the library in a conda environment on my macOS arm64 laptop without any problem. If all the build tools are available to be installed in a conda environment, would it be possible to use GitHub actions to create conda packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are "plans" to use github runners for OSX builds, but specific platforms are always limited. And it is difficult to guarantee access to the exact hardware. flexibility is "preffered". conda-forge/conda-forge.github.io#1781

I need to read through HDFGroup/hdf5#1203 but there is obviously a great desire to have cross compilation ability in the community beyond just OSX.

I guess you asked how you can help, helping us through the autotools -> cmake transition would be a great help.

If you had time to write a patch that enables these constants to be hardcoded that would go a long way already!!!

If you don't have time that's ok too. Maybe just let me know, and we can delay the whole cmake transition until 1.15

@Krande
Copy link

Krande commented Apr 8, 2025

@hmaarrfk Intel just released IFX on conda-forge for windows in conda-forge/intel-compiler-repack-feedstock#57. If you give me a few days I can try to add support for fortran in HDF5 for windows?

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Apr 9, 2025

@hmaarrfk Intel just released IFX on conda-forge for windows in conda-forge/intel-compiler-repack-feedstock#57. If you give me a few days I can try to add support for fortran in HDF5 for windows?

ok sounds great.

I don't know if you want to use Cmake or autotools. Somebody needs to write a cmake patch for osx cross compilation.

@Krande
Copy link

Krande commented Apr 10, 2025

@hmaarrfk Intel just released IFX on conda-forge for windows in conda-forge/intel-compiler-repack-feedstock#57. If you give me a few days I can try to add support for fortran in HDF5 for windows?

ok sounds great.

I don't know if you want to use Cmake or autotools. Somebody needs to write a cmake patch for osx cross compilation.

I have successfully added fortran support for windows using intel ifx for the nompi variant in the draft PR #256. I don't know enough about impi to know how to support that with ifx.

It should be safe to merge these changes into this PR.

Regarding

Somebody needs to write a cmake patch for osx cross compilation

I don't have access to any osx machine, so I might not be the best candidate for this. But I can probably find some time in the coming days to give it a shot (although I cannot make any promises).

@hmaarrfk
Copy link
Contributor Author

I don't have access to any osx machine, so I might not be the best candidate for this. But I can probably find some time in the coming days to give it a shot (although I cannot make any promises).

do you have access to a linux machine? You could uninstall qemu and ensure that it can be compiled without emulation, that is the big challenge.

@hmaarrfk
Copy link
Contributor Author

It should be safe to merge these changes into this PR.

if you could make it a single commit for me to cherry pick (keep the rerendering as a separate commit) that would make it easy to integrate here.

@Krande
Copy link

Krande commented Apr 10, 2025

if you could make it a single commit for me to cherry pick (keep the rerendering as a separate commit) that would make it easy to integrate here.

I squashed the commits and made a single commit containing the changes. Hopefully that can be used: Krande@6e1eb99

(btw. I closed my test PR to limit the number of concurrent jobs.)

do you have access to a linux machine? You could uninstall qemu and ensure that it can be compiled without emulation, that is the big challenge.

Yes, I have access to a linux machine. I can give it a shot

@@ -72,6 +72,7 @@ requirements:
- {{ stdlib('c') }}
- {{ compiler('cxx') }}
- {{ compiler('fortran') }} # [not win]
- ifx_win-64 # [win and mpi == 'nompi']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Krande this is honestly a little strange. I believe that the different variants of HDF5 are abi compatible. However, here you are omitting the fortran libraries for anything where mpi != 'nompi' is it possible to enable it on all MPI?

Copy link
Member

Choose a reason for hiding this comment

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

Also, instead of hardcoding this in the recipe, it would be better to remove the selectors here and just add

fortran_compiler:  # [win]
  - ifx            # [win]

in CBC

Copy link

@Krande Krande Apr 21, 2025

Choose a reason for hiding this comment

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

@hmaarrfk I have no experience with impi and it failed to compile when I tested last time locally, so I just omitted it in my PR. But I will take a another look at adding impi support in a new PR (see #257) based on the latest merged main and see if it I can figure it out.

@hmaarrfk
Copy link
Contributor Author

I've manually checked:

The first three don't even depend on cmake, Arch references, but then uses autotools.

Ultimately, we would be trailblazing here, and unless we get help from upstream, I am tempted to move back to #254

@hmaarrfk
Copy link
Contributor Author

@mgorny is there any synergy with gentoo package manager in moving from autotools to cmake and ensuring cross compilation works without emulation?

I think it needs some patches from some1 knowledgeable in cmake and I'm not sure if you are one that can help.

@mgorny
Copy link

mgorny commented Apr 17, 2025

Hmm, I see they intend to drop autotools entirely, so I guess we'll have to do that. Yeah, I'll ping our folk and see if we can do something, either me or somebody else. That said, cross is not high on our priority list, so we'll probably only be addressing the problems that affect native builds.

@mgorny
Copy link

mgorny commented Apr 17, 2025

Okay, I've asked around and we've tried the CMake build recently and it was really bad. Unfortunately, the problem is not CMake build per se but upstream deciding to make breaking changes to h5cc when building with CMake, which means reverse dependencies do not build anymore, e.g.:

And upstream's response is basically:

CMake h5cc changes have been noted in the Release.txt at the time of change - archived copy should exist in the history files.

So basically they require you to update all the reverse dependencies before you switch to CMake. I don't know if any progress has been made on this, or if reverse dependency maintainers are even aware of the problem.

@mgorny
Copy link

mgorny commented Apr 21, 2025

Small update: I've looked around and it is possible that the original issue was fixed after all (or at least there are some commits that look like a fix) and the bug wasn't closed. I'm going to try to find some time to investigate this.

@hmaarrfk
Copy link
Contributor Author

@mgorny in terms of "urgency" i've decided to "punt this" until 1.15 because #254 was ready for 1 month and the author had been patiently waiting.

it would be great to be "prepared" for this, but it also seems like there are some efforts in making native OSX-arm compilation a possibility.

However, any help getting cmake working with cross compilation would definitely be helpful.

However, if we try to do it for the same HDF5 version, we need to make sure that the Autotools ABI is compatible with that of the cmake
https://conda-forge.org/docs/maintainer/knowledge_base/#moving-from-an-autotools-build-to-a-cmake-build

We also just got the gift of this beautiful bug: #258

so ...

@mgorny
Copy link

mgorny commented Apr 22, 2025

I'll try to tackle it on the Gentoo end — here we can just bump subslot if necessary to indicate ABI incompatibility, and mask the new version for local testing.

@mgorny
Copy link

mgorny commented Apr 23, 2025

Okay, some good news is that it seems that CMake is now installing usable h5cc. For 1.14.6, I have used the following pull requests:

  1. Fixes for compiler wrappers and cmake config file dirs. HDFGroup/hdf5#5361 — this fixes paths in h5cc
  2. h5cc shell portability fixes HDFGroup/hdf5#5465 — here I fixed compatibility with POSIX sh (which regressed in the previous PR) and fixed quoting
  3. h5cc: Allow overriding the compilers written into the file HDFGroup/hdf5#5467 — here I've added CMake vars to let people override the compiler used by h5cc and so on (the problem was originally noticed by openSUSE, https://build.opensuse.org/projects/science/packages/hdf5/files/hdf5.spec?expand=1)

This is my work-in-progress, and I'm in process of testing reverse dependencies now: gentoo/gentoo#41704

Of course, I don't necessarily know what I'm doing.

@mgorny
Copy link

mgorny commented Apr 23, 2025

Okay, so far the conclusion is that I haven't seen a single package failing with hdf5 CMake version that worked with the autotools version. I've found other issues, such as packages not using hdf5 when --enable-hdf5 is passed due to messed up configure script, bugs in Gentoo packages, incompatibilities with new SWIG or GCC, but not a single failure resulting from use of CMake.

If you need to work on more specific CMake problems, please let me know what they are :-).

@hmaarrfk
Copy link
Contributor Author

single package failing with hdf5 CMake version that worked with the autotools version.

so are you saying that it is likely that CMake + autotools are ABI compatible in this case?
If you think so, we can likely do the cmake conversion before the 1.15.0 release. I was mostly worried about ABI compatibility

CMake problems

For conda-forge, the biggest challenge stems from the (in)ability to override certain build time variables such as
PAC_FORTRAN_NATIVE_INTEGER_SIZEOF https://github.com/HDFGroup/hdf5/blob/hdf5_1.14.6/config/cmake/HDF5UseFortran.cmake#L377

I am hoping to gather more information here, and make a plea upstream to have "more first class support for cross compilation", instead of asking downstream packagers to make their own patches.

It could be a win-win for all,

@mgorny
Copy link

mgorny commented Apr 23, 2025

so are you saying that it is likely that CMake + autotools are ABI compatible in this case?
If you think so, we can likely do the cmake conversion before the 1.15.0 release. I was mostly worried about ABI compatibility

No clue about ABI compatibility. I just meant that h5cc regressions have been fixed, and they restored API / build system compatibility.

To be honest, I've taken ABI incompatibility at your word and just immediately marked it as incompatible.

@mgorny
Copy link

mgorny commented Apr 23, 2025

There's at least incompatibility in Fortran libraries:

 *  SONAME:-libhdf5_f90cstub.so.310(64)
 *  SONAME:-libhdf5_hl_f90cstub.so.310(64)
 *  SONAME:-libhdf5_hl_fortran.so.310(64)
 *  SONAME:-libhdf5_tools.so.310(64)
 *  SONAME:+libhdf5hl_fortran.so.310(64)

- here are additional libraries installed by CMake, + by autotools.

@hmaarrfk
Copy link
Contributor Author

To be honest, I've taken ABI incompatibility at your word and just immediately marked it as incompatible.

To be honest, the process of marking ABI compatible/incompatible after packages have been built is rather annoying at conda-forge.

So its easier to use the standard run_exports and rebuild.

The rebuilding process also helps "refresh" recipes so i'm not always opposed.

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.

6 participants