Prevent installing crossbeam-channel with default-features="false"#550
Conversation
9a8176a to
1d02f8b
Compare
|
Thanks for running the workflow! I have adjusted the pull request with the following:
|
Debouncers used to install `crossbeam-channel`, due to keeping `notify`'s default features enabled even if their own default features were disabled. With this change, `crossbeam-channel` will no longer be installed if `notify-debouncer-*` as well as `notify` are installed with `default-features = false` Fixes notify-rs#549
1d02f8b to
ab0c6fd
Compare
|
Workflow run 643 can be cancelled. See intermediate version missing a job condition predicate. Workflow run 644 can be run. |
| default = ["crossbeam","macos_fsevent"] | ||
| # can't use dep:crossbeam-channel and feature name crossbeam-channel below rust 1.60 | ||
| crossbeam = ["crossbeam-channel","notify/crossbeam-channel"] | ||
| macos_fsevent = ["notify/macos_fsevent"] |
There was a problem hiding this comment.
Is there a reason for masquerading the notify features? We simply let user specify them in their own cargo toml until now.
There was a problem hiding this comment.
In the past, debouncers used notify with default features, including crossbeam-channel and macos_fsevents -- even if default features were disabled by the user on both the debouncer and the notify direct dependency.
Users were effectively unable to disable crossbeam-channel on notify when using a debouncer.
This change sets the debouncers' notify dependency to default-features = false, to allow users to even disable them at all.
However, leaving it at that would break compatibility for MacOS users, as macos_fsevent would no longer be enabled by default.
Therefore, this change to the debouncers' feature set makes notify's default features their own, to keep the existing behavior, where debouncers with default features include notify with default features.
Some details - skip if you like:
The reasoning has two parts: 1. Not breaking MacOS use by enabling macos_fsevent by default, as it used to be, and 2. convenience of pass through for ease of configuration for library users.
-
I made this change so that a missing
macos_fseventdoes not break MacOS use wherenotifydefault features used to be enabled when using a debouncer:
The debouncers now disablenotify's default features to fix the described issue.
However, this causesmacos_fseventsto be disabled by default. For compatibility, it needs to be kept enabled.
Therefore, the debouncers need to includemacos_fseventin their default features.
As a library user, I would wantnotifydefault features (crossbeam-channel+macos_fsevent) to work as they did before, but be able to disable them. -
I made this change so that
macos_fseventandmacos_kqueuecan make use of the convenient feature pass-through which had previously been available only forcrossbeam-channel:
If a MacOS user depends on one of the debouncers, they can define e.g.notify-debouncer-full = { version = "*", default-features = false, features = ["macos_kqueue"] }without having to addnotifyas a direct dependency.
The debouncers' and the underlyingnotify's features must match (seecrossbeamfeature). The easiest way to make them match is to make users define them only on the debouncers.
If themacos_fseventpass-through is removed (Breaking!), then probably themacos_kqueueandcrossbeam(Breaking!) pass-through features should be removed as well.
To my mind, it seems intransparent and error prone to require users to add a second explicit dependency just to enable some feature flags. As a library user, I would prefer to have all three pass-throughs available.
|
This will be part of #525 |
|
Hi @0xpr03 Thanks for your review! I have added an answer to your question, and in turn asked a clarifying question about your comment. |
Debouncers used to install
crossbeam-channel,due to keeping
notify's default features enabledeven if their own default features were disabled.
With this change,
crossbeam-channelwillno longer be installed if
notify-debouncer-*as well as
notifyare installed withdefault-features = falseFixes #549