workflows: run clippy on tests #260
Conversation
During the switch to a workspace repository the clippy run on tests was removed, so add it again. Signed-off-by: Carlos López <carlos.lopez@suse.com>
Fuzzing harnesses expect --cfg fuzzing to be set, so run clippy on them only with the configuration enabled. Signed-off-by: Carlos López <carlos.lopez@suse.com>
| args: --workspace --all-features --exclude svsm --target=x86_64-unknown-linux-gnu -- -D warnings | ||
| args: --workspace --all-features --exclude svsm --exclude svsm-fuzz --target=x86_64-unknown-linux-gnu -- -D warnings | ||
|
|
||
| - name: Clippy on svsm-fuzz |
There was a problem hiding this comment.
This really wasn't included in the above clippy call? I think it should be...
Ideally I think we don't run clippy on all the individual crates, but we use --workspace, --exclude and --target to filter crates that don't build on a given target
There was a problem hiding this comment.
It would be great if we could use --all-features and --all-targets but i don't know if that requires more changes in the code base to support that.
There was a problem hiding this comment.
This really wasn't included in the above clippy call? I think it should be...
Ideally I think we don't run clippy on all the individual crates, but we use
--workspace,--excludeand--targetto filter crates that don't build on a given target
Yes it was, but it was not using --cfg fuzzing. This is specific to the svsm-fuzz target, which is why I did not see the point of using --workspace + --exclude
It would be great if we could use
--all-featuresand--all-targetsbut i don't know if that requires more changes in the code base to support that.
Not sure if I understand, but the reason we cannot run a single command is that the different packages in the workspace use different targets, and there is no way currently to set a per-package target with the stable toolchain. If we set it via <package>/.cargo/config.toml it will be ignored by cargo (as of today) if ran from the root directory, for some reason. See rust-lang/cargo#7004 and rust-lang/cargo#9406.
There was a problem hiding this comment.
Ah yes you're right. In other projects I've set clippy to --workspace --all-features which would accomplish what we want here. But, I'm not sure yet if that's exactly what we want to do. We should make sure --all-features works though.
But yes, cargo doesn't handle these multi-target workspaces that well. In other projects, I've used https://github.com/matklad/cargo-xtask to handle that, we may want to look at doing that here as well. I could imagine something like cargo xtask clippy that would encapsulate all the logic, if we chose to go down that path. I think Joerg was going to maybe take a look at that.
There was a problem hiding this comment.
Ah yes you're right. In other projects I've set clippy to
--workspace --all-featureswhich would accomplish what we want here. But, I'm not sure yet if that's exactly what we want to do. We should make sure--all-featuresworks though.
Sadly fuzzing here is not a feature, so it won't be enabled by --all-features, if that's what you're referring to here. I don't think there's a way around this without a separate job run.
But yes, cargo doesn't handle these multi-target workspaces that well. In other projects, I've used https://github.com/matklad/cargo-xtask to handle that, we may want to look at doing that here as well. I could imagine something like
cargo xtask clippythat would encapsulate all the logic, if we chose to go down that path. I think Joerg was going to maybe take a look at that.
That would be interesting for sure. It would clean up a lot of the Makefile and CI commands.
During the switch to a workspace repository the clippy run on tests was removed, so add it again.
While we are at it, run clippy on
svsm-fuzzonly with--cfg fuzzing.