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

[Feature Request] Don't check KVM in tools/devtool checkstyle #4768

Closed
3 tasks done
zulinx86 opened this issue Sep 2, 2024 · 3 comments
Closed
3 tasks done

[Feature Request] Don't check KVM in tools/devtool checkstyle #4768

zulinx86 opened this issue Sep 2, 2024 · 3 comments
Labels
Good first issue Indicates a good issue for first-time contributors Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Parked Indicates that an issues or pull request will be revisited later

Comments

@zulinx86
Copy link
Contributor

zulinx86 commented Sep 2, 2024

Branched from issue #4747 .

Feature Request

Since tools/devtool checkstyle only checks coding style, it doesn't require KVM, but it does the KVM existence check. That prevents users from running it on their development env without KVM.

Describe the desired solution

Currently, it just calls cmd_test() that does the KVM check.

https://github.com/firecracker-microvm/firecracker/blob/main/tools/devtool#L900-L902

cmd_checkstyle() {
    cmd_test --no-build -- integration_tests/style -n 4 --dist worksteal
}

https://github.com/firecracker-microvm/firecracker/blob/main/tools/devtool#L686

cmd_test() {
// snipped
    ensure_kvm
// snipped
}

We might be able to add a new option of cmd_test() to bypass the check.

Describe possible alternatives

An alternative could be hardcoding a simplified version of cmd_test in cmd_checkstyle.

Additional context

N/A

Checks

  • Have you searched the Firecracker Issues database for similar requests?
  • Have you read all the existing relevant Firecracker documentation?
  • Have you read and understood Firecracker's core tenets?
@zulinx86 zulinx86 changed the title [Feature Request] M [Feature Request] Dpj' tools/devtool checkstyle Sep 2, 2024
@zulinx86 zulinx86 changed the title [Feature Request] Dpj' tools/devtool checkstyle [Feature Request] Don't check KVM in tools/devtool checkstyle Sep 2, 2024
@zulinx86 zulinx86 added Good first issue Indicates a good issue for first-time contributors Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Parked Indicates that an issues or pull request will be revisited later labels Sep 2, 2024
@tommady
Copy link
Contributor

tommady commented Oct 23, 2024

Hi, may I take this task?

tommady added a commit to tommady/firecracker that referenced this issue Oct 23, 2024
Introducing a new flag, do_kvm_check, within cmd_test to manage cmd_checkstyle, allowing the bypass of KVM checks as outlined in the PR.
tommady added a commit to tommady/firecracker that referenced this issue Oct 23, 2024
Introducing a new flag, do_kvm_check, within cmd_test to manage cmd_checkstyle, allowing the bypass of KVM checks as outlined in the PR.

Signed-off-by: tommady <[email protected]>
@tommady
Copy link
Contributor

tommady commented Oct 24, 2024

Hi @zulinx86, it seems this issue has been resolved with the merging of this PR. Please let me know if there are any additional goals or directions that this issue is intended to address.
thanks.

@zulinx86
Copy link
Contributor Author

Hi @tommady , thank you so much! I believe your PR fixed this issue!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Indicates a good issue for first-time contributors Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Status: Parked Indicates that an issues or pull request will be revisited later
Projects
None yet
Development

No branches or pull requests

2 participants