rust-windowing / winit

Window handling library in pure Rust
https://docs.rs/winit/
Apache License 2.0
4.76k stars 892 forks source link

Add atypical targets to CI #3751

Open notgull opened 3 months ago

notgull commented 3 months ago

This adds some Tier-2 and Tier-3 targets to CI, so we can detect breakage in these platforms early.

cc #3735

notgull commented 3 months ago

Hmm, not sure about this, our CI is already quite big, I'd rather cut down on it. Though I guess that doing it this way (only running cargo check, and doing it in a single "job") helps somewhat.

The tradeoff is that otherwise we are completely unaware of such CI failures until they're reported to us. I figure that doing a check here is a good compromise between "no checks" and, say, using qemu to run a full test suite.

I'll note that Cargo supports compiling for multiple targets simultaneously (pass --target multiple times), might be worthwhile to check if that's faster (vs. the debugging experience if one target fails, of course).

The issue is that we also want to test various feature combinations as well, but at least this will narrow it down to three calls.

kchibisov commented 3 months ago

As for musl, we need a musl based image, I guess.

notgull commented 3 months ago

I'd suggest to add only platforms which are only actually different, as in, you should cover possibilities and not all the cases.

The issue is that platforms may eventually become different. For instance FreeBSD regularly has breaking changes where fundamental types change. So in order to insulate us from unexpected breakage we need to cast a pretty wide net.

I'm glad to reduce the permutations here, but I think that the net this PR casts is just wide enough to capture all the permutations we care about (e.g. x86_64 vs aarch64 vs s390x, the various BSDs). So I would appreciate guidance on which cases we are fine with breaking for users.

kchibisov commented 3 months ago

The issue is that platforms may eventually become different

If the platform does so, then all the C stack has the issues as well. In general, I'd advice to have some sort of a lint that detects that the matching type alias is used, and not just the original type. Because the issue you're trying to prevent was caused by the fact that typedef was not used, but rather the original type.

notgull commented 3 months ago

In general, I'd advice to have some sort of a lint that detects that the matching type alias is used, and not just the original type.

Does this lint exist in the current version of Rust? I'm not sure this works, and the only real way to check this is to actually call cargo check.

kchibisov commented 3 months ago

There's no way and no indications in clippy I've also opened an issue https://github.com/rust-lang/rust-clippy/issues/12988 to at least suggest correct types and not expanded.

kchibisov commented 3 months ago

For now, I'd suggest to just test arm build, because it covers the c_char difference.

notgull commented 3 months ago

For now, I'd suggest to just test arm build, because it covers the c_char difference.

I've added the other builds for the following reasons:

kchibisov commented 3 months ago

To make things work on alpine, you should target alpine in particular, since every musl setup has their own quirks. I know that winit works just fine on alpine though.

I'm not sure how often x32 is even used, I think only gentoo has it as an option and that's about it.

FreeBSD and other BSDs should be tested on CI, not their target, because it's not really representative IIRC.

riscv has nothing special when it comes to testing.

s390x is a mainframe IIRC and I'm not sure that winit should target those, since they likely don't have graphics in the common sense. Especially given that you exclude Solaris.

I'd just suggest to add arm64 on CI and if you really want to have a separate CI (maybe powered by sr.ht) with FreeBSD, alpine. I can register the runner if you really want to.

kchibisov commented 3 months ago

The point is that they add nothing valuable other than build CI time, only arm64 is sufficient to test 99% of the edge cases, because of different char.

daxpedda commented 3 months ago

The point is that they add nothing valuable other than build CI time, only arm64 is sufficient to test 99% of the edge cases, because of different char.

I believe that this is adding "nothing valuable" has already been disputed by:

I'd suggest to add only platforms which are only actually different, as in, you should cover possibilities and not all the cases.

The issue is that platforms may eventually become different. For instance FreeBSD regularly has breaking changes where fundamental types change. So in order to insulate us from unexpected breakage we need to cast a pretty wide net.

While I agree that this is pretty messed up, if this is true, then checking these platforms does indeed provide some value, as long as we don't have a lint in place which would mitigate this issue otherwise (or some other way to prevent it).

I think the true discussion that should be happening here is to compare the added CI time vs the benefit. Though I agree that the benefits are quite low.

@notgull I think we can better discuss this if you would try to parallelize the checks as I noted in my review (or find out that I was wrong about that being better) and compare the runtime to our current CI and tell us how it went.

notgull commented 3 months ago

I think the true discussion that should be happening here is to compare the added CI time vs the benefit. Though I agree that the benefits are quite low.

I'd argue not being blindsided by build failures is a pretty big benefit.

@notgull I think we can better discuss this if you would try to parallelize the checks as I noted in my review (or find out that I was wrong about that being better) and compare the runtime to our current CI and tell us how it went.

There isn't much of a point; checks are pretty fast and all of the jobs complete way before the mainline test jobs anyways.