gixref 0.41 fails to compile in CI

[alice-i-cecile]

the method context exists for fn item fn(&mut &[u8]) -> Result<SignatureRef<'_>, ErrMode<_>> {decode::<'_, _>}, but its trait bounds were not satisfied

Spotted in https://github.com/bevyengine/bevy/actions/runs/8041691846/job/21961284606#step:4:409. This sort of spontaneous failure is generally caused by a semver violation somewhere.

[wasabrot]

Also cargo install cargo-binstall is failing due to this, I suppose many projects are affected

[Byron]

This should be fixed now. I investigated the update issue of gix-ref and why it wouldn't pull in the latest version of it, and it turned out that it was all based on other patch-releases needing gix-actor in the latest version, so the resolver did the right thing and found a lower version of gix-ref which was compatible with the latest version of gix-actor, at least on paper.

Since I wasn't really able to produce a gix-ref which can work with both versions of gix-actor as it would then have to mix winnow 0.5 and 0.6 which doesn't work, I decided to yank gix v0.59 entirely as it's the least used.

The culprit here is that an upgrade to winnow 0.6 happened without me realizing that this is a breaking change as it's not perfectly isolated. gix-actor for instance leaks an error type (for good reason), but that's also causing the issue.

In future, and it's something I usually do, I have to treat breaking winnow upgrades as breaking for the API, and even though it's something I kind-of know, this time it slipped by in the reviews. As the upgrade was contributed, it was less obvious to me I suppose, but I have to be more vigilant.

My greatest apologies for the disruption this has caused :/.

[wasabrot]

Thx a lot @Byron such a fast response is really great; right now I m testing with the script which is already working; later I will re-test with what caused the break and give short feedback here of the outcomes.

[alice-i-cecile]

Thanks a ton for the fast reply: this wasn't too disruptive for us in the end. Just glad to be able to help catch this stuff :)

Might be worth looking into running cargo-semver in CI: these sort of issues are really tricky to spot manually.

[leighmcculloch]

I investigated the update issue of gix-ref and why it wouldn't pull in the latest version of it, and it turned out that it was all based on other patch-releases needing gix-actor in the latest version, so the resolver did the right thing and found a lower version of gix-ref which was compatible with the latest version of gix-actor, at least on paper.

Thanks @Byron for shipping a fix so quickly.

For projects that hadn't updated gix or winnow, and didn't change their dependencies, do you know how the release of gix v0.59 caused builds that had Cargo.lock files present and dependent on gix v0.58 / gix-actor v0.30.0 / gix-object v0.41.0 to pickup the new versions gix v0.59 / gix-actor v0.30.1 / gix-object v0.41.1?

[NobodyXu]

Because cargo-install does not use the lockfile by default, unless you pass --locked to it

[Byron]

Might be worth looking into running cargo-semver in CI: these sort of issues are really tricky to spot manually.

I agree. Previously I was repelled by the anticipated CI cost of this, how much would it be able to do within a 30min window at most? But that was admittedly based mostly on feeling, not at all on data. I also know it can't detect everything yet, and in order to catch this, you'd really have to compile the current version of gix-actor with the most recent released version of gix-ref. See, I still don't think anything could really catch this in time, but what should work is a CI job that installs the previous version of gix which should then have failed just as early.

[alice-i-cecile]

@obi1kenobi, the tool author, will have better answers for you here :)

[obi1kenobi]

Thanks for the ping @alice-i-cecile, I appreciate it ❤️

I also know it can't detect everything yet

I don't think c-s-c would have caught this right now. We don't do any checking of trait bounds right now, and likely won't for at least another 9-12 months.

Previously I was repelled by the anticipated CI cost of this, how much would it be able to do within a 30min window at most?

On my laptop, time cargo semver-checks --default-features --workspace using cached baseline rustdoc JSON (the steady-state behavior of using the GitHub Action) and a from-scratch build of the current rustdoc JSON takes:

real    4m27.433s
user    18m41.499s
sys     1m56.532s

A best estimate of c-s-c runtime is "one cargo build" time, because we've optimized everything else quite heavily: baseline rustdoc JSON is cached, and running the lints takes milliseconds on most crates. Even on the largest crate on crates.io (~240k items), the lint-running portion itself takes only 2s whereas it takes 30s to build the current rustdoc JSON.

