Bump notify from 4.0.17 to 5.0.0

[dependabot]

Bumps notify from 4.0.17 to 5.0.0.

Release notes

Sourced from notify's releases.

notify 5.0.0

5.0.0 (2022-08-28)

For a list of changes when upgrading from v4 see https://github.com/notify-rs/notify/blob/HEAD/UPGRADING_V4_TO_V5.md.

Differences to 5.0.0-pre.16:

• FIX: update minimum walkdir version to 2.2.2 #[432]
• CHANGE: add need_rescan function to Event, allowing easier detection when a rescan is required #435
• FIX: debouncer-mini: change crossbeam feature to crossbeam, to allow passthrough with notify re-exports #429
• DOCS: improve v5-to-v5 upgrade docs #431
• DOCS: file back v4 changelog into main #437
• DOCS: cleanups and link fixes

#431: notify-rs/notify#431 #432: notify-rs/notify#432 #437: notify-rs/notify#437 #435: notify-rs/notify#435 #429: notify-rs/notify#429

Changelog

Sourced from notify's changelog.

notify 5.0.0 (2022-08-28)

For a list of changes when upgrading from v4 see https://github.com/notify-rs/notify/blob/main/UPGRADING_V4_TO_V5.md.

Differences to 5.0.0-pre.16:

• FIX: update minimum walkdir version to 2.2.2 #[432]
• CHANGE: add need_rescan function to Event, allowing easier detection when a rescan is required #435
• FIX: debouncer-mini: change crossbeam feature to crossbeam, to allow passthrough with notify re-exports #429
• DOCS: improve v5-to-v5 upgrade docs #431
• DOCS: file back v4 changelog into main #437
• DOCS: cleanups and link fixes

#431: notify-rs/notify#431 #432: notify-rs/notify#432 #437: notify-rs/notify#437 #435: notify-rs/notify#435 #429: notify-rs/notify#429

5.0.0-pre.16 (2022-08-12)

• CHANGE: require config for watcher creation and unify config #426
• CHANGE: fsevent: use RenameMode::Any for renaming events #371
• FEATURE: re-add debouncer as new crate and fixup CI #286
• FEATURE: allow disabling crossbeam-channel dependency #425
• FIX: PollWatcher panic after delete-and-recreate #406
• MISC: rework pollwatcher internally #409
• DOCS: cleanup all docs towards v5 #395

#395: notify-rs/notify#395 #406: notify-rs/notify#406 #409: notify-rs/notify#409 #425: notify-rs/notify#425 #286: notify-rs/notify#286 #426: notify-rs/notify#426 #371: notify-rs/notify#371

5.0.0-pre.15 (2022-04-30)

• CHANGE: raise MSRV to 1.56! #396 and #402
• FEATURE: add support for pseudo filesystems like sysfs/procfs #396
• FIX: Fix builds on (Free)BSD due to changes in kqueue fix release #399

#396: notify-rs/notify#396 #399: notify-rs/notify#399 #402: notify-rs/notify#402

5.0.0-pre.14 (2022-03-13)

• CHANGE: upgrade mio to 0.8 #386

... (truncated)

Commits

