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

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

Merged
merged 6 commits into from
Oct 6, 2023

Conversation

ssnover
Copy link
Contributor

@ssnover ssnover commented Sep 13, 2023

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: windy1/zeroconf-rs#31

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

This should resolve #68

@ivmarkov
Copy link
Contributor

CI is failing though. @ssnover can you fix?

@ssnover
Copy link
Contributor Author

ssnover commented Sep 15, 2023

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
Copy link
Contributor

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 ssnover marked this pull request as draft September 17, 2023 17:24
@ssnover
Copy link
Contributor Author

ssnover commented Sep 17, 2023

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 ssnover marked this pull request as ready for review September 25, 2023 01:45
@ssnover
Copy link
Contributor Author

ssnover commented Sep 25, 2023

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
Copy link
Contributor

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

@ssnover
Copy link
Contributor Author

ssnover commented Sep 25, 2023

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

@ivmarkov
Copy link
Contributor

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

@ssnover ssnover requested a review from kedars October 1, 2023 19:26
@ssnover
Copy link
Contributor Author

ssnover commented Oct 1, 2023

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.

@ivmarkov ivmarkov mentioned this pull request Oct 6, 2023
@kedars kedars merged commit 97c9619 into project-chip:main Oct 6, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update README.md with info that Avahi daemon and Chrome on Linux should be stopped
3 participants