fix either feature conditional compilation, again

[davidhewitt]

#3722 (comment), thanks for reporting @utkarshgupta137

[davidhewitt]

For what it's worth, I didn't fancy adding a test for this feature combination in CI, as testing the full set of all combinations of features seems impractical 😢

[codspeed-hq]

CodSpeed Performance Report

Merging #3834 will not alter performance

_{Comparing davidhewitt:either-type (51c53b0) with main (61bc02d)}

Summary

✅ 67 untouched benchmarks

[adamreichold]

I wonder how this go through CI, don't we test a lot of feature combinations? I think this tells us that we need at least one more which does not include the experiment-inspect feature.

[adamreichold]

For what it's worth, I didn't fancy adding a test for this feature combination in CI, as testing the full set of all combinations of features seems impractical 😢

Sorry for missing that. I also don't we should test the full power set. But I would rather drop a combination that includes the experiment feature than not testing stable features without the experimental bit enabled.

[utkarshgupta137]

Not sure what you guys are using right now, but we use https://github.com/taiki-e/cargo-hack in our workspace. We don't do powersets, because we often have conflicting feature, but we test each feature individually. I believe that would've been enough to catch this.

[adamreichold]

I don't think testing all features individually is reasonable for PyO3 because we have many integration features like either which do not plausibly interact with each other. We mainly need to ensure that we test with/without the infrastructure features like experimental-inspect. So I think an adjustment of our current curated feature sets is still the most reasonable approach for us specifically.

[davidhewitt]

👍 (I'll try to get to this tomorrow or over the weekend.)

[davidhewitt]

I decided to go for a combination of both; I used the noxfile to build up groups of features to then pass on to cargo hack. It probably wouldn't have been much harder for us to use itertools.combinations to invoke cargo test directly but it's nice to use the higher-level tool and we already use lots of tools from taiki-e 🚀 .

[davidhewitt]

We could maybe extend to run this for each OS, but my expectation is that there's unlikely to be platform differences caught out by testing specific feature combinations. It seemed wasteful.

Similarly, if running a full test is too slow, we could downgrade to a check.

[adamreichold]

Similarly, if running a full test is too slow, we could downgrade to a check.

I think I'd prefer to start with a simple compile check instead of full test build. (The runtime is also where more platform-specific behaviour could lurk, and hence I'd say that single-platform-compile-check is a more natural working point.)

[utkarshgupta137]

That's what we do for our CI (though it is hard to compare library vs binary). We run all tests with no features enabled & all features enabled (Mac once a day, Linux every time, no Windows). Then we use cargo-hack to run check/clippy for all features one-by-one, but no powerset (that probably makes sense for a library though).

[davidhewitt]

The "powerset" I added here has just got three "groups" if you like - abi3, all optional features, and all experimental features. That powerset creates just 8 test runs, which is hopefully a manageable number for a single CI job (especially as I opted to skip doctests for this job).

[davidhewitt]

Ufff I missed installing rust-src, was why the tests failed I think. @LilyFoote just ran into the same thing locally, which is what jogged my memory. (#3575)

[adamreichold]

LGTM now. Only open question is whether to downgrade this to check builds (which I would prefer, but not strongly enough to further delay this).

[davidhewitt]

Let's just go for check. The odds of a bug actually existing in a codepath which exists only in an edge case combination which isn't run in the rest of CI seems not worth the CI cycles for now.

[davidhewitt]

Thanks for the rounds of review here! I will get on with preparing that patch release once this merges.