Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Cached" registry rustdoc JSON version mismatch #415

Closed
thomaseizinger opened this issue Mar 13, 2023 · 8 comments · Fixed by #416
Closed

"Cached" registry rustdoc JSON version mismatch #415

thomaseizinger opened this issue Mar 13, 2023 · 8 comments · Fixed by #416
Labels
C-bug Category: doesn't meet expectations

Comments

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Mar 13, 2023

Steps to reproduce the bug with the above code

Actual Behaviour

cargo semver-checks fails to read the old JSON.

Expected Behaviour

I can think of multiple approaches:

  • Detects that the cached version is stale and re-generates it using the new toolchain
  • Incorporate the rustdoc version into the filename (and cache key), effectively invalidating the cache upon upgrades to the Rust toolchain
  • Tell people to pin the Rust version they are using in CI
  • Avoid the error in the first place: Do the rustdoc JSON versions have to line up? Can't we just use the existing adapters to still run the checks?

Generated System Information

Error appeared in CI so unavailable unfortunately. Version 0.18.3, ubuntu-latest GitHub runner.

Build Configuration

No response

Additional Context

No response

@rinde
Copy link

rinde commented Mar 21, 2023

Is there a workaround for this?

@thomaseizinger
Copy link
Contributor Author

Yes, deleting the cache fixes the problem.

obi1kenobi added a commit that referenced this issue Mar 22, 2023
@obi1kenobi
Copy link
Owner

obi1kenobi commented Mar 22, 2023

Apologies for the delay, I was travelling and only just got home. I've started working on a fix in #416.

The recommended workaround in the meantime is clearing the target/cargo-semver-checks/cache directory manually, or running cargo clean (at the expense of a project rebuild).

Do the rustdoc JSON versions have to line up? Can't we just use the existing adapters to still run the checks?

Rustdoc JSON is quite complex and ships bugfixes fairly frequently, so it's better from a correctness and maintenance perspective if the versions match up — it significantly narrows the state space for incoming bug reports on cargo-semver-checks, which not-infrequently produce bug reports upstream. From a purely technical standpoint, though, the versions don't have to match up.

Detects that the cached version is stale and re-generates it using the new toolchain

This is the approach I'm going for. There's no use case I can see where someone would want to semver-check on more than one Rust version — it's always best to check on the latest stable Rust version due to the likelihood of relevant rustdoc JSON bugfixes, so I don't think keeping multiple cached rustdoc JSON versions simultaneously is useful. Let me know if you feel I've missed something here.

obi1kenobi added a commit that referenced this issue Mar 22, 2023
* CI repro for #415.

* Wipe the cached baseline file on rustdoc version mismatch.

* Delint.

* Add defensive check to ensure the test actually tests the new behavior.

* Fix paths.

* Invert condition.
@thomaseizinger
Copy link
Contributor Author

Detects that the cached version is stale and re-generates it using the new toolchain

This is the approach I'm going for. There's no use case I can see where someone would want to semver-check on more than one Rust version — it's always best to check on the latest stable Rust version due to the likelihood of relevant rustdoc JSON bugfixes, so I don't think keeping multiple cached rustdoc JSON versions simultaneously is useful. Let me know if you feel I've missed something here.

The issue with CI caches here is going to be that a modified cache directory won't get saved so we will re-generate the new JSON over and over again until we manually clear it.

We would have to incorporate the version into the cache key for it to automatically invalidate but that is rather tricky to do I think.

@obi1kenobi
Copy link
Owner

Fixed in #416, will be released shortly.

The issue with CI caches here is going to be that a modified cache directory won't get saved so we will re-generate the new JSON over and over again until we manually clear it.

Would a trick similar to the one used in this repo's CI work?
https://github.com/obi1kenobi/cargo-semver-checks/blob/main/.github/workflows/ci.yml#L163-L172

Essentially, version the cache based on the Rust version. Even if the format version didn't change, it's good to regenerate the rustdocs for new Rust versions in case there are bugfixes in rustdoc that didn't cause a format change — and that cache miss would only happen once every ~6 weeks.

@thomaseizinger
Copy link
Contributor Author

The issue with CI caches here is going to be that a modified cache directory won't get saved so we will re-generate the new JSON over and over again until we manually clear it.

Would a trick similar to the one used in this repo's CI work? main/.github/workflows/ci.yml#L163-L172

Essentially, version the cache based on the Rust version. Even if the format version didn't change, it's good to regenerate the rustdocs for new Rust versions in case there are bugfixes in rustdoc that didn't cause a format change — and that cache miss would only happen once every ~6 weeks.

Yep that should work!

@thomaseizinger
Copy link
Contributor Author

I ended up incorporating the tool-version too: libp2p/rust-libp2p#3664

@obi1kenobi
Copy link
Owner

mergify bot pushed a commit to libp2p/rust-libp2p that referenced this issue Mar 24, 2023
With an upgrade to the Rust toolchain, the version of the rustdoc JSON may change. We incorporate the rustc version into the cache key to automatically invalidate the cache in that case. Additionally, we also incorporate the version of the `cargo semver-checks` tool.

Related: obi1kenobi/cargo-semver-checks#415.

Pull-Request: #3664.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: doesn't meet expectations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants