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

Support wsl. #9

Merged
merged 2 commits into from
Apr 13, 2021
Merged

Support wsl. #9

merged 2 commits into from
Apr 13, 2021

Conversation

dvc94ch
Copy link
Collaborator

@dvc94ch dvc94ch commented Apr 13, 2021

Closes #8

@dvc94ch dvc94ch requested a review from rklaehn April 13, 2021 10:28

impl IfWatcher {
/// Create a watcher
pub async fn new() -> Result<Self> {
Ok(Self(platform_impl::IfWatcher::new().await?))
if std::env::var_os("WSL_DISTRO_NAME").is_none() {
Ok(Self::Platform(platform_impl::IfWatcher::new().await?))

Choose a reason for hiding this comment

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

not sure where exactly this failed before, but does it make sense to try the fallback if this errored?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this might lead to missing bug reports, so I'd prefer not to do it

Copy link

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

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

So the check is done at runtime? Works for me.


impl IfWatcher {
/// Create a watcher
pub async fn new() -> Result<Self> {
Ok(Self(platform_impl::IfWatcher::new().await?))
if std::env::var_os("WSL_DISTRO_NAME").is_none() {
Copy link

Choose a reason for hiding this comment

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

I got a feeling that there will be more weird linux situations where the event based stuff does not work. So I am not really opposed to just trying and using the fallback on error.

But since so far we know of only this case, I am also fine with checking for it.

Let's revisit it once this becomes a longer list.

@@ -51,7 +51,7 @@ impl IfWatcher {
Ok(())
}

pub fn iter(&self) -> impl Iterator<Item = &IpNet> {
pub fn iter(&self) -> Iter<'_, IpNet> {
Copy link

Choose a reason for hiding this comment

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

What's the reason for this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because the fallback and platform_impl need to return the same iterator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

rust can't assume that it is the same iterator when using impl even if it is

@rklaehn
Copy link

rklaehn commented Apr 13, 2021

FYI:

not sure what the tests do, but they are green on the wsl branch, and red on master, on WSL2

@dvc94ch dvc94ch merged commit 9eb63c1 into master Apr 13, 2021
dvc94ch added a commit that referenced this pull request Apr 21, 2021
This reverts commit 9eb63c1.
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.

if-watch does not work on windows/WSL2
3 participants