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.

Sign up for GitHub

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

Jump to bottom

ci: Test for unused dependencies #4117

Merged
merged 4 commits into from
Dec 16, 2024
Merged

ci: Test for unused dependencies #4117

merged 4 commits into from
Dec 16, 2024
+23 −36

Conversation

JonathanWoollett-Light
Copy link
Contributor

@JonathanWoollett-Light JonathanWoollett-Light commented Sep 20, 2023 •
edited
Loading

Changes

Adds a test for unused cargo dependencies.

Reason

Prevents including unused dependencies see #4113 (comment).

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet contribution quality standards.
  • This functionality cannot be added in rust-vmm.

@JonathanWoollett-Light JonathanWoollett-Light added the Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` label Sep 20, 2023
@codecov
Copy link

codecov bot commented Sep 20, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.98%. Comparing base (77d3704) to head (c5a315d).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4117   +/-   ##
=======================================
  Coverage   83.98%   83.98%           
=======================================
  Files         251      251           
  Lines       27889    27889           
=======================================
  Hits        23422    23422           
  Misses       4467     4467
Flag Coverage Δ
5.10-c5n.metal 84.55% <ø> (ø)
5.10-m5n.metal 84.53% <ø> (ø)
5.10-m6a.metal 83.82% <ø> (ø)
5.10-m6g.metal 80.68% <ø> (ø)
5.10-m6i.metal 84.53% <ø> (+<0.01%) ⬆️
5.10-m7g.metal 80.68% <ø> (ø)
6.1-c5n.metal 84.55% <ø> (ø)
6.1-m5n.metal 84.54% <ø> (ø)
6.1-m6a.metal 83.82% <ø> (ø)
6.1-m6g.metal 80.68% <ø> (ø)
6.1-m6i.metal 84.52% <ø> (ø)
6.1-m7g.metal 80.68% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@JonathanWoollett-Light JonathanWoollett-Light added the Status: Blocked Indicates that an issue or pull request cannot currently be worked on label Sep 20, 2023
@JonathanWoollett-Light
Copy link
Contributor Author

Blocked on docker changes from: #4089

@roypat
Copy link
Contributor

roypat commented Sep 25, 2023

Unblocked :)

@JonathanWoollett-Light
Copy link
Contributor Author

#4089 merged, unblocked.

@JonathanWoollett-Light JonathanWoollett-Light removed the Status: Blocked Indicates that an issue or pull request cannot currently be worked on label Sep 25, 2023
@JonathanWoollett-Light JonathanWoollett-Light force-pushed the cargo-udeps branch 4 times, most recently from 96cde67 to 08d6250 Compare September 28, 2023 09:50
@JonathanWoollett-Light JonathanWoollett-Light force-pushed the cargo-udeps branch 2 times, most recently from 2ed8ef8 to 0d7731f Compare October 3, 2023 15:53
@JonathanWoollett-Light JonathanWoollett-Light marked this pull request as ready for review October 3, 2023 15:53
@JonathanWoollett-Light JonathanWoollett-Light added Priority: Medium Indicates than an issue or pull request should be resolved ahead of issues or pull requests labelled Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` labels Oct 3, 2023
bchalios
bchalios requested changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@bchalios bchalios left a comment

Choose a reason for hiding this comment

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

Could you please break this down to two commits, one that adds the test (test, Docker changes) and one that removes the unused dependencies?

Also, there are here, seemingly, completely unrelated changes (the ones in the documentation). Please drop these and add them in a different PR.

@JonathanWoollett-Light JonathanWoollett-Light force-pushed the cargo-udeps branch 2 times, most recently from 498b576 to 2830e0d Compare October 4, 2023 10:34
@JonathanWoollett-Light
Copy link
Contributor Author

Could you please break this down to two commits, one that adds the test (test, Docker changes) and one that removes the unused dependencies?

I've updated the PR breaking it down into 4 commits.

Also, there are here, seemingly, completely unrelated changes (the ones in the documentation). Please drop these and add them in a different PR.

These are required by the updated version of markdown lint. mdl is a ruby dependency so we don't have a lock file for it.

roypat
roypat previously approved these changes Oct 4, 2023
View reviewed changes
@JonathanWoollett-Light JonathanWoollett-Light added Status: Blocked Indicates that an issue or pull request cannot currently be worked on and removed Status: Awaiting review Indicates that a pull request is ready to be reviewed labels Dec 7, 2023
@roypat roypat mentioned this pull request Dec 10, 2024
Merged
10 tasks
@roypat
test: add support for running commands with nightly toolchain
0f5eee3 
Add a `nightly` flag to the `cargo` function, which causes the requested
command to be executing using the nightly toolchain.

We have a nightly toolchain in our container due to kani. But since the
toolchain version is prescribed by kani, we need to dynamically
determine its name from `rustup toolchain list`, and cannot just use
`+nightly`.

Signed-off-by: Patrick Roy <[email protected]>
@roypat roypat added Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Status: Blocked Indicates that an issue or pull request cannot currently be worked on labels Dec 11, 2024
pb8o
pb8o previously approved these changes Dec 11, 2024
View reviewed changes
tests/integration_tests/build/test_dependencies.py Outdated Show resolved Hide resolved
@roypat
ci: Test for unused cargo dependencies
59d865c 
From: Jonathan Woollett-Light <[email protected]>

Adds test for unused cargo dependencies.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
Co-authored-by: Patrick Roy <[email protected]>
Signed-off-by: Patrick Roy <[email protected]>
roypat
roypat previously approved these changes Dec 11, 2024
View reviewed changes
@roypat roypat requested a review from pb8o December 11, 2024 16:36
@roypat
chore: remove unused dependencies from Cargo.tomls
5088728 
The new test caught these. yay!

Signed-off-by: Patrick Roy <[email protected]>
@roypat roypat merged commit 0bee970 into firecracker-microvm:main Dec 16, 2024
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roypat roypat roypat approved these changes

@bchalios bchalios bchalios approved these changes

@xmarcalx xmarcalx Awaiting requested review from xmarcalx

@kalyazin kalyazin Awaiting requested review from kalyazin

@wearyzen wearyzen Awaiting requested review from wearyzen

@pb8o pb8o Awaiting requested review from pb8o

Assignees

@JonathanWoollett-Light JonathanWoollett-Light

@roypat roypat

Labels
Priority: Medium Indicates than an issue or pull request should be resolved ahead of issues or pull requests labelled Status: Awaiting review Indicates that a pull request is ready to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JonathanWoollett-Light @roypat @bchalios @pb8o