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.

Sign up for GitHub

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

Jump to bottom

CI: Add rustfmt and cargo clippy to CI #95

Merged
merged 17 commits into from
Apr 26, 2023
Merged

CI: Add rustfmt and cargo clippy to CI #95

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
51526b2
Minor cargo clippy fixes.
ipuustin Mar 20, 2023
cc3c6d5
CI: Add rustfmt and cargo clippy to CI
Mossaka Mar 21, 2023
bc8c7a0
run rustfmt
Mossaka Mar 21, 2023
b88a676
Merge remote-tracking branch 'upstream/main' into add-fmt-to-ci
Mossaka Mar 21, 2023
556ba00
remove all-features flag from clippy
Mossaka Mar 21, 2023
669b463
Fix Clippy-detected issues.
ipuustin Mar 22, 2023
6d5f067
Merge pull request #2 from ipuustin/add-fmt-to-ci
Mossaka Mar 28, 2023
000e221
Apply rustfmt
Mossaka Mar 28, 2023
a980341
Last clippy fixes.
ipuustin Mar 29, 2023
37498c9
Merge pull request #3 from ipuustin/add-fmt-to-ci
Mossaka Apr 24, 2023
f432761
Merge branch 'main' into add-fmt-to-ci
Mossaka Apr 26, 2023
a445acb
Resolve clippy issues
Mossaka Apr 26, 2023
a58a8a1
pin rust version to 1.69.0 in CI
Mossaka Apr 26, 2023
9dd0a2a
run rustfmt
Mossaka Apr 26, 2023
3b437d0
added setup OCI runtime build env in fmt job
Mossaka Apr 26, 2023
4476a1e
remove unneeded return statement to make clippy happy
Mossaka Apr 26, 2023
256fbc0
Applied clippy suggestions
Mossaka Apr 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,24 @@ env:
CARGO_TERM_COLOR: always

jobs:
fmt:
runs-on: "ubuntu-latest"
steps:
- uses: Swatinem/rust-cache@v2
- uses: actions/checkout@v3
- uses: actions-rs/toolchain@v1
with:
toolchain: stable
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be a rolling version. It means PR might pass a check when merging but main branch might fail later and need to be updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you explain why will the main branch might fail later if the CI passes?

Copy link
Contributor

Choose a reason for hiding this comment

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

if this runs on 1.68 and passes, later 1.69 comes out with new clippy rules. Then locally update to 1.69, pull main, run clippy and it will fail.

This can be annoying for contributors as they might have a PR open and rebased on the main branch but now this starts failing when the new tool chain comes out. Either they need to submit a sperate PR and fix then rebase or fix it in same PR to get it in.

If you do release branches this can be a similar issue, where before the fix goes into the branch the formats need to be applied for the new version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I see. What do you recommend to mitigate this issue? I think what we can do is that whenever a new release of clippy comes out (together with a new cargo release), we manually submit a PR to fix the main branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should be pinning the version of Rust that we build against. This creates a little bit more work from a maintenance perspective but it allows more control, sets up for repeatable builds and avoids the pitfalls I mentioned above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I've pinned the rust version to 1.69.0

- name: Setup WasmEdge build env
run: |
cpuguy83 marked this conversation as resolved.
Show resolved Hide resolved
curl -sSf https://raw.githubusercontent.com/WasmEdge/WasmEdge/master/utils/install.sh | bash -s -- --version=0.11.2
echo "LD_LIBRARY_PATH=$HOME/.wasmedge/lib" >> $GITHUB_ENV
- name: Run cargo fmt
run: |
cargo fmt --all -- --check
- name: Run cargo clippy
run: |
cargo clippy --all --all-targets -- -D warnings
build:
strategy:
matrix:
Expand Down
2 changes: 1 addition & 1 deletion crates/containerd-shim-wasmedge/src/bin/containerd-wasmedged/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use std::sync::Arc;
use containerd_shim_wasm::sandbox::EngineGetter;
use containerd_shim_wasm::sandbox::{Local, ManagerService};
use containerd_shim_wasm::services::sandbox_ttrpc::{create_manager, Manager};
use log::info;
use containerd_shim_wasmedge::instance::Wasi as WasiInstance;
use log::info;
use ttrpc::{self, Server};

fn main() {
Expand Down