python3Packages.dendropy: 4.5.1 -> 5.0.2 #326199
Conversation
|
Result of 4 packages built:
|
9ba2a70 to
982f067
Compare
5c4347c to
2bd2d34
Compare
|
Result of 5 packages built:
|
2bd2d34 to
167d42c
Compare
|
|
||
| pythonImportsCheck = [ "dendropy" ]; | ||
|
|
||
| meta = with lib; { | ||
| env.DENDROPY_PAUP_EXECUTABLE_PATH = if paupIntegration then lib.getExe paup else "NONE"; |
There was a problem hiding this comment.
It's a bit tricky.
It seems reasonable to leave it to upstream to make sure the paup integration works, and we disable it.
There was a problem hiding this comment.
Annoyingly, upstream doesn't make sure the paup integration works. If you remove this line, you'll get failed tests because it can't find the paup binary.
There was a problem hiding this comment.
In any case, paup is marked as unfree, so our CI will not run the test either.
There was a problem hiding this comment.
In any case, paup is marked as unfree, so our CI will not run the test either.
There are four tests that test PAUP integration. They are disabled when DENDROPY_PAUP_EXECUTABLE_PATH is set to NONE. They are not disabled when the DENDROPY_PAUP_EXECUTABLE_PATH environment variable is not set. Then, dendropy just assumes that PAUP is in the PATH.
Specifically, those tests are:
"test_basic_split_count_with_incorrect_rootings_raises_error"
"test_basic_split_count_with_incorrect_weight_treatment_raises_error"
"test_basic_split_counting_under_different_rootings"
"test_group1"
These tests used to be disabled. However, I changed this so that those tests can be run if PAUP is enabled.
Also worth noting: the author of PAUP has plans to make it open-source later this year.
There was a problem hiding this comment.
The upstream looks like it is running an integration test.
https://github.com/jeetsukumaran/DendroPy/blob/a27a6d46e5a7e1c9573b3022a18f3005cd8b996e/.github/workflows/ci.yaml#L30-L37
I'm not willing to expose checkValidity just for the four tests in this package.
There was a problem hiding this comment.
You're right in regards of just four unit tests being insufficient to expose checkValidity. I meant to additionally figure out a way to make PAUP* integration work for dependents of dendropy.
Also, there are a couple other packages where access to checkValidity is useful (#310138 integration with monado, for example). In fact, any open source project with an optional non-free dependency could really make use of checkValidity.
There was a problem hiding this comment.
I'm not very familiar with check-meta, so I can't decide if it should be exposed.
However, I agree that there are a number of useful examples.
How about implementing this suggestion in another PR?
I think it would get more people involved in the discussion.
There was a problem hiding this comment.
Okay, then I will make it a separate PR.
167d42c to
8a2e721
Compare
67a65c8 to
6a3575c
Compare
|
Updated to 5.0.2 Would really like this to be merged. It's blocking #311463 |
|
Result of 5 packages built:
|
6a3575c to
c53bb22
Compare
|
Had some nixfmt errors. Those are now fixed. |
c53bb22 to
6d1c50c
Compare
6d1c50c to
0c28817
Compare
|
Result of 5 packages built:
|
|
Result of 2 packages built successfully:
|
|
Not backporting because this is a major version bump. |
Description of changes
Updates
dendropyto the latest version, revamps the derivation, and adds myself as maintainer.Please see my comments for explanation about some of the changes I made.
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.