Skip to content

rapidfuzz-cpp: add passthru.tests.levenshtein#283915

Merged
dotlambda merged 1 commit intoNixOS:masterfrom
bryango:rapidfuzz-cpp-test-levenshtein
Jan 29, 2024
Merged

rapidfuzz-cpp: add passthru.tests.levenshtein#283915
dotlambda merged 1 commit intoNixOS:masterfrom
bryango:rapidfuzz-cpp-test-levenshtein

Conversation

@bryango
Copy link
Member

@bryango bryango commented Jan 26, 2024

As suggested by @dotlambda in #283726 (comment).

Description of changes

python3Packages.levenshtein crucially depends on rapidfuzz-cpp, so we add it to passthru.tests to prevent future breakage. The patch is targeted at master, as it should not cause any rebuild when the former staging-next is fully cached.

ofborg all green: https://github.com/NixOS/nixpkgs/pull/283915/checks?check_run_id=20887684631

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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

@bryango bryango requested a review from dotlambda January 26, 2024 03:36
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jan 26, 2024
Copy link
Contributor

@illustris illustris left a comment

Choose a reason for hiding this comment

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

Not a blocker for merging, but it might better to do

passthru.tests.levenshtein = python3Packages.levenshtein.override {
  rapidfuzz-cpp = finalAttrs.finalPackage;
};

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jan 27, 2024
@dotlambda
Copy link
Member

it might better to do

passthru.tests.levenshtein = python3Packages.levenshtein.override {
  rapidfuzz-cpp = finalAttrs.finalPackage;
};

Why so? Isn't the point to test the python3Packages.levenshtein package? If that ends up eventually overriding rapidfuzz-cpp but still builds, that's fine too.
I would just do

passthru.tests = {
  inherit (python3Packages) levenshtein;
};

@bryango bryango force-pushed the rapidfuzz-cpp-test-levenshtein branch from bc55ae7 to 14a72a3 Compare January 29, 2024 03:27
@bryango
Copy link
Member Author

bryango commented Jan 29, 2024

I would just do

passthru.tests = {
  inherit (python3Packages) levenshtein;
};

I am okay with either. I have simply reverted the last commit and now the code is equivalent to the suggestion here, albeit less idiomatic.

`python3Packages.levenshtein` crucially depends on `rapidfuzz-cpp`, so
we add it to `passthru.tests` to prevent future breakage.

Co-authored-by: Robert Schütz <github@dotlambda.de>
@bryango bryango force-pushed the rapidfuzz-cpp-test-levenshtein branch from 6bde094 to 450e082 Compare January 29, 2024 03:33
@bryango
Copy link
Member Author

bryango commented Jan 29, 2024

Done! Tested locally, looks good! @dotlambda

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Jan 29, 2024
@dotlambda dotlambda merged commit 24d9778 into NixOS:master Jan 29, 2024
@illustris
Copy link
Contributor

it might better to do

passthru.tests.levenshtein = python3Packages.levenshtein.override {
  rapidfuzz-cpp = finalAttrs.finalPackage;
};

Why so? Isn't the point to test the python3Packages.levenshtein package? If that ends up eventually overriding rapidfuzz-cpp but still builds, that's fine too. I would just do

passthru.tests = {
  inherit (python3Packages) levenshtein;
};

Depends on what you're trying to test. If you are just trying to test whether python3Packages.levenshtein builds with the default version of rapidfuzz-cpp, that is sufficient. Instead, if you want to test whether a modified version of the package, let's say with some patches (applied to it with overrideAttrs), would break python3Packages.levenshtein, this wouldn't be enough. The way it is now (with finalPackage), it can test both. passthru.tests.levenshtein = python3Packages.levenshtein; is less useful as a test than python3Packages.levenshtein.override {rapidfuzz-cpp = finalAttrs.finalPackage;};.

That being said, going back and forth on this change isn't worth blocking this PR. It can be merged with either version.

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

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants