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

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

Conversation

LeoniePhiline
Copy link
Contributor

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 #549

@LeoniePhiline LeoniePhiline force-pushed the fix/debouncer-always-installs-crossbeam-channel branch from 9a8176a to 1d02f8b Compare November 23, 2023 11:20
@LeoniePhiline
Copy link
Contributor Author

LeoniePhiline commented Nov 23, 2023

Thanks for running the workflow!

I have adjusted the pull request with the following:

  • Allow both macos_fsevent and macos_kqueue features to be forwarded from debouncer crates to their notify dependency.
  • Forward macos_fsevent to notify by default, as is notify's default feature set.
  • Adjust the README of both debouncers to mention enabling a backend of choice on MacOS if default features are disabled.
  • Adjust notify-debouncer-mini's README formatting to match notify-debouncer-full's README.
  • Adjust the github workflow to check debouncers with both MacOS backends forwarded as feature flags to notify.
  • Align github workflow job names in regards to disabled features. (Align debouncer feature check job names with notify feature check job names.)

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
@LeoniePhiline LeoniePhiline force-pushed the fix/debouncer-always-installs-crossbeam-channel branch from 1d02f8b to ab0c6fd Compare November 23, 2023 11:28
@LeoniePhiline
Copy link
Contributor Author

Workflow run 643 can be cancelled. See intermediate version missing a job condition predicate.

Workflow run 644 can be run.

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.

Looks good overall. I've left some comments.

In any case thanks for the work, especially regarding streamlining docs.

.github/workflows/main.yml Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
# 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

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.

@0xpr03
Copy link
Member

0xpr03 commented Nov 28, 2023

This will be part of #525

@LeoniePhiline
Copy link
Contributor Author

Hi @0xpr03

Thanks for your review!

I have added an answer to your question, and in turn asked a clarifying question about your comment.

@0xpr03 0xpr03 merged commit 627f9aa into notify-rs:main Jan 4, 2024
13 checks passed
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.

notify-debouncer-full / -mini always enable notify default features
2 participants