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

Fix CI #712

Merged
merged 3 commits into from
Aug 13, 2024
Merged

Fix CI #712

merged 3 commits into from
Aug 13, 2024

Conversation

jeffcharles
Copy link
Collaborator

@jeffcharles jeffcharles commented Aug 2, 2024

Description of the change

Removing deprecated crates from the workspace, the fmt checks for deprecated crates from the makefile, and a few updates to appease the new version of Clippy:

  • changing &mut store to store.as_mut_context()
  • adjust indentation in one set of docs
  • mark two imports that are only used if a particular feature is enabled are only imported when that feature is enabled

Why am I making this change?

CI always uses the latest stable version of Clippy and that version of Clippy introduced new checks which our code fails. I adjusted the workspace and the Makefile since I don't want to make changes to the deprecated quickjs-wasm-sys or quickjs-wasm-rs crates but still want to be able to run make fmt to see all failing Clippy checks that would show up in CI.

For the &mut store to store.as_mut_context() changes, Clippy seems to sometimes get confused when passing &mut store to a Wasmtime method that takes store: impl AsContextMut and states we should only pass store. Unfortunately the Rust compiler obviously complains when passing store without the &mut if store is referenced again later because Rust will treat it as a move. At first I wrote I just updated call sites where Clippy complained but then it looks inconsistent within a function or file where sometimes we're passing store.as_context_mut() and sometimes we're passing &mut store and it's not obvious why we're using the first in some places and the second in others. So, I decided to make passing stores consistent since the behaviour is identical anyway.

Checklist

  • I've updated the relevant CHANGELOG files if necessary. Changes to javy-cli and javy-core do not require updating CHANGELOG files.
  • I've updated the relevant crate versions if necessary. Versioning policy for library crates
  • I've updated documentation including crate documentation if necessary.

@jeffcharles jeffcharles marked this pull request as ready for review August 2, 2024 15:34
@jeffcharles jeffcharles requested a review from surma August 2, 2024 15:34
Copy link
Member

@saulecabrera saulecabrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me. I'm concerned with the clippy/rustc dance, though. I'm wondering, as I've mentioned informally in the past, if we should consider removing clippy altogether or at the very least reduce the lints, similar to https://github.com/bytecodealliance/wasmtime/blob/main/Cargo.toml#L174, to avoid this from happening in the future. Personally I don't find it useful and can't remember an instance in which it was useful to the project.

@jeffcharles jeffcharles merged commit bf8bb6e into main Aug 13, 2024
16 checks passed
@jeffcharles jeffcharles deleted the jc.fix-ci branch August 13, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants