Tooling updates - Githubissues

godot-rust / gdnative

Rust bindings for Godot 3

https://godot-rust.github.io

MIT License

3.61k stars 210 forks source link

Tooling updates #1074

Closed Bromeon closed 5 months ago

Bromeon commented 5 months ago

Make CI work again, and modernize things a bit.

Changes:

Address clippy lints (stable + some nightly), including the one suggested by y21 to fix the ICE
Replace obsolete actions-rs toolchain with direct rustup calls
Update actions/checkout, actions/cache
Update cargo-dinghy, cargo-deny, cargo-machete
Bump MSRV from 1.67 to 1.70
Remove serde_yaml from tests (no longer maintained)

Closes #1073.

Bromeon commented 5 months ago

Ugh... an ICE in the futures crate, see CI run.

Unfortunately I don't have the time to debug/minimize that right now 😕

chitoyuu commented 5 months ago

Ugh. I think it's the same ICE that I've ran into earlier here. Haven't had the time to debug it properly yet either. I guess we can also just report the issue immediately. It's more work for upstream but I'd imagine them to be much better at debugging ICEs than either of us, and some report is better than nothing.

Bromeon commented 5 months ago

Agree. Do you have time to report it, or should I?

chitoyuu commented 5 months ago

I've created https://github.com/rust-lang/rust-clippy/issues/12616.

Bromeon commented 5 months ago

That was fixed insanely fast 👀

I guess we have to wait until next stable, unless we change CI config?

chitoyuu commented 5 months ago

Probably. I don't think waiting for a stable release should be much of a problem for this repo, vs changing the CI config now only to have to change it back in a few more weeks. It would be productive to check locally with nightly after the changes get released in one of course, just to make sure that there isn't anything else the MCVE failed to capture.

y21 commented 5 months ago

(coming from the clippy issue since I saw it linked) If this ICE blocks you and you want to "work around" it, you can also just fix the pedantic warning that would have been emitted if clippy didn't die on that fn call, by using .cast::<sys::godot_object>() instead of an as cast here https://github.com/godot-rust/gdnative/blob/7e55836e53bfd1ccd618939ec30ab6bf2400b614/gdnative-core/src/object/mod.rs#L358

fwiw, I did verify that clippy no longer ICEs on this crate with the fix applied

warning: `gdnative-core` (lib) generated 27 warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 13.56s

Bromeon commented 5 months ago

Thanks a lot @y21, also for the very fast clippy fix. It's really appreciated! ❤️

Bromeon commented 5 months ago

@chitoyuu Addressed several things to make CI green again -- see initial message for details.

Nightly clippy has a lot of warnings that would take significant maintenance effort (moving around imports, refactoring ToString, etc). I'm not sure that has any benefit for this library, so I don't plan on spending more time on those. We can maybe selectively disable some, or opt out of nightly altogether, but that can be another discussion 🙂 some less controversial ones are fixed here, too.