kivikakk / comrak

CommonMark + GFM compatible Markdown parser and renderer
Other
1.12k stars 133 forks source link

workflows: check MSRV in CI. #406

Closed kivikakk closed 1 month ago

kivikakk commented 1 month ago
github-actions[bot] commented 1 month ago
Command Mean [ms] Min [ms] Max [ms] Relative
./bench.sh ./comrak-1e8fa42 318.5 ± 2.2 314.8 323.9 2.85 ± 0.04
./bench.sh ./comrak-main 318.4 ± 1.5 316.1 323.0 2.85 ± 0.03
./bench.sh ./pulldown-cmark 111.7 ± 1.2 110.2 115.0 1.00
./bench.sh ./cmark-gfm 119.6 ± 4.1 116.7 135.0 1.07 ± 0.04
./bench.sh ./markdown-it 480.6 ± 2.0 475.7 484.3 4.30 ± 0.05

Run on Thu May 9 15:27:35 UTC 2024

kivikakk commented 1 month ago

Hooboy. This is frustrating enough that I want to pull the CLI binary out as its own crate — which makes sense, given I want different MSRV disciplines between them.

digitalmoksha commented 1 month ago

frustrating enough that I want to pull the CLI binary out as its own crate

I understand - I'm at 1.73 and while starting a new branch I had to up level because of the CLI.

However it would be a bit of a bummer, as it's nice to be able to debug directly using the RustRover IDE - it runs the CLI and I can just step through code.

Also, I think your compliance specs are run using the CLI.

Can we not pin the CLI crates to the version that we're supporting? Is some critical functionality/bug fixes getting lost?

kivikakk commented 1 month ago

Also, I think your compliance specs are run using the CLI.

Yep, my current WIP builds the library on nightly/beta/stable+MSRV and runs unit tests with it, then builds the binary on nighty/beta/stable and runs specs with it.

Can we not pin the CLI crates to the version that we're supporting? Is some critical functionality/bug fixes getting lost?

There's just a bunch of crates that are starting to get rather old — they left 1.62 behind a while ago — and so as a matter of course I don't really love leaving them back. This mostly applies to the CLI ones. The recently added in-place is hard to support too, though I can just as easily tear that out again, it's minor.

kivikakk commented 1 month ago

I think it's easier just to set a standard MSRV across both, so yeah, I'm going to do the work to get us back to 1.62.1 👍

digitalmoksha commented 1 month ago

If there are say security or performance fixes in a crate, when a user builds the library they should be able to specify a more recent version of that crate, right? My Rust dependency knowledge is still young.

kivikakk commented 1 month ago

when a user builds the library they should be able to specify a more recent version of that crate, right? My Rust dependency knowledge is still young.

Mine too, lol 😅

I'm not quite sure — probably? If it gets to that point, I'm sure someone will let us know!

kivikakk commented 1 month ago

lol okay. I was thinking cargo update would know not to update to versions out of its MSRV. It doesn't, yet, but it wouldn't help us anyway until we bump our MSRV to one with that Cargo anyway, I guess. Yeesh.

kivikakk commented 1 month ago

I don't think there's any non-insane way to make cargo update work while respecting MSRV for us. Even if I pin our top-level packages (some e.g.s: regex at = 1.9.6, clap at = 4.0.32), their dependencies still happily update to things beyond our MSRV (e.g. clap 4.0.32 depends on clap_lex@^0.3.0; clap_lex 0.3.3 requires Rust 1.64.0).

Dependency updates that Dependabot opens and CI passes on (as of this PR) are fine, and we can respond to e.g. cargo audit or requests if something has to change, but unless there's any other automated means out there ... ¯\_(ツ)_/¯ I've typed cargo update -p blah@x.y.z --precise a.b.c too many times already today.

kivikakk commented 1 month ago

Happy to take feedback from anyone interested; will merge in a day or two otherwise!

digitalmoksha commented 1 month ago

This looks great @kivikakk. I tested a build on 1.73 and it worked like a charm - it didn't require me to bump the rust version.

kivikakk commented 1 month ago

Awesome, really glad! 😊 Thanks for testing it!

digitalmoksha commented 1 month ago

@kivikakk so propfuzz.rs is no longer needed to do fuzzing?

kivikakk commented 1 month ago

That used https://github.com/facebookarchive/propfuzz, which (as the URL implies!) isn't actively worked on these days and never made it past an initial test release. (IIRC propfuzz predated cargo-fuzz, but yeah, we're in a much better position with the latter now.)

gjtorikian commented 1 month ago

Bleh, looks like the latest dependabot updates didn't merged because the requires MSRV tests failed. Maybe we should just chuck that automation—the longest time interval for the check is a month, which isn't going to really change anything in terms of support for MSRV (I would assume that would be on the order of months, if not years).

digitalmoksha commented 1 month ago

Ack, I think you might be right. I don't suppose dependabot can be made to have awareness of what our MSRV is and only recommend those updates?

What about just commenting it out for now, instead of ripping it out? Or do you think it's cleaner just to pull it.

kivikakk commented 1 month ago

What about just commenting it out for now, instead of ripping it out? Or do you think it's cleaner just to pull it.

Always delete! It'll be there in revision history when we need it again.

the longest time interval for the check is a month, which isn't going to really change anything in terms of support for MSRV (I would assume that would be on the order of months, if not years).

I'm inclined to agree. :/ Will remove for now. Thanks for all your work on that! I feel like we'll be restoring it in no time, the way the days seem to pass lately.

gjtorikian commented 1 month ago

I don't suppose dependabot can be made to have awareness of what our MSRV is and only recommend those updates?

Afraid not. The same issue @kivikakk linked to in rust-core is what's holding up dependabot 🙃 https://github.com/dependabot/dependabot-core/issues/5423