I suspect we can drag these numbers down further by improving our cargo cache hit rate while building rustdoc. But this would require finding some more funding.

IMHO a stronger argument against using cargo-semver-checks here at the moment is how we handle (or fail to handle) complex edge cases in workspaces, like the addition of a new crate that hasn't been published yet (which currently would cause a "failed to find a baseline version to compare against" error and exit 1). More details on these edge cases + a concrete proposal for addressing them can be found here.

In future, and it's something I usually do, I have to treat breaking winnow upgrades as breaking for the API, and even though it's something I kind-of know, this time it slipped by in the reviews. As the upgrade was contributed, it was less obvious to me I suppose, but I have to be more vigilant.

I've been mulling over writing an extension to cargo-semver-checks that could allow users to define custom lints for situations like that, to tackle cases of "I know this is breaking due to some circumstances unique to this project." It could be used to write a rule like "consider winnow upgrades to be semver-major" which may have helped here.

Is that something you'd find useful and might be interested in trying?

I haven't started building such a tool yet and I don't have a concrete timeline — but if there's interest in the community and if I can find some funding, I'd love to give it a shot.

[Byron]

Thanks for chiming in, it's much appreciated, and apologies for the late response. It took me a while to organise my thoughts about this.

IMHO a stronger argument against using cargo-semver-checks here at the moment is how we handle (or fail to handle) complex edge cases in workspaces, like the addition of a new crate that hasn't been published yet (which currently would cause a "failed to find a baseline version to compare against" error and exit 1). More details on these edge cases + a concrete proposal for addressing them can be found here.

Without fully understanding what's written there, let me just share what I think c-s-c would have to do to be truly sure that a change isn't breaking after it was published. First, I think it would have to understand the version that the crate would be published at which is project specific and based on 'git-conventional' commit messages. And then it would actually have to check all changed crates (in a PR, for example) against the currently published versions of all remaining crates, possibly with a controllable variety of feature toggles. In case of gitoxide, it would probably be best to run part of the normal CI.

When reading the above, it truly feels like I am describing a different tool, and one that is either very specific to gitoxide or extremely customisable. Much of the information that would be required to pull this off is already known to cargo smart-release, and in a way, what it really wants to do is to automatically determine under which version a crate should be published. Right now, it uses the crutch of relying on me to always use the correct 'git-conventional' message when I know it's breaking. But any time I fail, I break an increasingly large set of downstream projects which is painful for everyone.

I've been mulling over writing an extension to cargo-semver-checks that could allow users to define custom lints for situations like that, to tackle cases of "I know this is breaking due to some circumstances unique to this project." It could be used to write a rule like "consider winnow upgrades to be semver-major" which may have helped here.

Is that something you'd find useful and might be interested in trying?

I think I'd want a tool that leaves no room for error, and in principle I believe it can be done but only if rustc is used. Writing custom lints for after something broke (to prevent the same or similar issues from occurring again) could be a way to go, but I keep thinking that it should be possible to actually try the 'real' thing, and gain much greater confidence that way while being less reliance on human input.

Like I mentioned earlier, ideally cargo smart-release would not need 'git-conventional' at all, and I intuitively (and naively) think it can be done.

And there is probably a ton of details I am missing which would make such a tool even harder to realise, but… if you think there is a chance, please let me know :). A true solution to this problem is definitely something I'd want to sponsor.

[obi1kenobi]

Quick thoughts:

• The extremely customizable tool you describe (at least as I understand it) sounds very doable.
• Using rustc itself is absolutely doable too (we have a working proof-of-concept!), and is merely a question of having ongoing funding to keep it supported as rustc's internals change often.
• 100% certainty is impossible because of the Halting Problem (new bugs are arguably breaking changes), but a good test suite + custom lints system we used at my last job (10min talk I gave on it) felt like it got us to damn near 100%.

The bottom line is that with a steady source of funding, I think we have a very solid shot at all this — in a way where the entire Rust ecosystem could win, not just gitoxide and projects that use it.

I'd love your thoughts! Might you be up for a chat sometime in the next couple of weeks?

[Byron]

Yes, and I sent you an email hoping we can schedule that call this week even :D.