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

Add cargo xtask commands for dependencies and vulnerabilities checks #1181

Merged
merged 15 commits into from Jan 27, 2024

Conversation

syl20bnr
Copy link
Member

@syl20bnr syl20bnr commented Jan 26, 2024

Checklist

  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Related Issues/PRs

Fixes issue #965

Changes

This PR adds two new commands to cargo xtask:

  • cargo xtask dependencies
  • cargo xtask vulnerabilities

All the checks defined in our github workflows are added. I also implemented all the sanitizers. I did not thoroughly test all of them, some additional tweaks might be necessary.

I tried to be as clear as possible in the command line documentation so that users can just explore what is available and then the help should guide them in order to correctly execute the checks (for instance some checks need to be executed using a nightly toolchain). The cargo third-party commands should be automatically installed as well.

Dependencies documentation generation is also skipped with --no-deps.

@syl20bnr syl20bnr force-pushed the xtask/new-command-vulnerabilities branch 3 times, most recently from 13d28d3 to aab6578 Compare January 26, 2024 04:58
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b9bd429) 84.46% compared to head (4673f6e) 84.46%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1181   +/-   ##
=======================================
  Coverage   84.46%   84.46%           
=======================================
  Files         546      546           
  Lines       61296    61296           
=======================================
  Hits        51774    51774           
  Misses       9522     9522           

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

Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work!

Just simple changes and two questions:

Instead of having patterns such as "hi".to_string()", can we reduce allocations using std::borrow::Cow<'static, str>` in the code?

Can we add these new commands to CI?

xtask/src/vulnerabilities.rs Outdated Show resolved Hide resolved
xtask/src/vulnerabilities.rs Outdated Show resolved Hide resolved
xtask/src/vulnerabilities.rs Outdated Show resolved Hide resolved
xtask/src/runchecks.rs Outdated Show resolved Hide resolved
xtask/src/dependencies.rs Outdated Show resolved Hide resolved
xtask/src/dependencies.rs Outdated Show resolved Hide resolved
@syl20bnr
Copy link
Member Author

Thank you for the review, I pushed some fixes.

Instead of having patterns such as "hi".to_string()", can we reduce allocations using std::borrow::Cow<'static, str>` in the code?

Do you have a concrete example you can share ?

Can we add these new commands to CI?

I was planning to do that but in the end I prefer to open a different PR for this.

@syl20bnr syl20bnr requested a review from Luni-4 January 26, 2024 15:22
@syl20bnr syl20bnr force-pushed the xtask/new-command-vulnerabilities branch from 7008d88 to daa49b3 Compare January 26, 2024 15:23
@Luni-4
Copy link
Collaborator

Luni-4 commented Jan 26, 2024

Do you have a concrete example you can share ?

I think the best examples could be the ones listed in the documentation page https://doc.rust-lang.org/std/borrow/enum.Cow.html

Actually, you can write std::borrow::Cow::Borrowed("hi") if it's borrowed, or std::borrow::Cow::Owned("hi".to_string()) if it's necessary to own the type, but we avoid allocations in case of a borrowed value

Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

Good job, not sure if all those checks are important, but at least we have a way to test them easily.

xtask/src/dependencies.rs Outdated Show resolved Hide resolved
xtask/src/dependencies.rs Show resolved Hide resolved
xtask/src/dependencies.rs Outdated Show resolved Hide resolved
xtask/src/utils.rs Outdated Show resolved Hide resolved
xtask/src/utils.rs Outdated Show resolved Hide resolved
xtask/src/utils.rs Outdated Show resolved Hide resolved
xtask/src/vulnerabilities.rs Outdated Show resolved Hide resolved
xtask/src/vulnerabilities.rs Outdated Show resolved Hide resolved
xtask/src/vulnerabilities.rs Outdated Show resolved Hide resolved
@syl20bnr
Copy link
Member Author

Ready for another round of review.

I parsed the strings returned by rustup in the end as @nathanielsimard proposed. I also added some tests and converted some strings to constants. Utils module is now a directory.

@syl20bnr syl20bnr force-pushed the xtask/new-command-vulnerabilities branch from f0c6431 to 883de0f Compare January 26, 2024 21:12
Copy link
Member

@nathanielsimard nathanielsimard left a comment

Choose a reason for hiding this comment

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

LGTM

@syl20bnr
Copy link
Member Author

@Luni-4 Is that OK with you too ?

@syl20bnr syl20bnr force-pushed the xtask/new-command-vulnerabilities branch from 883de0f to 4673f6e Compare January 27, 2024 04:13
Copy link
Collaborator

@Luni-4 Luni-4 left a comment

Choose a reason for hiding this comment

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

Fine for me too! Thanks a lot for your work!

@syl20bnr syl20bnr merged commit 3814c4c into main Jan 27, 2024
15 checks passed
@nathanielsimard nathanielsimard deleted the xtask/new-command-vulnerabilities branch January 27, 2024 15:34
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.

None yet

3 participants