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

Update inotify to 0.10 #547

Merged
merged 5 commits into from
Jan 4, 2024
Merged

Update inotify to 0.10 #547

merged 5 commits into from
Jan 4, 2024

Conversation

fornwall
Copy link
Contributor

See the CHANGELOG.

@fornwall
Copy link
Contributor Author

Hi @0xpr03, could you have a look at this PR?

@0xpr03
Copy link
Member

0xpr03 commented Nov 28, 2023

Is there a strong reason to push for 0.10 ASAP ? I don't see any real reasons in their changelog.

@0xpr03
Copy link
Member

0xpr03 commented Nov 28, 2023

In any case I'd like to land #549 first as you both interact with the CI and your changes are easier to rebase on top of that. The MSRV break will also make this part of #525.

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.

Blocked by #549.
And we might want to look into re-using watches() for hot loops.

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

0xpr03 commented Nov 28, 2023

Thanks, I've added one comment regarding the watches() overhead.

@fornwall
Copy link
Contributor Author

Is there a strong reason to push for 0.10 ASAP ? I don't see any real reasons in their changelog.

The main driver here is to avoid duplication - other crates in the ecosystem are using 0.10 (for instance https://gitlab.com/gilrs-project/gilrs), and there is a desire (or rule) in many projects to avoid pulling in multiple versions of the same crate.

So in general, unless there is a good reason, I think the latest released version of a crate should be used.

@fornwall
Copy link
Contributor Author

fornwall commented Nov 29, 2023

Tested this a bit, and it seems we might need to adopt to: hannobraun/inotify-rs#190. EDIT: Done in 9f0c4d5

@fornwall
Copy link
Contributor Author

Thanks, I've added one comment regarding the watches() overhead.

Thank you for the review! Addressed that one in b70b72b

@fornwall fornwall requested a review from 0xpr03 December 4, 2023 20:53
@MarijnS95
Copy link

The main driver here is to avoid duplication - other crates in the ecosystem are using 0.10 (for instance https://gitlab.com/gilrs-project/gilrs), and there is a desire (or rule) in many projects to avoid pulling in multiple versions of the same crate.

Yeah, same for us, we'd love to see both crates in sync :)

@0xpr03
Copy link
Member

0xpr03 commented Jan 4, 2024

Thanks, I got to merging the blocker and fixed one small readme formatting error while rebasing for (valid) CI results.

@0xpr03 0xpr03 merged commit 6c1798d into notify-rs:main Jan 4, 2024
13 checks passed
@0xpr03
Copy link
Member

0xpr03 commented Jan 4, 2024

Now let's see, there is some hope I can get around to a new release this weekend, but no promises.

@fornwall fornwall deleted the inotify-0.10 branch January 19, 2024 22:37
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.

3 participants