search

balena-io-modules / network-manager

Rust NetworkManager bindings
Apache License 2.0
37 stars 30 forks source link

Added wifi scan API #115

Closed spiderkeys closed 6 years ago

spiderkeys commented 6 years ago

Just needed the functionality for a project I'm working on. Feel free to modify to your liking. Or not :)

The RequestScan API is not very well documented: https://developer.gnome.org/NetworkManager/stable/gdbus-org.freedesktop.NetworkManager.Device.Wireless.html#gdbus-method-org-freedesktop-NetworkManager-Device-Wireless.RequestScan

... so I did what everyone else who has written a wrapper for this command and passed in an empty option map. ---- Autogenerated Waffleboard Connection: Connects to #116

resin-io-modules-versionbot[bot] commented 6 years ago

@spiderkeys, status checks have failed for this PR. Please make appropriate changes and recommit.

spiderkeys commented 6 years ago

Also, let me know if there is a better way to submit contributions, re: your versionist requirements.

majorz commented 6 years ago

Thanks again for the new contribution! It definitely makes sense to expose RequestScan there.

As for VersionBot, please take a look at my last commit: https://github.com/resin-io-modules/network-manager/pull/114/commits/79b37ee671fa2c84ed046fa329fe04fcb17a4b24

The commit contains two footer fields in the commit message: Change-Type: patch and Connects-To: hashtag-issue-number. This one in particular will need a minor version bump, since we are adding a new feature. Change-Type: minor. Connects-To will need a new issue created as well.

Just for a bit more info: internally VersionBot uses versionist: https://github.com/resin-io/versionist. And VersionBot's repo is https://github.com/resin-io-modules/resin-procbots.

However I see that our CircleCI build is not triggered for some reason. I need to check with another team member why this is the case. There are some linting and formatting checks running on CircleCI, and those definitely need to happen before we merge it.

Let's change the new method from scan to request_scan to be more close to the original method.

You may undo the commit on the same branch and create a new one with a new commit message. Let's also change the main part of the commit message to something like: "Expose RequestScan through WiFiDevice::request_scan" or something similar.

BTW, I am exploring the option of converting this crate to use libnm instead of directly communicating with NetworkManager through the D-Bus API. There is a Rust generator that can create automatically bindings to it: https://github.com/gtk-rs/gir. libnm is the official NetworkManager client library and it handles all the D-Bus stuff internally. I still have not looked very closely at this, but it looks quite promising.

majorz commented 6 years ago

Alright, we enabled the CircleCi builds. I think once you recommit, they will be triggered. The important one with the linting and formatting checks is ci/circleci: test.

It turns out I will still have to make a second PR, as VersionBot still does not support fully user-contributed PRs. Not a big issue for me though.

spiderkeys commented 6 years ago

@majorz I've made the requested changes, and think I did the versioning correctly, but let me know if anything else needs fixing. For some reason, some of the CI steps aren't progressing, though. Maybe that's what you meant by needing to recreate the PR, which is fine.

As far as using libnm goes, that sounds like it could be a great idea; would certainly reduce the scope of responsibilities for this library. I'm very new to Rust, so I can't comment much on the aspects relating to wrapping a third party's native library. From what I understand though, Rust makes it fairly straightforward to interface with C, so that probably wouldn't be too much of a concern.

majorz commented 6 years ago

@spiderkeys thanks a lot for the changes. I will test it and merge it early next week when I get back from vacation. No problem for the CI, I will have to recreate the PR anyway.

For libnm it will probably be even easier to make the bindings, since the Gir tool will generate automatically Rust code for them. I tried it a couple of weeks ago again and saw how it works, but never tried to run the bindings, as I had to do some manual work to get it all running. Hopefully I will have more time for this soon, as it is not only interesting, but it will provide full coverage of what NetworkManager provides. I will probably still have to add some Rust code on top of it for convenience, but let's see how that goes.

majorz commented 6 years ago

Closing the PR in favor of #117, which is the same code.