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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

zbus update and a fix #412

Merged
merged 4 commits into from Nov 21, 2022
Merged

zbus update and a fix #412

merged 4 commits into from Nov 21, 2022

Conversation

zeenix
Copy link
Contributor

@zeenix zeenix commented Sep 19, 2022

Hi there,

zbus maintainer here. 馃憢 I wanted to help out a bit with zbus part of this project so providing this PR.

@flouthoc
Copy link
Collaborator

Hi @zeenix , Thank you so much, PR looks good but CI is red.

@zeenix
Copy link
Contributor Author

zeenix commented Sep 19, 2022

Hi @zeenix , Thank you so much, PR looks good but CI is red.

Thanks for the quick response. make test was fine on my Fedora machine. I'll check..

@zeenix
Copy link
Contributor Author

zeenix commented Sep 19, 2022

Ah, I think I know the issue: with tight tokio integration enabled, zbus uses tokio API that assume tokio runtime and futures::block_on is being used here. Is it ok if I switch all the code to use tokio::Runtime::block_on instead? Since you enable all tokio features anyway, this makes sense.

On a related note, are there plans to make good use of tokio and async/await?

@mheon
Copy link
Member

mheon commented Sep 19, 2022

Not really? The API process for firewalld is very synchronous - we can't do much while waiting for responses from the server.

Thanks for this, by the way - I've really been neglecting the firewalld/dbus bits of the project.

@Luap99
Copy link
Member

Luap99 commented Sep 19, 2022

The main problem with zbus is the MSRV requirement, which makes it impossible to build on stock debian/ubuntu, see #355 and the ubuntu20_build job

@zeenix
Copy link
Contributor Author

zeenix commented Sep 19, 2022

Not really? The API process for firewalld is very synchronous - we can't do much while waiting for responses from the server.

Ah ok. There is spawn_blocking API for turning blocking calls to async but I'm not familiar enough with the code to say if that's worth it.

Thanks for this, by the way - I've really been neglecting the firewalld/dbus bits of the project.

No worries.

The main problem with zbus is the MSRV requirement, which makes it impossible to build on stock debian/ubuntu, see #355 and the ubuntu20_build job

Most of the MSRV bumps were not planned ones but we tackled the issue of unintentional bumps a month back. What's left now is to decide how recent an MSRV we can require w/o breaking things for debian. If 6 months old releases are OK then that's good enough for me as well.

@zeenix
Copy link
Contributor Author

zeenix commented Sep 22, 2022

The main problem with zbus is the MSRV requirement, which makes it impossible to build on stock debian/ubuntu, see #355 and the ubuntu20_build job

I'm having a bit of difficulty figuring out the latest MSRV that would be good for debian. Could you please let me know which version is that and I'll see if I can port to that version?

@Luap99
Copy link
Member

Luap99 commented Sep 23, 2022

For ubuntu 2004 at least it seems to be 1.59 at the moment: https://packages.ubuntu.com/focal/rustc.
Debian stable seems to shippe a much older version with 1.48: https://tracker.debian.org/pkg/rustc.
I don't think we have to support debian given that the version is very old but v1.59 seems reasonable to me.

@zeenix
Copy link
Contributor Author

zeenix commented Sep 23, 2022

I don't think we have to support debian given that the version is very old but v1.59 seems reasonable to me.

Agreed and I've a much better chance of succeeding. :) I'll see what I can do.

@zeenix
Copy link
Contributor Author

zeenix commented Sep 28, 2022

I briefly looked into what 1.60 features zbus depends on and one that seems impossible to port back from (w/o it being a breaking change) is use of dep: features introduced in cargo 1.60. Any ideas?

@Luap99
Copy link
Member

Luap99 commented Sep 29, 2022

I briefly looked into what 1.60 features zbus depends on and one that seems impossible to port back from (w/o it being a breaking change) is use of dep: features introduced in cargo 1.60. Any ideas?

Can we just wait for ubuntu to update the version? Looks like 22.10 ships with rust v1.61 so I guess we could wait and see if they will backport it to the other ubuntu versions as well.

@zeenix
Copy link
Contributor Author

zeenix commented Sep 29, 2022

Can we just wait for ubuntu to update the version?

Oh you will not hear any objections from me. :) I'm not a user of this crate at all (tbh I keep forgetting what the crate is really about even). I just saw this crate appear as a dependent crate on crates.io for zbus, and then saw that it's still on 2.x so I decided to help out.

@Luap99
Copy link
Member

Luap99 commented Sep 29, 2022

I very much appreciate that, thanks for this contribution and your work on the zbus crate.

@zeenix
Copy link
Contributor Author

zeenix commented Oct 30, 2022 •
edited

Can we just wait for ubuntu to update the version? Looks like 22.10 ships with rust v1.61 so I guess we could wait and see if they will backport it to the other ubuntu versions as well.

