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

build(deps): bump rust-vmm-ci from 7f22582 to 68d4dbf #19

Closed
wants to merge 1 commit into from

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Oct 11, 2021

Bumps rust-vmm-ci from 7f22582 to 68d4dbf.

Commits

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Bumps [rust-vmm-ci](https://github.com/rust-vmm/rust-vmm-ci) from `7f22582` to `68d4dbf`.
- [Release notes](https://github.com/rust-vmm/rust-vmm-ci/releases)
- [Commits](rust-vmm/rust-vmm-ci@7f22582...68d4dbf)

---
updated-dependencies:
- dependency-name: rust-vmm-ci
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot dependabot bot added dependencies Pull requests that update a dependency file submodules Pull requests that update Submodules code labels Oct 11, 2021
@andreeaflorescu
Copy link
Member

andreeaflorescu commented Oct 11, 2021

@liuw @russell-islam we're now running the style checks on examples as well, and it looks like there are some changes required. Could you please fix the style? To do that locally you can run the following commands:

docker run -it --volume $(pwd):/mshv rustvmm/dev:v13
cd mshv
cargo fmt --all -- --check --config format_code_in_doc_comments=true

@liuw
Copy link
Member

liuw commented Oct 11, 2021

@liuw @russell-islam we're now running the style checks on examples as well, and it looks like there are some changes required. Could you please fix the style? To do that locally you can run the following commands:

docker run -it --volume $(pwd):/mshv rustvmm/dev:13
cd mshv
cargo fmt --all -- --check --config format_code_in_doc_comments=true

When I run the aforementioned docker command I got the following.

Unable to find image 'rustvmm/dev:13' locally
docker: Error response from daemon: manifest for rustvmm/dev:13 not found: manifest unknown: manifest unknown.
See 'docker run --help'.

@andreeaflorescu
Copy link
Member

Unable to find image 'rustvmm/dev:13' locally
docker: Error response from daemon: manifest for rustvmm/dev:13 not found: manifest unknown: manifest unknown.
See 'docker run --help'.

I fixed the example. It was missing a v in front of 13.

@liuw
Copy link
Member

liuw commented Oct 11, 2021

For avoidance of doubt, I can just fish out that cargo clippy command myself. I'm working on a fix now.

Unable to find image 'rustvmm/dev:13' locally
docker: Error response from daemon: manifest for rustvmm/dev:13 not found: manifest unknown: manifest unknown.
See 'docker run --help'.

I fixed the example. It was missing a v in front of 13.

Sure. Thanks. I have in fact fished out the command from the logs and am running it without docker.

@liuw
Copy link
Member

liuw commented Oct 11, 2021

I have one question though, rustfmt's document says format_code_in_doc_comments is not stable. Why is it used then? It looks to me it can be gone or subject to change. Wouldn't that break the CI?

@andreeaflorescu
Copy link
Member

Sure. Thanks. I have in fact fished out the command from the logs and am running it without docker.

The reason for running with the container is that the style is tied to the Rust version. That's why it is important to use the same version as the one that is running in the CI.

I have one question though, clippy's document says format_code_in_doc_comments is not stable. Why is it used then? It looks to me it can be gone or subject to change. Wouldn't that break the CI?

It is now part of Rust stable (i.e. you can run the check with cargo fmt), so I am a bit surprised by this. Do you have a link to where it says that it's unstable?

@liuw
Copy link
Member

liuw commented Oct 11, 2021

Sure. Thanks. I have in fact fished out the command from the logs and am running it without docker.

The reason for running with the container is that the style is tied to the Rust version. That's why it is important to use the same version as the one that is running in the CI.

I have one question though, clippy's document says format_code_in_doc_comments is not stable. Why is it used then? It looks to me it can be gone or subject to change. Wouldn't that break the CI?

It is now part of Rust stable (i.e. you can run the check with cargo fmt), so I am a bit surprised by this. Do you have a link to where it says that it's unstable?

I forgot to paste in the link. Here you go. https://rust-lang.github.io/rustfmt/?version=v1.4.36&search=format_code#format_code_in_doc_comments

@liuw
Copy link
Member

liuw commented Oct 11, 2021

Oh, my bad. That doc is for rustfmt. I have not found a link for clippy. Sorry for the confusion.

@andreeaflorescu
Copy link
Member

No problem.

Okay, so I took a look at the issue that tracks stabilizing this option (rust-lang/rustfmt#3348), and I think that the risk for it to disappear is low. I think what can change from a version to another is how it is used/called which I don't think is such a big problem because the CI will not fail unless we upgrade to a newer rustfmt version. Even then, it looks like it is fixable, and the fix will only need to be implemented in rust-vmm-ci, and then it will just be pulled by the crates. We now have a CI for the rust-vmm-ci so if/when changes are required, we should catch them before PRs in rust-vmm-ci are merged.

I find the formatting of code examples a good thing, so if you don't mind I'd like to keep it. If anything goes wrong on newer versions, we can always go back to running fmt only on code. WDYT?

@liuw
Copy link
Member

liuw commented Oct 11, 2021

No problem.

Okay, so I took a look at the issue that tracks stabilizing this option (rust-lang/rustfmt#3348), and I think that the risk for it to disappear is low. I think what can change from a version to another is how it is used/called which I don't think is such a big problem because the CI will not fail unless we upgrade to a newer rustfmt version. Even then, it looks like it is fixable, and the fix will only need to be implemented in rust-vmm-ci, and then it will just be pulled by the crates. We now have a CI for the rust-vmm-ci so if/when changes are required, we should catch them before PRs in rust-vmm-ci are merged.

I find the formatting of code examples a good thing, so if you don't mind I'd like to keep it. If anything goes wrong on newer versions, we can always go back to running fmt only on code. WDYT?

Sure. We can keep it. Just wanted to make sure you're aware of the potential pitfall from using that option. :-)

Copy link
Member

@liuw liuw left a comment

Choose a reason for hiding this comment

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

Needs to be merged after #20 is merged.

@liuw liuw closed this Oct 11, 2021
@dependabot @github
Copy link
Contributor Author

dependabot bot commented on behalf of github Oct 11, 2021

OK, I won't notify you again about this release, but will get in touch when a new version is available. You can also ignore all major, minor, or patch releases for a dependency by adding an ignore condition with the desired update_types to your config file.

If you change your mind, just re-open this PR and I'll resolve any conflicts on it.

@liuw liuw reopened this Oct 11, 2021
@dependabot @github
Copy link
Contributor Author

dependabot bot commented on behalf of github Oct 11, 2021

Superseded by #21.

@dependabot dependabot bot closed this Oct 11, 2021
@dependabot dependabot bot deleted the dependabot/submodules/rust-vmm-ci-68d4dbf branch October 11, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file submodules Pull requests that update Submodules code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants