Wasmtime GitHub notifications bot (Sep 26 2024 at 22:44): Wasmtime GitHub notifications bot (Sep 26 2024 at 22:44):abrown opened PR #9318 from abrown:code-scans to bytecodealliance:main:
In the past, we've overlooked clippy warnings that get lost in the CI build logs. This change would collect all of those warnings, put them in [SARIF] form, and list them in GitHub's code scanning view. I recently added this to
ittapiand it looks like this: [Code Scanning]. This means warnings and errors will show up on the security tab as a notification; the UI allows one to dismiss the warnings. There might be some integration with PRs but I haven't experimented with that.I configured this to also run periodically (every Tuesday night); we can remove that if we only want commits to
main, e.g. If we do adopt this, we should think about what to do with theclippyjob inmain.yml--does it stay or go?[SARIF]: https://sarifweb.azurewebsites.net
[Code Scanning]: https://github.com/intel/ittapi/security/code-scanning?query=branch%3Amaster+<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Sep 26 2024 at 22:47):abrown updated PR #9318.
Wasmtime GitHub notifications bot (Sep 26 2024 at 22:54):abrown updated PR #9318.
Wasmtime GitHub notifications bot (Sep 26 2024 at 22:58):abrown updated PR #9318.
Wasmtime GitHub notifications bot (Sep 26 2024 at 22:59):abrown updated PR #9318.
Wasmtime GitHub notifications bot (Sep 26 2024 at 23:05):github-advanced-security[bot] commented on PR #9318:
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.
Wasmtime GitHub notifications bot (Sep 26 2024 at 23:10):abrown has marked PR #9318 as ready for review.
Wasmtime GitHub notifications bot (Sep 26 2024 at 23:10):abrown requested fitzgen for a review on PR #9318.
Wasmtime GitHub notifications bot (Sep 26 2024 at 23:10):abrown requested wasmtime-default-reviewers for a review on PR #9318.
Wasmtime GitHub notifications bot (Sep 27 2024 at 05:26):fitzgen commented on PR #9318:
Because we already gate PRs on clippy, so I don't think this would ever catch anything? Am I missing something? I guess for new versions of clippy with new lints? But we configure crates to inherit the workspace lints, which allows all clippy rules by default, and then we only deny select ones.
In general, I'd rather catch these things at the time they're authored (ie in the PR) than after the fact on the next Tuesday.
But I feel like I might be missing something here...
Wasmtime GitHub notifications bot (Sep 27 2024 at 05:26):fitzgen edited a comment on PR #9318:
Because we already gate PRs on clippy, I don't think this would ever catch anything? Am I missing something? I guess for new versions of clippy with new lints? But we configure crates to inherit the workspace lints, which allows all clippy rules by default, and then we only deny select ones.
In general, I'd rather catch these things at the time they're authored (ie in the PR) than after the fact on the next Tuesday.
But I feel like I might be missing something here...
Wasmtime GitHub notifications bot (Sep 27 2024 at 23:39):This code scan is configured to run on every PR _and_ on Tuesdays. The results of code scans during a PR should be available in the PR itself (docs... IIUC). The results of code scans otherwise (on
mainand on Tuesdays) should show up in GitHub's Code Scanning UI.This PR is not about adding any new scan functionality, it's about integrating better with GitHub:
- warnings should show up in the UI instead of being lost in build logs; this means that we could enable more warnings if we wanted to)
- errors should show up as PR comments; this means we don't have to dig through build logs to see what failed CI
- if any issues somehow get through the review process, they show up in the "Security" tab; this improves visibility in the UI for any warnings
Wasmtime GitHub notifications bot (Sep 27 2024 at 23:40):abrown edited a comment on PR #9318:
This code scan is configured to run on every PR _and_ on Tuesdays. The results of code scans during a PR should be available in the PR itself (docs... IIUC). The results of code scans otherwise (on
mainand on Tuesdays) should show up in GitHub's Code Scanning UI.This PR is not about adding any new scan functionality, it's about integrating better with GitHub:
- warnings should show up in the UI instead of being lost in build logs; this means that we could enable more warnings if we wanted to
- errors should show up as PR comments; this means we don't have to dig through build logs to see what failed CI
- if any issues somehow get through the review process, they show up in the "Security" tab; this improves visibility in the UI for any warnings
Wasmtime GitHub notifications bot (Sep 27 2024 at 23:43):One other comment: my recollection is that we limit our clippy lints to allow or deny only because of the lack of visibility into warnings. My take is that, beyond just the "use the nice tools better" argument, if we wanted to be more picky and warn (but not deny) in more cases, this would be the way to do that.
Wasmtime GitHub notifications bot (Sep 28 2024 at 15:49):alexcrichton commented on PR #9318:
I share similar confusion to @fitzgen about the state as-is here in the sense that we're "guaranteed clippy clean" on all PRs because we
-Dwarningsin CI and also run clippy on CI. Or at least for the "run continuously and have a dashboard" aspect I don't think this'll be too useful without other changes.Now better surfacing errors in CI to PRs, that sounds great! Do you have an example of what that might look like? Ideally we could surface things like normal compile errors too, but I'm not sure how it works. Or could you perhaps enable a new clippy lint on this PR and we can see what the UI looks like? (assuming enabling a new clippy lint will have lots of errors)
For exploring this a bit, it looks like the links you have (e.g. Code Scanning) are unfortunately only available to maintainers. (it's a 404 for me). I think that means that the dashboard access wouldn't be accessible to contributors-at-large and instead just maintainers?
I can also expand a bit more on the Clippy bits we have in that you're right that we allow-by-default all clippy lints and then selectively enable them. One of the purposes for this is that it's not always the easiest to get PRs from contributors that run
cargo clippyand then make thousands of lines of changes to appease clippy. Clippy's defaults are, at least in my opinion, not always the best suggestions and are not universally applicable. This means that the allow-by-default policy isn't just because we want clean CI, it's also that we don't want to get accidentally overburdened by contributors sending clippy fixes.So overall I'd love to get a better UI for surfacing CI errors into PRs, but I don't think that some of the other benefits you've outlined will be as applicable to us?
Wasmtime GitHub notifications bot (Sep 28 2024 at 15:53):alexcrichton commented on PR #9318:
On the clippy front I can also expand a bit in that it's fine to enable more clippy lints. The only caveat is that it'll want to have some motivation for why it's applicable to Wasmtime or general agreement that Clippy's suggestions are "basically alwasy right" and not accidentally reducing the quality fo the code. For example https://github.com/bytecodealliance/wasmtime/pull/9025 enabled a clippy lint for the whole workspace and https://github.com/bytecodealliance/wasmtime/pull/9209 enabled a lint for just one tree of modules.
This reminds me that we should document this policy...
Wasmtime GitHub notifications bot (Sep 28 2024 at 21:13):fitzgen commented on PR #9318:
+1 to what Alex said. If we can make clippy errors more visible and more easily parseable in CI failures, that's great. But if this makes them only visible to maintainers, that's not really gonna work. And the cron-job aspect is something we shouldn't need.
Wasmtime GitHub notifications bot (Sep 29 2024 at 11:47):Github actions supports problem matches to parse compiler errors and show then inline in the Files tab of a PR.
Wasmtime GitHub notifications bot (Oct 02 2024 at 00:33):Any suggestions on a clippy lint to enable to check the GitHub PR interaction?
Wasmtime GitHub notifications bot (Oct 02 2024 at 15:02):alexcrichton commented on PR #9318:
I randomly selected
clippy::question_marklocally and got a few warnings perhaps? I'm not sure how the integration here will work with warnings-on-files-the-PR-didn't-touch though.
Wasmtime GitHub notifications bot (Oct 02 2024 at 15:23):abrown updated PR #9318.
Wasmtime GitHub notifications bot (Oct 02 2024 at 15:29):abrown updated PR #9318.
Wasmtime GitHub notifications bot (Oct 10 2024 at 17:38):I think what may be going on here is that, since warnings are upgraded to errors, the clippy task will just fail.
Last updated: Nov 22 2024 at 16:03 UTC