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

vigra: enable tests #375853

Merged
merged 4 commits into from
Jan 26, 2025
Merged

vigra: enable tests #375853

merged 4 commits into from
Jan 26, 2025

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Jan 22, 2025

This PR enables testing of upstream-provided test cases for the package vigra. and switch on the tests during the build.

The test cases took a significant amount of time to run (several minutes, while the build only took 15 seconds). Whether to enable the test during build or in a package test (under passthru.tests) is debatable.

Update: Now the test is available as vigra.tests.check.

Benchmark on a 8-core, 16 logical core laptop:

singlethreaded parallel
doCheck = false 10s 36s
doCheck = true 2m7s 12m52s
nixpkgs_losslesscut on  vigra-test [$] nix-store --gc
...

nixpkgs_losslesscut on  vigra-test [$] nix build --no-link -f . vigra.inputDerivation

nixpkgs_losslesscut on  vigra-test [$] took 40s time nix build --no-link --impure --expr "(import ./. { }).vigra.overrideAttrs { enableParallelBuilding = true; doCheck = false; }"

real	0m10.108s
user	0m0.560s
sys	0m0.190s

nixpkgs_losslesscut on  vigra-test [$] took 10s time nix build --no-link --impure --expr "(import ./. { }).vigra.overrideAttrs { enableParallelBuilding = false; doCheck = false; }"

real	0m36.278s
user	0m0.601s
sys	0m0.171s

nixpkgs_losslesscut on  vigra-test [$] took 36s time nix build --no-link --impure --expr "(import ./. { }).vigra.overrideAttrs { enableParallelBuilding = true; doCheck = true; }"

real	2m7.454s
user	0m0.724s
sys	0m0.253s

nixpkgs_losslesscut on  vigra-test [$] took 2m7s time nix build --no-link --impure --expr "(import ./. { }).vigra.overrideAttrs { enableParallelBuilding = false; doCheck = true; }"

real	12m52.407s
user	0m1.290s
sys	0m0.486s

nixpkgs_losslesscut on  vigra-test [$] took 12m52s 

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ShamrockLee
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 375853


x86_64-linux

❌ 4 packages failed to build:
  • digikam
  • enblend-enfuse
  • hugin
  • saga
✅ 1 package built:
  • vigra

aarch64-linux

❌ 5 packages failed to build:
  • digikam
  • enblend-enfuse
  • hugin
  • saga
  • vigra

@ShamrockLee
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 375853


x86_64-linux

✅ 5 packages built:
  • digikam
  • enblend-enfuse
  • hugin
  • saga
  • vigra

aarch64-linux

✅ 5 packages built:
  • digikam
  • enblend-enfuse
  • hugin
  • saga
  • vigra

@ShamrockLee
Copy link
Contributor Author

@autra, could you help review this PR? With parallel build enabled, the build time should be faster.

Copy link
Contributor

@autra autra left a comment

Choose a reason for hiding this comment

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

Code looks fine to me. The build time doesn't seem too bad. I've seen plenty of packages that takes a lot longer (ifcopenshell build itself is far longer, gdal tests are far longer too etc), so I'd vote to include the tests. I don't know if there is a global policy to choose between the 2 though.

@ShamrockLee ShamrockLee merged commit 2ccc5f1 into NixOS:master Jan 26, 2025
73 of 75 checks passed
@ShamrockLee ShamrockLee added the backport release-24.11 Backport PR automatically label Jan 26, 2025
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Jan 26, 2025

Successfully created backport PR for release-24.11:

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

Successfully merging this pull request may close these issues.

3 participants