-
Notifications
You must be signed in to change notification settings - Fork 229
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
Conversation
I enabled the CI for this PR so if possible you can setup & test github CI for freebsd |
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. |
We could setup a ci pipeline under MacOS in combination with a feature to select between FSEvent and kqueue. |
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. |
Which solution should i implement?
|
The only thing the I'm currently seeing that is still missing is some kind of CI solution. |
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. |
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 ? |
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 :) |
Hey, original author of the 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 |
There was a problem hiding this 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.
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.) |
Quick side note: After this is merged could you release a new preview build? |
Yeah, I'll probably merge #337 first and then release them together when I cleaned up the changelog, some stuff got left out. |
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'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. |
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. 🎉 |
I don't have a strong opinion but it's worth trying out the latter if we've implemented it. |
Just pushed to last commit, which should ™️ fix all code review issus |
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. :/ |
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. |
I'm wondering if there is something that this PR is waiting on or if you just haven't come around to merge it? |
Thanks, was busy last week and githubs UI made it looks like merging this isn't possible without manual merge conflict intervention. |
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:
This relates to #327 and maybe #136