search

nicoburns / blessed-rs

A community guide to the Rust ecosystem
https://blessed.rs
1.32k stars 77 forks source link

Add `cargo-semver-checks` to linting category #105

Closed obi1kenobi closed 7 months ago

obi1kenobi commented 11 months ago

cargo-semver-checks is used by many large projects in the Rust ecosystem, such as tokio and even cargo itself. Add it as a recommendation in the linting category.

If you feel a different category would be a better fit, I'd be happy to move it there.

Thanks for maintaining this list!

obi1kenobi commented 7 months ago

Thanks!

epage commented 7 months ago

Speaking personally, I think this should be reverted.

I am very excited about cargo-semver-checks and look forward to the potential of what it can do for the ecosystem but there is a fundamental workflow flaw at this time that means I cannot endorse it and I think it should generally not be endorsed.

cargo-semver-checks, and the associate action, encourage people to bump their version in Cargo.toml during development. This breaks a fundamental workflow within Cargo, [patch]. You can only patch a dependency if the version field matches between what you have locked and what you are patching in. For example, if I depend on a project that has adopted the common cargo-semver-checks workflow, I can't patch in the git repo to test if a fix addresses a problem to verify it before it gets released.

nicoburns commented 7 months ago

You can only patch a dependency if the version field matches between what you have locked and what you are patching in.

Hmm... that seems non-ideal. What is the reason for this restriction? Could it be relaxed on the cargo side? (perhaps make it a warning rather than a hard error?). Because even aside from cargo-semver-checks this seems like a very common workflow. And if you're patching something then you should be on the lookout for breakage anyway.

obi1kenobi commented 7 months ago

encourage people to bump their version in Cargo.toml during development

@epage, I don't think this is a fair or accurate representation of the facts.

We explicitly encourage people to use the action as part of a cargo publish workflow only at this time. I've been saying so at every opportunity: in talks, in docs, and in blog posts. You've opened two issues on the action's repo mentioning that the action isn't yet suitable for running on PRs, and they both remain open since we haven't fixed them yet.

Of course some users have come up with workarounds that let them run the action on PRs too. Some of them even just straight-up run the action on PRs and make it a non-required step. But I don't think it's fair or accurate to say that we encourage this when I've done everything I possibly can to discourage it.

Even for local workflows, this is still not an accurate representation. I don't see a reason why a maintainer would have to bump their local Cargo.toml version number — it doesn't seem to offer anything that some combination of the --release-type, --baseline-rev, and/or --baseline-version flags won't also do in a less clunky way.

djc commented 7 months ago

IMO there are still other good reasons to bump versions during development and the imposition of that workflow on patch at least in my experience has not been a big deal, so I don't think it's a good reason to drop cargo-semver-checks from this page.