search

project-chip / rs-matter

Rust implementation of the Matter protocol. Status: Experimental
Apache License 2.0
329 stars 45 forks source link

Add feature zeroconf in order to support avahi mDNS registration via zeroconf crate #94

Closed ssnover closed 1 year ago

ssnover commented 1 year ago

Add feature zeroconf in order to support avahi mDNS registration via zeroconf crate.

Right now it is pointed at a local branch I have while I wait for the upstream PR to get merged: https://github.com/windy1/zeroconf-rs/pull/31

I've tested and verified this code is functional however, so it should be ready for review!

This should resolve #68

ivmarkov commented 1 year ago

CI is failing though. @ssnover can you fix?

ssnover commented 1 year ago

CI is failing though. @ssnover can you fix?

It will continue to fail until I get the upstream MR fixed, then I'll update this repo to point at that one. Is it okay to use a git-based dependency so that this isn't blocked waiting for a release of zeroconf?

ivmarkov commented 1 year ago

CI is failing though. @ssnover can you fix?

It will continue to fail until I get the upstream MR fixed, then I'll update this repo to point at that one. Is it okay to use a git-based dependency so that this isn't blocked waiting for a release of zeroconf?

Git dependency is better than what you have right now (local path) because the CI will at least get a chance to run. It is another story that - because the zeroconf feature is not enabled by default, Clippy will not check it, so you might want to run Clippy on your machine separately, with this feature enabled.

By the way, the CI fails already at cargo fmt so you might want to address that as well.

Now, until the upstream issue is fixed, this PR cannot be merged or else we'll end up with a GIT-only dependency, which (I think) will not allow us to publish on crates.io. So we need the upstream fix. Or - alternatively - if the upstream fix is not big and not an API change, you can depend on the released version of the upstream crate, and then - for a while - document for users that they need to have it patched using [patch.crates-io] in their own Cargo.toml. That's assuming that your fix will eventually get merged, of course.

ssnover commented 1 year ago

Sure, I've updated the MR to point at my git fork for now so that it can build in CI but also converted to draft so as not to merge by accident. If y'all are on Linux with avahi it should be testable with chip-tool.

ssnover commented 1 year ago

I pulled in the change for #98 in order to pass the build, it looks like they're not planning to sign the CLA.

ivmarkov commented 1 year ago

@ssnover Were your changes merged upstream? The PR is not (any longer?) in Draft?

ssnover commented 1 year ago

Yes, changes merged into a new version 0.12 of zeroconf that was published on crates.io.

ivmarkov commented 1 year ago

@kedars I've not tested the zeroconf mDNS (yet), but given that it is behind a feature flag, we should probably merge.

ssnover commented 1 year ago

One other thing I'd note is that with additional testing it should be possible for this implementation to replace the MacOS mDNS client as well as zeroconf is supposed to be able to support both. I cannot test that though and I think it's lower impact to initially merge for Linux to get testing via users of the main branch.