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

Create GitHub Action for Rust-Clippy #8122

Open
josepalafox opened this issue Dec 13, 2021 · 13 comments
Open

Create GitHub Action for Rust-Clippy #8122

josepalafox opened this issue Dec 13, 2021 · 13 comments

Comments

@josepalafox
Copy link

Description

If adding Sarif Support as outlined in #8121

Write GitHub Action and integration as described here.

Result is a GH action that can trigger Rust-Clippy scans during CI and then upload the results to the Code Scanning interface via trivial one click install from the GH UI. GitHub provides actions minutes for free to all users, and code scanning is available on OSS projects for free. Will help drive adoption of tool for GH hosted Rust OSS projects.

@michaelcfanning and @yongyan-gh

Version

No response

Additional Labels

No response

@matthiaskrgr
Copy link
Member

@josepalafox
Copy link
Author

josepalafox commented Dec 14, 2021 via email

@flip1995
Copy link
Member

So the Clippy repo itself won't implement a GitHub action. I also don't see why such an action would be necessary. You can just add

- name: Run Clippy
  run: cargo clippy -Dwarnings

as a step for your workflow will fail, if there is Clippy output.

IIUC, what you're missing is output in SARIF format? Clippy is able to output JSON with cargo clippy --message-format=json, if you use cargo and clippy-driver --error-format=json if you don't.

I'm not familiar with the SARIF format, so I don't know if the JSON output complies with this format, but if you need that specific format, you'd have to raise this issue with rustc directly, since all the error IO Clippy does is shared with rustc.

@josepalafox
Copy link
Author

josepalafox commented Dec 20, 2021

Thank you for the information. I think what we can do is see if we can just make an intermediary action to munge the existing output into SARIF and upload it, then support does not need to be added to rustc. @michaelcfanning and @yongyan-gh does this approach make sense? Basically we use the existing action to run the linter, a second action to munge, and then a third to upload and string all of those into a workflow file for the starter-workflows page?

Does Clippy have a SVG logo we can use and a short description (140 char) the maintainers would prefer for us to put in the UI?

@flip1995
Copy link
Member

Does Clippy have a SVG logo we can use and a short description (140 char) the maintainers would prefer for us to put in the UI?

We don't have a logo right now. As for a text, just use the first sentence (and more, if you want) from the README:

A collection of lints to catch common mistakes and improve your Rust code.

@yongyan-gh
Copy link

@josepalafox pls confirm my understanding based on your proposal:
A new github action should be created includes 3 steps:

  1. Calls existing rust clippy action (https://github.com/actions-rs/clippy-check) to generate JSON output.
  2. Calls an action to convert clippy JSON output into SARIF format.
  3. Upload SARIF file converted from clippy JSON using existing action (e.g. github/codeql-action/upload-sarif)
    Where should be code for step 2 above located?

@flip1995 can you pls share the documentation of clippy JSON output?

@josepalafox
Copy link
Author

josepalafox commented Dec 20, 2021

Yes.

Ideally it lives in the Clippy project somewhere if y'all are open to accepting the PR, @flip1995 & @matthiaskrgr ?

@xFrednet
Copy link
Member

The JSON output is handled by rustc. You can find the official documentation here: https://doc.rust-lang.org/rustc/json.html

@flip1995
Copy link
Member

flip1995 commented Dec 20, 2021

@yongyan-gh

  1. You don't necessary need to use the clippy-check action. You can just run cargo clippy without an action. It is pre-installed in GHA hosted runners.
  2. An action / script / ... that converts the JSON output from Clippy / Rust to SARIF should be enough. As @xFrednet said, this is all handled and shared with rustc. So Clippy outputs the same format as rustc. You may want to make the case in rustc (possibly as an RFC or MCP) to introduce a SARIF error format. But this is nothing that could be done in Clippy.

FWIW, there's the sarif-rs project that seems to be maintained, that can convert the output of Clippy (and with that probably also rustc) to SARIF. They also have instructions on how to use this crate in a GHA: https://github.com/psastras/sarif-rs/tree/main/clippy-sarif#example-1

Ideally it lives in the Clippy project somewhere

I don't see why this should live in the Clippy repo. I don't think any maintainer has experience working with or on SARIF, so this would put a maintenance burden on us that we would like to avoid.

@yongyan-gh
Copy link

Hi @flip1995, thanks for the information, I tried to install clippy-sarif and get SARIF result in a test Github worflow but installation failed due to below error. I opened an issue psastras/sarif-rs#104 in sarif-rs, is it the right place to report the issue?

error[E0599]: no method named `about` found for struct `Arg` in the current scope
  --> /home/yongyan/.cargo/registry/src/github.com-1ecc6299db9ec823/clippy-sarif-0.2.20/src/bin.rs:90:18
   |
90 |                 .about("input file; reads from stdin if none is given")
   |                  ^^^^^ method not found in `Arg<'_>`

error[E0599]: no method named `about` found for struct `Arg` in the current scope
  --> /home/yongyan/.cargo/registry/src/github.com-1ecc6299db9ec823/clippy-sarif-0.2.20/src/bin.rs:95:18
   |
95 |                 .about("output file; writes to stdout if none is given")
   |                  ^^^^^ method not found in `Arg<'_>`

For more information about this error, try `rustc --explain E0599`.
error: failed to compile `clippy-sarif v0.2.20`, intermediate artifacts can be found at `/tmp/cargo-installg97K0W`

Caused by:
  could not compile `clippy-sarif` due to 2 previous errors

@GeekMasher
Copy link

@yongyan-gh I found out that the current version rustc and clap doesn't work. I created a PR which I fixed this, its still in review

psastras/sarif-rs#116

@yongyan-gh
Copy link

@GeekMasher thank you for taking care of the build issue.

@GeekMasher
Copy link

@yongyan-gh The latest version should have a fix now

psastras/sarif-rs#104 (comment)

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

No branches or pull requests

6 participants