Bump versions to 0.8.3 (and fix some clippy complaints)

[thomcc]

I'll cut the releases after this lands. @dbr is there any reason to think this might have the same issues as #631?

[dbr]

Maybe stating the obvious, but there's a bunch of breaking changes in main since the 0.8 release, so the 0.8.3 release will need to come from a branch rather than merging this!

I've slightly lost track of everything, but I did have a branch set up for this which I think is still good to use:
#629 (specifically the last three commits of https://github.com/imgui-rs/imgui-rs/commits/patch-v0.8

The changes should fix both the docs.rs build, and using freetype via crates.io (also backports #624 which updated the internally used parking_lot)

[MarijnS95]

@dbr What happened to the authorship on 5207b29, btw?

[MarijnS95]

(and fix some clippy complaints)

@thomcc For a big project like imgui-rs, any reason to not check this in the CI for every push/contribution?

[dbr]

@dbr What happened to the authorship on 5207b29, btw?

Hmm, not sure, sorry about that! I think I may just manually copied the change over to the branch rather than properly cherry-picking the particular commit (which preserves the authored-by metadata)

any reason to not check this in the CI for every push/contribution?

We do run lots of checks on each push/PR, but there's a few complications which means they sometimes miss issues:

1. The clippy fixes are likely from lints added in newer releases of Rust. We pin the version of clippy used in CI to the MSRV (this is to avoid random new clippy lints causing unrelated failures, like if someone makes a PR with a tweak to some documentation, and the build fails because a newly added lint)
2. The docs.rs doc-builds happen in their own particular environment, so don't necessarily work exactly same as our CI, and relatedly..
3. Installing from a .crate file can be slightly different to building a git repo - mainly if the Cargo.toml specifies files to include/exclude (this caused a build to fail only when using the imgui .crate with the freetype feature via crates.io)

😓

Of those I think that:

1. the pinned clippy thing is better than any alternative I'm aware of
2. having some kind of "docs.rs CI" would be nice but unlikely to happen (would drastically increase the requirements on doc.rs infrastructure). We already do a test cargo doc build in CI, it just doesn't catch all possible cases
3. we should probably look into having at least some of the CI checks run on via a .crate file instead of always using the git checkout - but, just not a high priority

[MarijnS95]

Hmm, not sure, sorry about that! I think I may just manually copied the change over to the branch rather than properly cherry-picking the particular commit (which preserves the authored-by metadata)

That's a lot of work, more so than fetching from the remote (it is already merged) or even running curl https://github.com/imgui-rs/imgui-rs/commit/b5376c3fb4e40a45d537a5d4db39e0fa79df84a7.patch | git am :)

We pin the version of clippy used in CI to the MSRV

That explains it. I didn't remember these clippy lints being anything recent but the pinned 1.54 is quite old by now :)

(this is to avoid random new clippy lints causing unrelated failures, like if someone makes a PR with a tweak to some documentation, and the build fails because a newly added lint)

That's fair, depending on the project I usually allow the contributor to fix it or - to keep every commit its own isolated change - open a separate PR to address new lints as maintainer. It can be a little hassle at times but it's a way of staying up-to-date with lints. You can also schedule a cron job in the CI which imgui already does (and GitHub recently told me it would get disabled on my imgui fork...).

The docs.rs doc-builds happen in their own particular environment, so don't necessarily work exactly same as our CI, and relatedly..

Fair enough, docs are built on nightly so if you're cross-testing that you'll end up with a lot of noise in your environment. Nightly changes a lot to be really useful for clippy lint checks though, so you don't want that (as required step) in your CI.

Installing from a .crate file can be slightly different to building a git repo - mainly if the Cargo.toml specifies files to include/exclude (this caused a build to fail only when using the imgui .crate with the freetype feature via crates.io)

Always a good idea to test this, it happened on a few occasions (to me) that a crate built fine but missed some source files, readme, licenses when packaged and uploaded to crates.io 😬

1. the pinned clippy thing is better than any alternative I'm aware of

Perhaps bump it more often though? Unless clippy starts recommending things that MSRV-incompatible.

2. having some kind of "docs.rs CI" would be nice but unlikely to happen (would drastically increase the requirements on doc.rs infrastructure). We already do a test cargo doc build in CI, it just doesn't catch all possible cases

For one you're setting RUSTDOCFLAGS: -D warnings under the clippy job but cargo doc is only ran under the test job, that's not helping... Can't help/comment on the environment issue though.

3. we should probably look into having at least some of the CI checks run on via a .crate file instead of always using the git checkout - but, just not a high priority

Using an artifact to let people download it too, should be pretty trivial to add? With the artifact you can make sure the crate is tested on a completely clean image.

[thomcc]

Maybe stating the obvious, but there's a bunch of breaking changes in main since the 0.8 release, so the 0.8.3 release will need to come from a branch rather than merging this!

Thanks. I didn't realize that.

[thomcc]

(nevermind, already discussed)

[thomcc]

Regarding pinned clippy, I'm opposed to it, and didn't realize we were doing it. It seems kind of to defeat the point. We could also just stop using clippy and use cargo check instead -- this is what I do in most new projects, because clippy lints are... poor signal to noise a lot of the time. Builtin (non-clippy) warnings have a much higher bar, generally requiring no false positives and (at least for new ones) not being particularly opinionated.

Regarding running against a .crate file, I'm not sure even how to do run cargo against a .crate without either unpacking it and running rustc manually, or running a custom registry. Both of these sound pretty painful, although perhaps the 2nd has gotten easier. I might be missing something, though -- I seem to have missed a lot in this issue 😅, so seems very possible!

[dbr]

Might be worth splitting this discussion out elsewhere, but.. with the clippy CI thing it seems like there is a few different levels of "progressiveness":

1. Don't bother running it in CI. Just use the compiler warnings instead. Compiler will catch the important things.
2. Run a pinned version. Gives us a set of well-tested lints without much churn
3. Run latest version of clippy all the time.

I guess there's also a forth option of running latest clippy it in CI with the allow-failures option enabled for it

As mentioned, personally I think the pinned version is a good middleground - while clippy is indeed very noisy, I think there a few lints which are reasonably worthwhile.. while equally I don't think they are too vital as to require constnatly updating clippy (especially as there was a time in the last year or so where lots of new lints were added without sufficient ecosystem-wide testing - there has since been work towards reducing this, rust-lang/rust-clippy#6623 rust-lang/rust-clippy#7666 )

I'd also be in favour of just ditching it from CI - although given the pinned clippy doesn't really add any extra work (and we already run builds with the pinned version to ensure we don't break MSRV), I also don't see much benefit in removing it

Regarding running against a .crate file, I'm not sure even how to do run cargo against a .crate without either unpacking it and running rustc manually, or running a custom registry

Ah, I hadn't really thought much into how to actually do this - I kind of assumed you could just specify it like imgui = { path = "../whatever.crate" } or something, but, it seems not 😢

[dbr]

Regarding running against a .crate file, I'm not sure even how to do run cargo against a .crate

Oh, actually, I just read the cargo package docs.. well, actually read it once, did lots of searching and was confused why no one had raised the inability to test it as a problem, then re-read it, before noticing: running cargo package already verifies the the crate can be built:

This performs the following steps:
...
3. Extract the .crate file and build it to verify it can build

..and, importantly, you can specify cargo package --features ... - so we we just do that in CI, and it'd catch the problem we had with accidentally excluding imgui headers 🥳

Edit: Made PR to test this theory, #646

[MarijnS95]

Shouldn't the 0.8.3 patches+bump end up on top of https://github.com/imgui-rs/imgui-rs/tree/v0.8.2-patch, or have there not been any breaking changes to main since the last release? It seems our discussion above overshadowed @dbr's comment:

Maybe stating the obvious, but there's a bunch of breaking changes in main since the 0.8 release, so the 0.8.3 release will need to come from a branch rather than merging this!

Personal recommendation: why not name that branch 0.8-stable, do stable development/backporting work on that branch, and merely tag the actual releases? Then main can proceed pulling in breaking changes.

[MarijnS95]

@thomcc @dbr Where are we at on this? Are there still plans to release 0.8.3 (without the breaking changes from main, see comments above and #629) or is the main focus now on releasing 0.9.0? Or is this frozen until the release process it written in stone via #665?

[thomcc]

Yeah, good question. I'm inclined to just release 0.9.0, since I don't think the approach that has worked for the backports in the past has really... worked well at all.

[MarijnS95]

@thomcc Thanks, that'd work for me. If you do that, please also delete stale branches (i.e. those associated with closed PRs) to prevent confusion: https://github.com/imgui-rs/imgui-rs/branches. The relevant commits are tagged anyway.

[dbr]

I'd agree with skipping any further 0.8.x releases - best to make v0.9 and settle on a good system for future patch releases

[dbr]

Closing this in favour of releasing v0.9