Closed jonassmedegaard closed 7 months ago
Oh, and full credit for the above patch goes to @plugwash - I am simply forwarding here what he offered for the Debian packaging.
Thanks for taking the time to get this to work. This library has a long history, so I'm not surprised there are a few bumps in the road.
These two features have been considered always available to the tests. Unit tests have to run with the std
library enabled (unless that has changed), so disabling the std
feature would historically either be a hassle or not change much, although now there are more features that use Vec
and Box
. The approx
feature is always there for floating point comparisons. Many of the test cases produce some small inaccuracies (sometimes platform dependent) and need less precise comparison than complete equality.
I suppose the std
feature could be made optional for the tests. I should add an alloc
feature, but that's perhaps for later. I'm not sure disabling approx
is meaningful, since it should just result in a lot of the most important test cases being disabled. 🤔
Hello again! I'm finally looking through these issues again (I needed more of a break than first expected), and just want to know if this is a blocking issue for you. I'm most likely not gating the test functions on approx
since it's such an essential part (only optional because users may not need it), but std
could be added although preferably also with alloc
separated out.
Since only tests fail, it is not exactly blocking: I do have the option of skipping tests failing due to this. Obvious problem in that is that I may mask detection of other bugs by doing so.
What I do now is apply the patch, and I intent on attempting to massage that patch to match newer releases for as long as I can - and if at some point I am no longer able to keep that hack of mine alive and you haven't addressed this issue by then, I will shift the the lesser preferred option of sloppily skipping tests that fail.
Thanks for all your work on this.
Hmm, that's not ideal. :grimacing: Are the tests only running the features one by one, or do they also try combinations? I'm asking because it's likely that some tests will need both approx
and some other feature. If not already now, then perhaps in the future, if it turns out to be a good idea to give color spaces their own features for faster compile time. It wouldn't be great if they don't run at all because of that.
If it's not possible for you to run the tests with approx
always enabled, it may at least perhaps be possible to find some option where it's not sprinkled all over the place. I should also go through them and see how many actually need it in that case. :thinking:
It is possible for me to do whatever adjustments to tests needed - including always adding approx
feature.
Sorry that I seemingly failed to get my point across: I am flexible, and as a last resort I can simply skip tests.
...and to answer your concrete question: No, what I practice currently is not a full combinational matrix of features, only each single feature done alone, and all features, and no features.
Thanks that's good to know, so I know what's expected to work. 👍
If it's not too much of a hassle, I think it's a safer bet to include approx
than to use this patch. At least while they are written for that assumption. I can still check what it would take to avoid that special case, though, other than what's suggested in the patch.
Like I mentioned in the PR, this patch should be unnecessary after the next release. Just keep in mind that a couple of tests use both approx
and alloc
, in case you want to be extra thorough.
When built without default features, only with std feature, a bunch of tests fail.
This can be fixed by adding appropriate feature guards, like this: