hdf5: refactor to reduce downstream closure size #277224
Merged
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
|
Running nixpkgs-review now ... |
I needs to build 5000 packages now. I'll wait until more packages are built by Hydra. |
I just learned that there is no binary cache available for |
|
Hmm, well this branch was based on the merge-base of staging and master so that shouldn't be an issue if you're testing the branch head. AFAIR nixpkgs-review uses a merge commit so maybe that's why you're noticing caching issues with that tool, idk if that's configurable. It should be feasible to build all the changed derivations vs. |
|
Oh, I remembered how I've sometimes tested staging PRs in the past. You can check out a PR locally and run I'm running that now, will report back. Some of these leaf packages are pretty hefty so might take a day or so. Current status:
|
Thank you for suggestion. I tried similar approach. I ran nixpkgs-review against in hope to reduce number of required builds, but it was still trying to build few thousands of packages. |
|
Result of 47 packages marked as broken and skipped:
48 packages failed to build:
1174 packages built:
|
|
Thank you very much for running this big nixpkgs-review. I don't see a problem with this PR from geospatial packages side. Packages like bsuite, plotnine and rlax are always failing (should be marked as broken). |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/week-in-geospatial-team/37035/2 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/week-in-geospatial-team/37035/3 |
markuskowa
requested changes
Jan 16, 2024
There was a problem hiding this comment.
The names of the tools in the bin output are appended with -shared: e.g. h5ls-shared.
This was already broken before, but no one noticed because there was the corresponding static binary without shared next to it.
Can we please fix the names of the tools?
Looks good otherwise.
@r-burns , do you want to fix those remaining issues so we can merge this PR ? |
|
Ah whoops, missed that review suggestion. I'm really busy with personal stuff the next few days but I should be able to make that change next week. |
r-burns
force-pushed
the
hdf5-closure-size
branch
from
21424f2
to
afe4a52
Compare
markuskowa
reviewed
Mar 1, 2024
* Only build static libs by default when stdenv.hostPlatform.isStatic * Add separate -bin output * Move h5hlcc/c++ dev binaries to -dev output * Default to building with C++ support to cut down on duplicated hdf5/hdf5-cpp in downstream closures
r-burns
force-pushed
the
hdf5-closure-size
branch
from
3f90a0e
to
9046690
Compare
markuskowa
approved these changes
Mar 4, 2024
|
Fails on staging-next, because of missing https://hydra.nixos.org/build/253019375 |
12 tasks
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.
cc @NixOS/geospatial - this is motivated by geospatial packages, e.g. gdal
Description of changes
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.