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

Clippy Allowing warnings #80

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Clippy Allowing warnings #80

merged 1 commit into from
Sep 10, 2024

Conversation

didierofrivia
Copy link
Member

@didierofrivia didierofrivia commented Sep 6, 2024

@@ -71,4 +71,4 @@ jobs:
- uses: actions-rs/cargo@v1
with:
command: clippy
args: --all-targets --all-features -- -D warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

This is opening the door for all the warnings. Today have none due to this.

Are we sure we want this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The alternative is to pin rustc to 1.80, the one that is marking the allowing of box_pointers is rustc 1.81.0 (eeb90cda1 2024-09-04)

Copy link
Contributor

Choose a reason for hiding this comment

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

What about updating src/envoy path content? I am aware it is auto-generated from protobuf descriptors. So maybe change the source of truth or add some customization https://github.com/Kuadrant/wasm-shim/blob/main/build.rs#L55-L106

It is all about deleting

#![allow(box_pointers)]

That is a lint being removed in rust 1.80

@alexsnaps
Copy link
Member

Whichever route you all decide to take, I'd like for it to remain a temporary fix until this PR is released into a newer version of the crate... thanks 🙏

@eguzki
Copy link
Contributor

eguzki commented Sep 9, 2024

Whichever route you all decide to take, I'd like for it to remain a temporary fix until this PR is released into a newer version of the crate... thanks 🙏

Good to see there is a fix. Indeed, we need just a temporary fix (and a reminder to undo when the upstream fix is available)

Currently:

* 0beff241 - (3 weeks ago) Update CHANGELOG.md - Stepan Koltsov (HEAD -> master, origin/master, origin/HEAD)
* 60c7e3c3 - (3 weeks ago) Remove protoc version check because they changed versioning - Stepan Koltsov
* 0dcd7e3c - (5 weeks ago) Remove use of box_pointers lint - Rob Ede
| * 1e1e80ad - (3 weeks ago) Bump version to 3.5.1 - Stepan Koltsov (tag: v3.5.1, origin/v3.5)
| * 2b893299 - (5 weeks ago) Remove use of box_pointers lint - Rob Ede
| * 22655e35 - (3 months ago) Bump version to 3.5.0 - Stepan Koltsov (tag: v3.5.0)
| * 7843e95d - (3 months ago) Regnerate protos - Stepan Koltsov
| * 515bfe02 - (3 months ago) Bump version to 3.5.0-pre - Stepan Koltsov
|/  
* 0e6f3ff5 - (3 months ago) Disable miri test which seems to run too slowly - Stepan Koltsov

The fix we want is 0dcd7e3c - (5 weeks ago) Remove use of box_pointers lint - Rob Ede

@didierofrivia didierofrivia force-pushed the ci-clippy-allow-warnings branch from a4607dd to f1df905 Compare September 10, 2024 08:32
Signed-off-by: dd di cesare <[email protected]>
@didierofrivia
Copy link
Member Author

@eguzki Added a TODO comment pointing out the PR that needs to be released

@didierofrivia didierofrivia merged commit ff7e501 into main Sep 10, 2024
8 checks passed
@didierofrivia didierofrivia deleted the ci-clippy-allow-warnings branch September 10, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants