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

Kqueue backend for use on BSD #335

Merged
merged 26 commits into from Jun 26, 2021
Merged

Kqueue backend for use on BSD #335

merged 26 commits into from Jun 26, 2021

Conversation

xanderio
Copy link
Contributor

@xanderio xanderio commented Jun 9, 2021

Kqueue Backend for all the BSDs 🎉

It's based on the kqueue crate and the in watcher implementation is based on the INotifyWatcher.

BSDs where this PR is check that it compiles:

  • FreeBSD
  • OpenBSD
  • NetBSD
  • DragonflyBSD

This relates to #327 and maybe #136

@0xpr03
Copy link
Member

0xpr03 commented Jun 10, 2021

I enabled the CI for this PR so if possible you can setup & test github CI for freebsd

@xanderio
Copy link
Contributor Author

Thanks, but Github Actions don't have support for FreeBSD.

I found this action1, but i'm not sure if this is a greate solution for that problem.

@xanderio
Copy link
Contributor Author

We could setup a ci pipeline under MacOS in combination with a feature to select between FSEvent and kqueue.

@JohnTitor
Copy link
Member

FWIW, the libc crate and Tokio use Cirrus CI (e.g. https://github.com/tokio-rs/tokio/blob/master/.cirrus.yml) for FreeBSD testing though it has some limitations for free users.

@xanderio
Copy link
Contributor Author

Which solution should i implement?

  1. Cargo feature to toggle the mac backend to kqueue
  2. Cirrus CI on FreeBSD

@xanderio xanderio marked this pull request as ready for review June 14, 2021 20:06
@xanderio
Copy link
Contributor Author

The only thing the I'm currently seeing that is still missing is some kind of CI solution.

@JohnTitor
Copy link
Member

I don't have a strong opinion here but is the first option beneficial for mac users as well? If so, it's better than the second as we don't need additional CI.

@0xpr03
Copy link
Member

0xpr03 commented Jun 15, 2021

I think we want some freebsd CI if possible. I'm fine with testing this only on mac if nothing else, but to have official BSD support it'd be very nice to have a CI run. Otherwise you have a very high chance it'll break and nobody will notice (until the release is out). @JohnTitor can you help setup cirrus CI for that if possible ?

@JohnTitor
Copy link
Member

Otherwise you have a very high chance it'll break and nobody will notice.

AFAIK macOS is one of BSD targets and it should have kqueue API. Does this have FreeBSD-specific operations?

I'm happy to help setup Cirrus CI though :)

@worr
Copy link

worr commented Jun 15, 2021

Hey, original author of the kqueue crate here.

This doesn't have anything FreeBSD-specific, since as of right now, the high-level parts of the kqueue are all cross-platform. I've looked at the NOTEs and stuff they're using and ensured that they're also cross-platform. In other words, LGTM.

While macOS does support kqueue(2) and theoretically this could be used there, not sure what cases someone would want to use kqueue(2) rather than FSEvents which is more native, and afaict already supported by this crate. A feature like @xanderio suggested might be a good way to gate it, for either easier CI or in case someone v specifically wants to use kqueue(2). That's just my 2¢ tho, feel free to disregard 🙂

Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I've added some minor questions or notes what could be improved. If possible I'd like to resolve the todos. I think we can also just track some stuff if we're already re-indexing the whole path upon link change.

I don't have a hard opinion on the macos stuff, but if we're already missing out on a CI for freebsd I'd like to test this via macos one way or the other.

src/kqueue.rs Outdated Show resolved Hide resolved
src/kqueue.rs Show resolved Hide resolved
src/kqueue.rs Outdated Show resolved Hide resolved
src/kqueue.rs Outdated Show resolved Hide resolved
src/kqueue.rs Outdated Show resolved Hide resolved
src/kqueue.rs Outdated Show resolved Hide resolved
src/kqueue.rs Outdated Show resolved Hide resolved
src/kqueue.rs Show resolved Hide resolved
@0xpr03
Copy link
Member

0xpr03 commented Jun 16, 2021

Ping me again here if you're not hearing anything from me, I just discovered your comments on the review by accident and didn't get any notification. (Also the github CI run apparently has to be re-approved every time again, as long as you haven't made at least one commit into the repo.)

@xanderio
Copy link
Contributor Author

Quick side note: After this is merged could you release a new preview build?

@0xpr03
Copy link
Member

0xpr03 commented Jun 16, 2021

Yeah, I'll probably merge #337 first and then release them together when I cleaned up the changelog, some stuff got left out.

@xanderio
Copy link
Contributor Author

After we fix the could of code review issues with this pr. The only thing that is missing is some kind of CI setup.
I would start working on the macos based solution previously discussed, if i don't here anything else from you.

@0xpr03
Copy link
Member

0xpr03 commented Jun 16, 2021

I'm sorry but I'm not exactly sure what you mean with that. If you want you can go ahead and make a macOS PR.

@xanderio
Copy link
Contributor Author

The latest commit include a CI pipeline, as this is the first time i ever did something with github actions, I'm not sure if this is correct.

Other then the unwrap issue on line 343 and 359. This PR is in my eyes almost complete. 🎉

@JohnTitor
Copy link
Member

Yeah that sounds like a good idea. @JohnTitor do you prefer doing this via cirrus or just a cargo feature ?

I don't have a strong opinion but it's worth trying out the latter if we've implemented it.

@xanderio
Copy link
Contributor Author

Just pushed to last commit, which should ™️ fix all code review issus

@xanderio
Copy link
Contributor Author

Thanks for fixing the CI @JohnTitor

@0xpr03 According to github you're still requesting changes, but i can't figure out what changes those are. :/

@0xpr03
Copy link
Member

0xpr03 commented Jun 20, 2021

I think that was simply because I had a change-request review with a general comment which probably isn't resolved by the individual code comments.

@xanderio
Copy link
Contributor Author

I'm wondering if there is something that this PR is waiting on or if you just haven't come around to merge it?

@0xpr03 0xpr03 merged commit 57daf85 into notify-rs:main Jun 26, 2021
@0xpr03
Copy link
Member

0xpr03 commented Jun 26, 2021

Thanks, was busy last week and githubs UI made it looks like merging this isn't possible without manual merge conflict intervention.

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.

None yet

4 participants