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 clippy & use musl target on Rust compiler for static compilation #554

Merged
merged 3 commits into from
Oct 14, 2021

Conversation

dnaka91
Copy link
Contributor

@dnaka91 dnaka91 commented Oct 13, 2021

Just some cleanups here and there, mostly related to clippy:

  • Fix all existing clippy lints reports for the project.
  • Additionally enable clippy::pedantic and rust_2018_idioms lints and fix any reported warnings.
  • Update CI to include an extra step for running clippy.

@vercel
Copy link

vercel bot commented Oct 13, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/schniz/fnm/EJtEca9Hf4BmMGwvSNpbcu9FTYFV
✅ Preview: https://fnm-git-fork-dnaka91-clippy-schniz.vercel.app

@dnaka91
Copy link
Contributor Author

dnaka91 commented Oct 14, 2021

Seems like the ekidd/rust-musl-builder docker image hasn't been updated in a while, hence doesn't include the latest Rust compiler 🤔.

Is it okay for you if I switch to an alternative?

Also, I see you use --target x86_64-unknown-linux-gnu when running in that container but it's a container for building x86_64-unknown-linux-musl. Not 100% sure I understand the purpose. You want to have a fully statically compiled binary right? Then compiling for the musl target should be fine.

@Schniz
Copy link
Owner

Schniz commented Oct 14, 2021

You want to have a fully statically compiled binary right?

Yup, you are right 😃 I don't remember why I needed to use a container. Maybe it's just something I did the same as I did on the ReasonML version

@dnaka91
Copy link
Contributor Author

dnaka91 commented Oct 14, 2021

Okay I updated the CI job now to simply use the Rust compiler directly.
Didn't need any Docker images for this and tested that the output artifact works in Linux 🎉

@Schniz Schniz added the PR: Internal An internal work has been made label Oct 14, 2021
@Schniz
Copy link
Owner

Schniz commented Oct 14, 2021

Amazing! thank you!

@Schniz Schniz changed the title Fix all clippy lints and enable more lints Fix clippy & use musl target on Rust compiler for static compilation Oct 14, 2021
@Schniz Schniz merged commit 74c9b37 into Schniz:master Oct 14, 2021
@dnaka91 dnaka91 deleted the clippy branch October 14, 2021 14:00
t-botz added a commit to t-botz/fnm that referenced this pull request Dec 13, 2022
Redoing Schniz#389 which was cancelled by Schniz#554 and reintroduced the error.

Fixes Schniz#520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Internal An internal work has been made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants