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

update docs for call analysis. #682

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Conversation

hogo6002
Copy link
Contributor

Previous PR #665 updated '--experimental-call-analysis' to '--call-analysis' and '--no-call-analysis'.
Updating docs to reflect the changes.

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (98933ac) 78.88% compared to head (629fd9e) 78.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #682      +/-   ##
==========================================
+ Coverage   78.88%   78.93%   +0.05%     
==========================================
  Files          84       84              
  Lines        5935     5935              
==========================================
+ Hits         4682     4685       +3     
+ Misses       1050     1048       -2     
+ Partials      203      202       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@another-rex
Copy link
Collaborator

Can you add the experimental tag to the rust call analysis, and change the description to indicate that all the experimental analysis is off by default (rust), while the non experimental languages (go) is turned on by default.

Co-authored-by: Hayley Denbraver <denbraver@google.com>
Co-authored-by: Hayley Denbraver <denbraver@google.com>
@hayleycd
Copy link
Collaborator

I'm confused. Why do we have both a --call-analysis and a --no-call-analysis flag? shouldn't it be either one or another?

@hogo6002
Copy link
Contributor Author

I'm confused. Why do we have both a --call-analysis and a --no-call-analysis flag? shouldn't it be either one or another?

Hi Hayley, it is because we want go call analysis to be enabled by default. So we need to add one more flag to turn it off. More details: #513

@hayleycd
Copy link
Collaborator

But if call analysis is the default behavior, we wouldn't need a flag for it right? Or is it because it is not the default for rust?

Either way, the behavior is not currently clear in the docs.

@oliverchang
Copy link
Collaborator

oliverchang commented Nov 29, 2023

I'm confused. Why do we have both a --call-analysis and a --no-call-analysis flag? shouldn't it be either one or another?

Hi Hayley, it is because we want go call analysis to be enabled by default. So we need to add one more flag to turn it off. More details: #513

And also because not all languages are enabled by default. Only Go is enabled so far. If someone wants rust, then they need to specify --call-analysis=rust.

And if we want only rust, you'd do:

--call-analysis=rust --no-call-analysis=go

@hogo6002 is that right?

@hogo6002
Copy link
Contributor Author

And also because not all languages are enabled by default. Only Go is enabled so far. If someone wants rust, then they need to specify --call-analysis=rust.

And if we want only rust, you'd do:

--call-analysis=rust --no-call-analysis=go

@hogo6002 is that right?

Yeah that's right. With this implementation, we are able to enable/disable call analysis per language.

@hogo6002
Copy link
Contributor Author

hogo6002 commented Nov 29, 2023

But if call analysis is the default behavior, we wouldn't need a flag for it right? Or is it because it is not the default for rust?

Either way, the behavior is not currently clear in the docs.

Call analysis in Go is enabled by default (don't need to put any flags) as it is stable. But users may still want to disable it, so we added a '--no-call-analysis' flag.
Usages:

# this has all non-experimental call analysis being run (only go now)
osv-scanner .

# this enables all call analysis (both go and rust)
osv-scanner --call-analysis=all

# this enables all default call analysis and rust (call analysis for go is also enabled as it is by default.)
osv-scanner --call-analysis=rust

# this disables all call analysis
osv-scanner --disable-call-analysis=all

# this disables call analysis for go, but rust still runs
osv-scanner --call-analysis=rust --disable-call-analysis=go

Copy link
Collaborator

@hayleycd hayleycd left a comment

Choose a reason for hiding this comment

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

I took a closer look at things and I think things are looking alright, especially with the 'experimental' label on the rust section.

Thanks for clarifying about the different flags.

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

5 participants