Seems they've done that already. I rebased again my work (was trivial actually) so if the Ubuntu20 CI jobs still fails, maybe it needs an update?

@zeenix
Copy link
Contributor Author

zeenix commented Oct 30, 2022

oh, I had forgotten that the issue wasn't just that they don't ship Rust 1.60 but rather that they don't ship zbus 3. Since they do have the MSRV for zbus 3, they should be able to upgrade now.

@zeenix
Copy link
Contributor Author

zeenix commented Nov 19, 2022

@Luap99 I feel like there is a bit of a chicken&egg situation involved if you want to only depend on rust crates already packaged in the distro.

Wouldn't it be better to leave that problem to distros themselves and just ensure that this project along w/ all its deps can be built with Rust tools available in the distro in question?

@Luap99
Copy link
Member

Luap99 commented Nov 21, 2022

@Luap99 I feel like there is a bit of a chicken&egg situation involved if you want to only depend on rust crates already packaged in the distro.

Wouldn't it be better to leave that problem to distros themselves and just ensure that this project along w/ all its deps can be built with Rust tools available in the distro in question?

Wait, the debian/ubuntu cargo install does not use the official crates.io index? Do they only pull from debian sources? If this is the case I agree we should not wait for them. I only care about a to fast moving MSRV.

@Luap99
Copy link
Member

Luap99 commented Nov 21, 2022

Hmm, maybe this error is caused by rust-lang/cargo#10623?

@Luap99
Copy link
Member

Luap99 commented Nov 21, 2022

The latest version should be 1.61 in ubuntu 2004, however our image is not updating on quay.io? https://quay.io/repository/libpod/ubuntu20rust
@baude PTAL

@zeenix
Copy link
Contributor Author

zeenix commented Nov 21, 2022

Wait, the debian/ubuntu cargo install does not use the official crates.io index?

I am not sure about the details but the CI clearly shows that current failure is about too old zbus. Otherwise, we'd have this resolved 22 days ago. :)

@zeenix
Copy link
Contributor Author

zeenix commented Nov 21, 2022

Hmm, maybe this error is caused by rust-lang/cargo#10623?

Ah, makes sense.

@zeenix
Copy link
Contributor Author

zeenix commented Nov 21, 2022

I only care about a to fast moving MSRV.

As we have it checked in the CI now in zbus, accidental bumps hopefully would never happen anymore and I don't intend to bump to too new MSRV, no matter how appealing a new feature might be. :) I promise you that. :) I think a 6 months grace period is good enough in the Rust world but I'll still first check availability in Ubuntu LTS before bumping.

@Luap99
Copy link
Member

Luap99 commented Nov 21, 2022

Yes we should be good in the future, I look into why the quay.io auto builder did not rebuild the image so far.

In any case could you rebase this PR, we no longer use tokio for netlink and just make blocking calls.

@zeenix
Copy link
Contributor Author

zeenix commented Nov 21, 2022

In any case could you rebase this PR

Sure.

we no longer use tokio for netlink and just make blocking calls

Interesting.

@zeenix
Drop direct zvariant dep
a3c8156 
Use it through zbus to ensure it's always compatible with the zbus
version being used.

Signed-off-by: Zeeshan Ali <zeeshanak@gnome.org>
@zeenix
Port to zbus 3.5
84a0ddd 
3.x is the latest stable and supported release cycle.

Signed-off-by: Zeeshan Ali <zeeshanak@gnome.org>
@zeenix
Make use of zbus::blocking wrappers
c858e2f 
Instead of wrapping async calls in block_on manually. This also fixes
the issue of zbus using tokio API underneath and failing because of
these APIs assuming a tokio runtime context.

Signed-off-by: Zeeshan Ali <zeeshanak@gnome.org>
@zeenix
Drop now unneeded futures dep
84b9078 
This also drops a few subcrate deps.

Signed-off-by: Zeeshan Ali Khan <zeeshanak@gnome.org>
@zeenix
Copy link
Contributor Author

zeenix commented Nov 21, 2022

In any case could you rebase this PR, we no longer use tokio for netlink and just make blocking calls.

Done. I'm afraid porting to 3 doesn't provide any other benefits than just using the latest and supported release cycle. The commit that switches to use of zbus' blocking wrapper does allow dropping some deps though.

Luap99
Luap99 approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@mheon
Copy link
Member

mheon commented Nov 21, 2022

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Nov 21, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 21, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, mheon, zeenix

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 65afcb3 into containers:main Nov 21, 2022
@zeenix
Copy link
Contributor Author

zeenix commented Nov 21, 2022

Yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luap99 Luap99 Luap99 approved these changes

Assignees

@mheon mheon

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@zeenix @flouthoc @mheon @Luap99 @openshift-merge-robot