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 non-linux compilation warnings. #248

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tomusdrw
Copy link

@tomusdrw tomusdrw commented Jan 7, 2025

I've started adding #[cfg(target_os = "linux")] but for some cases it starts a cascade of related code not being used, so I think it's safer to go with conditionally allowing dead code.

Let me know what's your preference.

@koute
Copy link
Collaborator

koute commented Jan 8, 2025

Hmmm... well, I'm not sure whether gating those to Linux is the way we want to go, due to a few reasons:

  • it's not strictly correct (e.g. on Aarch64 Linux systems you'll also get extra warnings)
  • we will want to support the recompiler on other OSes in the future too (And in fact the recompiler used to work on macOS too in the past, before I had to refactor a ton of code and didn't have the time to get the generic sandbox updated too.)
  • and it seems kinda painful to maintain

So I think what we could do:

  • in cases where the code in question is, not sure how to call it, "general purpose", we could unconditionally allow dead_code (for example, Mutex::lock should never give out a warning even when unused, because the whole point of a mutex is locking, so it doesn't make sense for it to not have this method)
  • for compiler-specific code wrap it in if_compiler_is_supported! { (which is the proper way to gate code); unfortunately this is sometimes somewhat awkward to do (could be made easier with a procedural macro, but I refuse to bloat up the compile times for everyone just so that it's a little bit more convenient for me)
  • Run clippy on the CI twice, but use a target which doesn't have a recompiler the second time to make sure the warnings don't come back

@tomusdrw
Copy link
Author

tomusdrw commented Jan 10, 2025

Thanks for the suggestions! I've left a few allow(dead_code) where I felt it's general purpose. The rest was converted to use if_compiler_is_supported.
I've also added a clippy run on macos to prevent new warnings.

Comment on lines 45 to 46
- name: Install clippy (zygote toolchain)
run: cd crates/polkavm-zygote && rustup component add clippy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Zygote is (and will always be) Linux-only so for macOS it should be removed. (I guess you'll need to modify the clippy.sh script to only run clippy on zygote on Linux; you can copy-paste the detection from run-all-tests.sh)

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