Skip to content

Conversation

@marosset
Copy link
Contributor

@marosset marosset commented Jun 27, 2025

Fixes #549

WIP - I plan to move the clippy and fmt check out of dep_rust so we can a) run the in parallel, and b) not need to run them for each arch/hypervisor/target matrix. Right now I just want to test that we've caught all the clippy issues

syntactically
syntactically previously approved these changes Jun 30, 2025
Copy link
Member

@syntactically syntactically left a comment

Choose a reason for hiding this comment

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

Thanks @marosset! This and the follow-up to move it to a different workflow and reduce CI time will be great to have :)

ludfjig
ludfjig previously approved these changes Jun 30, 2025
@marosset marosset dismissed stale reviews from ludfjig and syntactically via aecac0b June 30, 2025 18:51
Copy link
Member

@syntactically syntactically left a comment

Choose a reason for hiding this comment

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

This looks good overall. I have left inline a bunch of opinionated tips for defensive shell coding, which mostly don't matter here due to the fact that cargo features have a fairly limited character set. Feel free to ignore them, but I thought you might find them interesting/useful---unless otherwise noted, the style that I suggest generally deals nicely with pretty much any special characters in names of things by being careful to control quoting and word splitting. The one exception is NULs, which cannot be dealt with in bash unless you go to a great deal of effort to never read anything into a bash string (which is at heart a C string, and can't contain NULs).

@marosset marosset added area/infrastructure Concerns infrastructure rather than core functionality github_actions Pull requests that update GitHub Actions code kind/refactor For PRs that restructure or remove code without adding new functionality. labels Jul 7, 2025
Copy link
Member

@syntactically syntactically left a comment

Choose a reason for hiding this comment

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

Again, feel free to ignore shell style nits

@marosset marosset changed the title WIP - Clippy updates Clippy updates Jul 8, 2025
@marosset
Copy link
Contributor Author

marosset commented Jul 8, 2025

It looks like some more clippy issues got introduced by #696
I'll fix those us as part of this PR, then let's merge this (even with some bash/shell nits :P)

@marosset
Copy link
Contributor Author

marosset commented Jul 8, 2025

@syntactically - Can you review the new updates I pushed (50b08fe) which fixes some clippy issues in hyperlight_guest_bin introduced by a recent PR?
Thanks!

ludfjig
ludfjig previously approved these changes Jul 8, 2025
Copy link
Contributor

@ludfjig ludfjig left a comment

Choose a reason for hiding this comment

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

LGTM, ignoring any potential bash hygiene issues.

Btw, if mshv3 is a required feature, will mshv2 ever be checked? Since mshv3 "overwrites" mshv2

@syntactically
Copy link
Member

Btw, if mshv3 is a required feature, will mshv2 ever be checked? Since mshv3 "overwrites" mshv2

I think we are planning to get rid of mshv2 very shortly.

@marosset marosset force-pushed the clippy-updates branch 2 times, most recently from e48d6f6 to 76eb1a2 Compare July 8, 2025 19:57
Copy link
Member

@syntactically syntactically left a comment

Choose a reason for hiding this comment

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

Thanks for catching these! Sorry, I guess our previous clippy configuration was missing this crate & I didn't think to run it manually. I have a bunch of comments on the safety notes; I can also do a commit to write them myself if you would like.

@marosset
Copy link
Contributor Author

marosset commented Jul 9, 2025

Thanks for catching these! Sorry, I guess our previous clippy configuration was missing this crate & I didn't think to run it manually. I have a bunch of comments on the safety notes; I can also do a commit to write them myself if you would like.

No problem. I think I addressed all the comments. PTAL

syntactically
syntactically previously approved these changes Jul 9, 2025
Copy link
Member

@syntactically syntactically left a comment

Choose a reason for hiding this comment

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

These look much better, thanks! Commented on a few typos I saw but other than that looks good :)

@marosset
Copy link
Contributor Author

marosset commented Jul 9, 2025

These look much better, thanks! Commented on a few typos I saw but other than that looks good :)

/sigh, let me fix those

@marosset marosset force-pushed the clippy-updates branch 2 times, most recently from 6a789fe to 8ef120b Compare July 9, 2025 23:26
ludfjig
ludfjig previously approved these changes Jul 10, 2025
@marosset marosset enabled auto-merge (squash) July 10, 2025 23:38
@marosset marosset disabled auto-merge July 10, 2025 23:40
@marosset marosset enabled auto-merge (squash) July 10, 2025 23:46
@marosset marosset merged commit 8c55d6e into hyperlight-dev:main Jul 11, 2025
36 checks passed
@marosset marosset deleted the clippy-updates branch July 11, 2025 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/infrastructure Concerns infrastructure rather than core functionality github_actions Pull requests that update GitHub Actions code kind/refactor For PRs that restructure or remove code without adding new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review just clippy command

4 participants