• d985ae1 prepare 5.0.0
• a83279f improve upgrade docs and fix Config link
• 65da37b file back v4 history into changelog
• 1fbf8fa add accessor for whether rescan is required on Event
• 17580f6 fixup rebase gunk
• 54465e9 fixup optional crossbeam feature selection in debouncer-mini
• 94f1680 update minimum walkdir version to 2.2.2 (#432)
• 4d16a54 fixup doc links post initial debouncer-mini release
• d698f90 fixup moved readme due to reorg
• 2f91e15 fix typo in readme
• Additional commits viewable in compare view

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

---

Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

• @dependabot rebase will rebase this PR
• @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
• @dependabot merge will merge this PR after your CI passes on it
• @dependabot squash and merge will squash and merge this PR after your CI passes on it
• @dependabot cancel merge will cancel a previously requested merge and block automerging
• @dependabot reopen will reopen this PR if it is closed
• @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
• @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
• @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
• @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

[netlify]

👷 Deploy Preview for chef-habitat processing.

Name	Link
🔨 Latest commit	1502382
🔍 Latest deploy log	https://app.netlify.com/sites/chef-habitat/deploys/63ea45869fa2ff0008169525

[chef-expeditor]

Hello dependabot[bot]! Thanks for the pull request!

Here is what will happen next:

1. Your PR will be reviewed by the maintainers.
2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

[jasonheath]

Fair point. I’ve been delivered not one but two thanksgiving turkeys this afternoon. This likely explains a mystery bug I’ve been hitting in my testing today and finally had to dig into. https://github.com/Stebalien/tempfile/blob/1a40687e06eb656044e3d2dffa1379f04b3ef3fd/src/lib.rs#L30 This is something I’ll need to account for as I work through responding to the different events. notify-rs/notify#166 I don’t think either is terrible, especially given that they’re known now. Will incorporate the feed back below and push a point in time check in of my changes. Need to start heading towards my mom’s place. From: Matt Wrock ***@***.***> Date: Tuesday, November 22, 2022 at 4:19 PM To: habitat-sh/habitat ***@***.***> Cc: Jason Heath ***@***.***>, Assign ***@***.***> Subject: Re: [habitat-sh/habitat] Bump notify from 4.0.17 to 5.0.0 (PR #8615) @mwrock commented on this pull request.

________________________________ In components/sup/src/manager/file_watcher.rs<#8615 (comment)>:

- #[derive(Clone, Debug, PartialEq)] - struct NotifyEvent { - path: PathBuf, - kind: NotifyEventKind, + // Logging the fact that a particular path was ignored. Ignoring a given + // path may be the correct thing to do so the purpose of logging that a + // particular path was ignored is to highlight that the path was ignored in + // case the decision made today to ignore the event needs to be changed + // due to design changes, lessons learned (bugs) or notify crate updates. + fn log_ignored_path(&mut self, notify_event: &Event, path_buf: &PathBuf) { + info!("IGNORING {:?} of {:?}", path_buf, notify_event) this is probably something you want to log at TRACE level. INFO would be way too noisy. — Reply to this email directly, view it on GitHub<#8615 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAAHD6CKX7STWJQVPYWLUNTWJU2EFANCNFSM6AAAAAAQA73KFE>. You are receiving this because you were assigned.Message ID: ***@***.***>

[mwrock]

I'm not done but I'm going to send these along for now.

[mwrock]

In addition to these comment there are a few general comments:

• The code comments seem somewhat excessive. I definitely appreciate assistance in understanding what is happening, but these seem super verbose.
• The debug logging also seems a bit excessive. It may just be that you need this for the nitial development but you may want to reevaluate if you need all of this.

[mwrock]

I have not looked over the tests yet but thought I would get this review batch out now so you could start looking sooner than later if you wanted.

[dependabot]

A newer version of notify exists, but since this PR has been edited by someone other than Dependabot I haven't updated it. You'll get a PR for the updated version as normal once this PR is merged.

[jasonheath]

Here's the changelog for 5.1.0.

notify 5.1.0 (2023-01-15)

CHANGE: switch from winapi to windows-sys https://github.com/notify-rs/notify/pull/457
FIX: kqueue-backend: batch file-watching together to improve performance https://github.com/notify-rs/notify/pull/454
DOCS: include license file in crate again https://github.com/notify-rs/notify/pull/461
DOCS: typo and examples fixups

How would you like to handle? For us, its really all about that change on the windows crate as since we don't use kqueue. I can argue for and against both finishing the upgrade work and bumping later vs bumping to 5.1.0 now. Given the nature of the change I think I would break towards upgrading now but you have a much better perspective on what that change will mean for windows.

[mwrock]

How would you like to handle? For us, its really all about that change on the windows crate as since we don't use kqueue. I can argue for and against both finishing the upgrade work and bumping later vs bumping to 5.1.0 now. Given the nature of the change I think I would break towards upgrading now but you have a much better perspective on what that change will mean for windows.

I would go ahead and bump and if there is no refactoring needed and the tests pass then we can just take it. Otherwise we will probably want to do separately unless it is something obvious and trivial.

[jasonheath]

How would you like to handle? For us, its really all about that change on the windows crate as since we don't use kqueue. I can argue for and against both finishing the upgrade work and bumping later vs bumping to 5.1.0 now. Given the nature of the change I think I would break towards upgrading now but you have a much better perspective on what that change will mean for windows.

I would go ahead and bump and if there is no refactoring needed and the tests pass then we can just take it. Otherwise we will probably want to do separately unless it is something obvious and trivial.

Bumped and pushed. Made sure that only the notify dependency changed. Verified that cargo test --package habitat_sup --lib is passing on windows and linux

[mwrock]

Still making my way through the tests but wanted to drop this one comment now.

There are a lot of tests and parts of tests that are relying on knowledge of the private API and implementation of the file watcher. For instance a lot of checking on the watched file state and other tests calling methods like preclude_directories and the ends_with functions. I think we really just want to have tests that call the public API: new, run, single_iteration and validate that certain events were called or not called.

I think you are already testing that the events are being called and I see pretty thorough testing on the error conditions of new. With that I think a lot of the other tests are repetitive and can be removed.

[mwrock]

It looks like Path does not transform a / to a \ and therefore you would need to check for both on windows since / is a legal substitute on windows. However, thinking more about this, I am inclined to eliminate the folder checking validation alltogether. There are only two users of the file watcher. First is user_config_watcher.rs which will only ever watch the user.toml file. The second user is the peer watcher. Users can pass in whatever path on the cli and therefore it is possible to pass a folder name. However, the current implementation has no validation for this. It would just result in nothing ever happening.

[jasonheath]

It looks like Path does not transform a / to a \ and therefore you would need to check for both on windows since / is a legal substitute on windows. However, thinking more about this, I am inclined to eliminate the folder checking validation alltogether. There are only two users of the file watcher. First is user_config_watcher.rs which will only ever watch the user.toml file. The second user is the peer watcher. Users can pass in whatever path on the cli and therefore it is possible to pass a folder name. However, the current implementation has no validation for this. It would just result in nothing ever happening.

The latest version of the code does this. (Just updating to keep track where we are)

[jasonheath]

Still making my way through the tests but wanted to drop this one comment now.

There are a lot of tests and parts of tests that are relying on knowledge of the private API and implementation of the file watcher. For instance a lot of checking on the watched file state and other tests calling methods like preclude_directories and the ends_with functions. I think we really just want to have tests that call the public API: new, run, single_iteration and validate that certain events were called or not called.

I think you are already testing that the events are being called and I see pretty thorough testing on the error conditions of new. With that I think a lot of the other tests are repetitive and can be removed.

Most recent version of the code should address all of this. (Just updating to keep track where we are)

[mwrock]

still going through this but here is what I have now. Also there are a lot of comments referencing v5.0.0 specifically. Eventually we will be on a higher version and these references may become confusing.

[mwrock]

In addition to these comments, I noticed that the test methods return a result. Typically, a test function returns nothing which eliminates the need for the empty Ok(()) at the end. It's not a big deal and you can leave it but thought you should know.

[mwrock]

Looks good! Just one super minor comment. And also looks like you need to address some clippy/linting stuff.

[mwrock]

Looks good pending rebase and merge. So much cleaner without the notify crate!