Skip to content

[CI] Add Infer Static Code Analysis#521

Merged
hydai merged 4 commits intoWasmEdge:masterfrom
avinal:avinal/issue519
Oct 23, 2021
Merged

[CI] Add Infer Static Code Analysis#521
hydai merged 4 commits intoWasmEdge:masterfrom
avinal:avinal/issue519

Conversation

@avinal
Copy link
Copy Markdown
Contributor

@avinal avinal commented Oct 22, 2021

Changes

Test Runs

Signed-off-by: Avinal Kumar avinal.xlvii@gmail.com

@avinal avinal requested a review from hydai as a code owner October 22, 2021 09:03
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 22, 2021

Codecov Report

Merging #521 (1cc7662) into master (cf290a4) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #521   +/-   ##
=======================================
  Coverage   70.52%   70.52%           
=======================================
  Files         115      115           
  Lines       15186    15186           
  Branches     3539     3539           
=======================================
  Hits        10710    10710           
  Misses       2480     2480           
  Partials     1996     1996           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf290a4...1cc7662. Read the comment docs.

@juntao
Copy link
Copy Markdown
Member

juntao commented Oct 22, 2021

Thanks! Does it cover the source files under the tools folder?

@avinal
Copy link
Copy Markdown
Contributor Author

avinal commented Oct 22, 2021

Thanks! Does it cover the source files under the tools folder?

No, I did not see into optional components. let me add it. Should I add tests too?

Fixes WasmEdge#519
Uses GitHub Actions and https://github.com/facebook/infer

Signed-off-by: Avinal Kumar <avinal.xlvii@gmail.com>
Signed-off-by: Michael Yuan <michael@michaelyuan.com>
Allow the fbinfer workflow to be triggered manually. Also, upload the report.txt as an artifact.

Signed-off-by: Michael Yuan <michael@michaelyuan.com>
@juntao
Copy link
Copy Markdown
Member

juntao commented Oct 22, 2021

I added some changes. Can you trigger it to run again on your repo? Thanks!

@avinal
Copy link
Copy Markdown
Contributor Author

avinal commented Oct 22, 2021

No, I can't see any option to manually trigger. As far as I remember it must be merged before forks can use it.

@juntao
Copy link
Copy Markdown
Member

juntao commented Oct 23, 2021

Hmm, how did you create those github actions reports in your repo then? 😂

@avinal
Copy link
Copy Markdown
Contributor Author

avinal commented Oct 23, 2021

Well, when I was testing, I added a trigger on my pull request branch, later I amended the commits 🤭.

@juntao
Copy link
Copy Markdown
Member

juntao commented Oct 23, 2021

That is a neat trick. Can I ask you to do the same so that we can have a build with uploaded report.txt artifact? Thanks!

@avinal
Copy link
Copy Markdown
Contributor Author

avinal commented Oct 23, 2021

Yeah sure, I identified some mistakes too, I will fix them.

@avinal
Copy link
Copy Markdown
Contributor Author

avinal commented Oct 23, 2021

@juntao
Copy link
Copy Markdown
Member

juntao commented Oct 23, 2021

Hmm, this report only has 4 issues. Run 6 identified 26?

https://github.com/avinal/WasmEdge/runs/3973556059?check_suite_focus=true#step:5:272

@avinal
Copy link
Copy Markdown
Contributor Author

avinal commented Oct 23, 2021

Ohh I see now. Seems like there is a conflict in results between different ways to analyze using CMake. https://fbinfer.com/docs/next/analyzing-apps-or-projects#cmake

I just wanted to see which method is faster, assuming both will produce the same result. Let me try that again.

@avinal
Copy link
Copy Markdown
Contributor Author

avinal commented Oct 23, 2021

Got 28 now 😕
report.txt

@juntao
Copy link
Copy Markdown
Member

juntao commented Oct 23, 2021

Thanks! :)

One more thing -- you might need to clean up the test commits to fix the DCO requirements.

- change the method to generate report using Infer
- Fix upload report to build artifact step

Signed-off-by: Avinal Kumar <avinal.xlvii@gmail.com>
Copy link
Copy Markdown
Member

@juntao juntao left a comment

Choose a reason for hiding this comment

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

I think this looks good.

@hydai hydai merged commit a525815 into WasmEdge:master Oct 23, 2021
@juntao juntao added the hacktoberfest-accepted Accepted and merged PR for Hacktoberfest label Oct 23, 2021
@juntao
Copy link
Copy Markdown
Member

juntao commented Oct 23, 2021

Thank you @avinal for your contribution. I added all issues identified by fbinfer as hacktoberfest issues. :)

https://github.com/WasmEdge/WasmEdge/issues?q=is%3Aissue+is%3Aopen+label%3AImprovement

avinal added a commit to avinal/WasmEdge that referenced this pull request Oct 24, 2021
- Removes redundant branch name mereged via PR WasmEdge#521

Signed-off-by: Avinal Kumar <avinal.xlvii@gmail.com>
@avinal
Copy link
Copy Markdown
Contributor Author

avinal commented Oct 24, 2021

Thanks @juntao, I will try to pick some issues in the coming days. Just for the note, a redundant line slipped through our reviews. I have added a PR to fix that.

@avinal avinal deleted the avinal/issue519 branch October 24, 2021 04:49
hydai pushed a commit that referenced this pull request Oct 24, 2021
- Removes redundant branch name mereged via PR #521

Signed-off-by: Avinal Kumar <avinal.xlvii@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest-accepted Accepted and merged PR for Hacktoberfest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create a static code analysis report

3 participants