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.

Sign up for GitHub

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

Jump to bottom

Prevent installing crossbeam-channel with default-features="false" #550

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Prevent installing crossbeam-channel with default-features="false" #550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,24 @@ jobs:
run: cargo check -p notify --no-default-features --features=macos_kqueue
# -p notify required for feature selection!

- name: check notify debouncer mini features
- name: check notify debouncer mini without crossbeam/default features
if: matrix.version == 'stable'
run: cargo check -p notify-debouncer-mini --no-default-features
run: cargo check -p notify-debouncer-mini --no-default-features --features=macos_fsevent
# -p required for feature selection to actually work!

- name: check notify debouncer full features
- name: check notify debouncer mini without crossbeam/default features on macos with kqueue
if: matrix.version == 'stable' && matrix.os == 'macos-latest'
0xpr03 marked this conversation as resolved.
Show resolved Hide resolved
run: cargo check -p notify-debouncer-mini --no-default-features --features=macos_kqueue
# -p required for feature selection to actually work!

- name: check notify debouncer full without crossbeam/default features
if: matrix.version == 'stable'
run: cargo check -p notify-debouncer-full --no-default-features
run: cargo check -p notify-debouncer-full --no-default-features --features=macos_fsevent
# -p required for feature selection to actually work!

- name: check notify debouncer full without crossbeam/default features on macos with kqueue
if: matrix.version == 'stable' && matrix.os == 'macos-latest'
0xpr03 marked this conversation as resolved.
Show resolved Hide resolved
run: cargo check -p notify-debouncer-full --no-default-features --features=macos_kqueue
# -p required for feature selection to actually work!

- name: check build examples
Expand Down
6 changes: 4 additions & 2 deletions notify-debouncer-full/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ name = "notify_debouncer_full"
path = "src/lib.rs"

[features]
default = ["crossbeam"]
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"]
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for masquerading the notify features? We simply let user specify them in their own cargo toml until now.

Copy link
Contributor Author

@LeoniePhiline LeoniePhiline Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

  1. I made this change so that a missing macos_fsevent does not break MacOS use where notify default features used to be enabled when using a debouncer:

    The debouncers now disable notify's default features to fix the described issue.

    However, this causes macos_fsevents to be disabled by default. For compatibility, it needs to be kept enabled.

    Therefore, the debouncers need to include macos_fsevent in their default features.

    As a library user, I would want notify default features (crossbeam-channel + macos_fsevent) to work as they did before, but be able to disable them.

  2. I made this change so that macos_fsevent and macos_kqueue can make use of the convenient feature pass-through which had previously been available only for crossbeam-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 add notify as a direct dependency.

    The debouncers' and the underlying notify's features must match (see crossbeam feature). The easiest way to make them match is to make users define them only on the debouncers.

    If the macos_fsevent pass-through is removed (Breaking!), then probably the macos_kqueue and crossbeam (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.

macos_kqueue = ["notify/macos_kqueue"]

[dependencies]
notify = { version = "6.1.1", path = "../notify" }
notify = { version = "6.1.1", path = "../notify", default-features = false }
crossbeam-channel = { version = "0.5", optional = true }
file-id = { version = "0.2.1", path = "../file-id" }
walkdir = "2.2.2"
Expand Down
13 changes: 12 additions & 1 deletion notify-debouncer-full/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,24 @@ A debouncer for [notify] that is optimized for ease of use.
- `crossbeam` enabled by default, for crossbeam channel support.

This may create problems used in tokio environments. See [#380](https://github.com/notify-rs/notify/issues/380).
Use someting like the following to disable it.
Use something like the following to disable it.

```toml
notify-debouncer-full = { version = "*", default-features = false }
```

This also passes through to notify as `crossbeam-channel` feature.

On MacOS, when disabling default features, enable either the `macos_fsevent` feature
or, on latest MacOS, the `macos_kqueue` feature to be passed through to notify.

```toml
# Using FSEvents
notify-debouncer-full = { version = "*", default-features = false, features = ["macos_fsevent"] }

# Using Kernel Queues
notify-debouncer-full = { version = "*", default-features = false, features = ["macos_kqueue"] }
```

[docs]: https://docs.rs/notify-debouncer-full
[notify]: https://crates.io/crates/notify
6 changes: 4 additions & 2 deletions notify-debouncer-mini/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ name = "notify_debouncer_mini"
path = "src/lib.rs"

[features]
default = ["crossbeam"]
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"]
macos_kqueue = ["notify/macos_kqueue"]

[dependencies]
notify = { version = "6.1.1", path = "../notify" }
notify = { version = "6.1.1", path = "../notify", default-features = false }
crossbeam-channel = { version = "0.5", optional = true }
serde = { version = "1.0.89", features = ["derive"], optional = true }
log = "0.4.17"
28 changes: 21 additions & 7 deletions notify-debouncer-mini/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,27 @@ Tiny debouncer for [notify]. Filters incoming events and emits only one event pe

## Features

- `crossbeam` enabled by default, for crossbeam channel support.
This may create problems used in tokio environments. See [#380](https://github.com/notify-rs/notify/issues/380).
Use something like the following to disable it.
```toml
notify-debouncer-mini = { version = "*", default-features = false }
```
This also passes through to notify as `crossbeam-channel` feature.
- `crossbeam` enabled by default, for crossbeam channel support.

This may create problems used in tokio environments. See [#380](https://github.com/notify-rs/notify/issues/380).
Use something like the following to disable it.

```toml
notify-debouncer-mini = { version = "*", default-features = false }
```

This also passes through to notify as `crossbeam-channel` feature.

On MacOS, when disabling default features, enable either the `macos_fsevent` feature
or, on latest MacOS, the `macos_kqueue` feature to be passed through to notify.

```toml
# Using FSEvents
notify-debouncer-mini = { version = "*", default-features = false, features = ["macos_fsevent"] }

# Using Kernel Queues
notify-debouncer-mini = { version = "*", default-features = false, features = ["macos_kqueue"] }
```
- `serde` for serde support of event types, off by default

[docs]: https://docs.rs/notify-debouncer-mini
Expand Down