Skip to content
This repository has been archived by the owner on Oct 24, 2022. It is now read-only.

backend: Set remote dependency branch as main #33

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

vireshk
Copy link
Contributor

@vireshk vireshk commented Sep 3, 2021

The builds are failing since the the remote branch's name is changed to
main from master. Fix it.

Signed-off-by: Viresh Kumar [email protected]

@andreeaflorescu
Copy link
Member

@vireshk can you sign your commit?

@vireshk
Copy link
Contributor Author

vireshk commented Sep 3, 2021

@vireshk can you sign your commit?

Did that, I created this pull request using the web interface (my first) and so the mess :(

@andreeaflorescu
Copy link
Member

The web interface is not adding the signed-off-by, that's why I don't use it.

@vireshk
Copy link
Contributor Author

vireshk commented Sep 3, 2021

@andreeaflorescu the build isn't starting again, how do I trigger it ?

@vireshk vireshk changed the title Set branch to main for github dependencies backend: Set remote branch as main for github dependencies Sep 3, 2021
@vireshk vireshk changed the title backend: Set remote branch as main for github dependencies backend: Set remote dependency branch as main Sep 3, 2021
@slp
Copy link
Collaborator

slp commented Sep 3, 2021

While the master branch has been renamed to main, the latter has been made the default one. Do we really need to explicitly reference main? Which error are you getting?

@vireshk
Copy link
Contributor Author

vireshk commented Sep 3, 2021

While the master branch has been renamed to main, the latter has been made the default one. Do we really need to explicitly reference main? Which error are you getting?

https://buildkite.com/rust-vmm/vhost-user-backend-ci/builds/60#0e2f7b7b-b1b3-41fa-9244-6c75223742de

@slp
Copy link
Collaborator

slp commented Sep 3, 2021

While the master branch has been renamed to main, the latter has been made the default one. Do we really need to explicitly reference main? Which error are you getting?

https://buildkite.com/rust-vmm/vhost-user-backend-ci/builds/60#0e2f7b7b-b1b3-41fa-9244-6c75223742de

This should have been fixed by rust-lang/cargo#9133. I've also confirmed it's working fine on my laptop.

Is our CI using an older version?

@vireshk
Copy link
Contributor Author

vireshk commented Sep 3, 2021

I will let @andreeaflorescu take over this discussion now :)

@andreeaflorescu
Copy link
Member

andreeaflorescu commented Sep 3, 2021

I guess it depends on the cargo version that you are using. In what version was that fixed?

Locally, for me it fails with:

$ cargo build
    Updating crates.io index
    Updating git repository `https://github.com/rust-vmm/vhost-user-backend`
warning: fetching `master` branch from `https://github.com/rust-vmm/vhost-user-backend` but the `HEAD` reference for this repository is not the `master` branch. This behavior will change in Cargo in the future and your build may break, so it's recommended to place `branch = "master"` in Cargo.toml when depending on this git repository to ensure that your build will continue to work.
    Updating git repository `https://github.com/rust-vmm/vhost`
warning: fetching `master` branch from `https://github.com/rust-vmm/vhost` but the `HEAD` reference for this repository is not the `master` branch. This behavior will change in Cargo in the future and your build may break, so it's recommended to place `branch = "master"` in Cargo.toml when depending on this git repository to ensure that your build will continue to work.
    Updating git repository `https://github.com/rust-vmm/vm-virtio`
warning: fetching `master` branch from `https://github.com/rust-vmm/vm-virtio` but the `HEAD` reference for this repository is not the `master` branch. This behavior will change in Cargo in the future and your build may break, so it's recommended to place `branch = "master"` in Cargo.toml when depending on this git repository to ensure that your build will continue to work.
    Updating git submodule `https://github.com/rust-vmm/rust-vmm-ci.git`
error: no matching package named `vm-virtio` found
location searched: https://github.com/rust-vmm/vm-virtio
required by package `vhost-user-backend v0.1.0 (https://github.com/rust-vmm/vhost-user-backend#ddc7dd5e)`
    ... which is depended on by `vhost-device-i2c v0.1.0 (/home/ANT.AMAZON.COM/fandree/sources/rust-vmm/vhost-device/src/i2c)`

Using HEAD is not a good option because the builds will fail with any incompatible change in the consuming crates. An explicit commit I think is the preferred way.

@slp
Copy link
Collaborator

slp commented Sep 3, 2021

I guess it depends on the cargo version that you are using. In what version was that fixed?

I'm running the latest stable version:

$ cargo --version
cargo 1.54.0 (5ae8d74b3 2021-06-22)

Using HEAD is not a good option because the builds will fail with any incompatible change in the consuming crates. An explicit commit I think is the preferred way.

I'd certainly prefer fixing this with a rev = "6013dd9" than with a branch = "main".

@vireshk
Copy link
Contributor Author

vireshk commented Sep 3, 2021

I'd certainly prefer fixing this with a rev = "6013dd9" than with a branch = "main".

But that will bind this to an older version (in future), why should we do that ?

@andreeaflorescu
Copy link
Member

I'd certainly prefer fixing this with a rev = "6013dd9" than with a branch = "main".

But that will bind this to an older version (in future), why should we do that ?

The idea is to fix the revision, and then update the dependabot configuration such that it provides the updates to the revision. This comes with the advantage that the main branch always builds, and you are not lagging behind because dependabot will open PRs to update the revision whenever a new commit is added to vm-virtio for example.

If you look at the current dependabot configuration, it does updates for git:

- package-ecosystem: gitsubmodule
(the package ecosystem is submodule). This means that it updates only the rust-vmm-ci.

This can be setup to also update crate dependencies, by specifying as ecosystem cargo. You can check the cloud hypervisor configuration for an example: https://github.com/cloud-hypervisor/cloud-hypervisor/blob/6c142e35f787aad2ac91f24e2379d5b1c685d599/.github/dependabot.yml#L3

With dependabot, it will be easy to always use the latest available option as long as the build still passes. IF the build fails, then manual updates will be needed, but this is expected to happen when you do not want to break the main branch.

@slp
Copy link
Collaborator

slp commented Sep 3, 2021

I'd certainly prefer fixing this with a rev = "6013dd9" than with a branch = "main".

But that will bind this to an older version (in future), why should we do that ?

Because, until it's published on crates.io, vm-virtio is allowed to introduce breaking changes in its API. This means we can't guarantee that we will support any future version without changes in vhost-user-backend itself.

In other words, referring to particular shields us against getting broken by changes in vm-virtio. Of course that means we'll need to periodically check manually if we should update the commit reference to incorporate new feature/fixes but, in practice, that's something we'll also need to do once vm-virtio gets published, so we might as well get used to that now.

EDIT: As @andreeaflorescu pointed out, dependabot can helps us automate some of this work.

The builds are failing since the remote branch's name is changed to
main from master. Fix it by adding the rev for virtio-queue.

Signed-off-by: Viresh Kumar <[email protected]>
@vireshk
Copy link
Contributor Author

vireshk commented Sep 3, 2021

I keep forgetting that dependabot have magical powers. Fixed it with rev now.

@slp
Copy link
Collaborator

slp commented Sep 3, 2021

@vireshk Thanks!

@jiangliu If we merge this, you'll need to drop your last commit in #30 . Are you OK with that?

@jiangliu jiangliu merged commit f0af5f7 into rust-vmm:main Sep 3, 2021
@vireshk vireshk deleted the patch-1 branch September 6, 2021 07:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants