hdf5: 1.14.3 -> 1.14.4.3 #309595
Merged
hdf5: 1.14.3 -> 1.14.4.3 #309595
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@GrahamcOfBorg build blender h5py openmolcas trexio hdf5-mpi |
|
Running nixpkgs-review now ... |
|
Result of 74 packages marked as broken and skipped:
1101 packages failed to build:
96 packages built:
|
My 128 GB memory + 50GB swap machine went out of memory :). Restarted now. |
imincik
approved these changes
May 8, 2024
Mine is still running and nearly done. 140 packages to go, 1400 done 🤞 Majority seems to build fine. |
|
Alright, here is my full nixpkgs-review, finally finished Result of 78 packages marked as broken and skipped:
656 packages failed to build:
537 packages built:
|
sheepforce
approved these changes
May 13, 2024
|
The result are better when I apply the patch to master and run nixpkgs-review. Result of 72 packages marked as broken and skipped:
284 packages failed to build:
948 packages built:
|
|
@imincik note that this may break |
Great thanks for notification. Can you share the log output of |
|
Please don't merge this PR until we investigate and fix the breakages caused by this PR (if any). |
|
@imincik This is the relevant tail of the build log: |
afh
suggested changes
May 23, 2024
There was a problem hiding this comment.
Thank you @markuskowa for putting up this PR for review.
Please find attached a few suggestions for general improvement.
I've summarised a few additional points below where I wasn't able to comment on the code directly:
- Updating this package provides an opportunity to move it to
pkgs/by-name/hd/hdf5/package.nix ++ lib.optionals mpiSupport [ "-DHDF5_ENABLE_PARALLEL=ON" ]
could be simplifed to++ lib.optional mpiSupport "-DHDF5_ENABLE_PARALLEL=ON"with lib;should probably be removed frommeta(see Tracking issue: remove overuses ofwith lib;#208242)
pkgs/tools/misc/hdf5/default.nix
Outdated
| owner = "HDFGroup"; | ||
| repo = "hdf5"; | ||
| rev = "hdf5_${version}"; | ||
| sha256 = "sha256-nKrKGL/bMphJAm8OxvMV5hpBWBhHVNXANqtNv7ouvV0="; |
There was a problem hiding this comment.
ℹ️ There seems to be a preference for src.hash over src.sha256
Suggested change
| sha256 = "sha256-nKrKGL/bMphJAm8OxvMV5hpBWBhHVNXANqtNv7ouvV0="; | |
| hash = "sha256-nKrKGL/bMphJAm8OxvMV5hpBWBhHVNXANqtNv7ouvV0="; |
There was a problem hiding this comment.
I could have changed, but simply forgot. In general, I think this should be done by automatic tooling and a tree-wide cleanup action.
| @@ -28,21 +28,18 @@ assert !cppSupport || !mpiSupport; | |||
| let inherit (lib) optional optionals; in | |||
There was a problem hiding this comment.
Use of the optional and optionals defined here is inconsistent within this package as there are also uses of lib.optional and lib.optionals. I'd like to recommend to remove this definition here and replace uses optional and optionals with lib.optional and lib.optionals.
| @@ -28,21 +28,18 @@ assert !cppSupport || !mpiSupport; | |||
| let inherit (lib) optional optionals; in | |||
|
|
|||
| stdenv.mkDerivation rec { | |||
There was a problem hiding this comment.
There seems to be a preference for using finalAttrs over rec… (remember add ) at the end of the package to close the newly opened ( here)
Suggested change
| stdenv.mkDerivation rec { | |
| stdenv.mkDerivation (finalAttrs: { |
| src = fetchFromGitHub { | ||
| owner = "HDFGroup"; | ||
| repo = "hdf5"; | ||
| rev = "hdf5_${version}"; |
There was a problem hiding this comment.
If the finalAttrs suggestion above is accepted the following change is necessary:
Suggested change
| rev = "hdf5_${version}"; | |
| rev = "hdf5_${finalAttrs.version}"; |
Thanks for your feedback. However, I think your suggestions are beyond the scope of this PR and are better suited for a "refactor PR".
I think that the in the context of
From my understanding existing packages should remain where they are. They will me moved to pkgs/by-name in the future by means of automatic tooling. |
|
Thank you for considering the suggestions, @markuskowa, and taking the time to respond. I'm happy to put up a refactor PR, unless you as a maintainer feel inclined to do so. |
Feel free to open a PR. I am bit short on time at the moment, but I can review it. Just ping me. |
|
ℹ️ HDF5 14.4.3 was recently released. Would you be okay with updating this PR accordingly, @markuskowa? I'll put up a refactoring PR, once this has landed, unless someone can tell me how I can create a PR here that will have |
Switch downloads to GitHub
+1 for updating this PR. |
Fixes a problem with h5py, and fixes CVEs: https://github.com/HDFGroup/hdf5/releases I am running nixpkgs-review currently. Will push when finished. @imincik does not resolve the gdal build failure though. |
Thanks for checking. I should report this, but not sure where - gdal or hdf5 ? |
gdal-3.9.0 has been released in beginning of May. Is it an option to update to 3.9.0 instead (and hope that the issue disapears)? |
|
Result of 86 packages marked as broken and skipped:
193 packages failed to build:
1061 packages built:
|
Unfortunately, GDAL 3.9.0 is breaking some packages (#311310). But it can be worth of checking if it fails with hdf5 1.14.4.3. I'll try to build it now and will report back. |
|
Unfortunately GDAL 3.9.0 fails as well: |
|
GDAL build issue reported to HDF5 HDFGroup/hdf5#4527 |
|
Both GDAL 3.8 and GDAL 3.9 are building successfully when HDF5 is built with Patch: Do we want to accept it as the workaround or should we ask GDAL devs for some better solution ? |
|
GDAL fix - OSGeo/gdal#10052 |
That means you could apply the upstream patch to gdal-3.8.5? If so, should we merge this PR then? |
Unfortunately,
|
|
@imincik An alternative could be to make the float16 feature optional in HDF5 and turn it off by default. I assume that this feature targets AI workloads. I could give it a try. |
Unfortunately, that breaks |
Hi @markuskowa, it's likely that h5py/h5py#2422 will need to be merged for builds of h5py to work when float16 support is disabled in HDF5. |
I suggest to wait for fix in GDAL (OSGeo/gdal#10052) |
|
GDAL 3.9 is ready for review now. |
|
Now that GDAL 3.9.0 is merged, should we move forward with merging this PR? |
Yes ! |
|
Looks much better now: 98 packages marked as broken and skipped:
50 packages failed to build:
1284 packages built:
|
|
Great thanks @markuskowa |
|
FYI, this or something else recent broke x86_64-darwin build (only that one, I think) /cc staging-next #322145 (report says it blocks 375 builds) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Changelog: https://github.com/HDFGroup/hdf5/releases/tag/hdf5_1.14.4.2
Switch downloads to GitHub. It appears that the new prefered download location is now GitHub
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.