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

Add a compile_error!() when central crate features clash #110

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

onkoe
Copy link
Contributor

@onkoe onkoe commented Sep 5, 2024

Fixes #94, fixes #108

This prevents users from accidentally using both features, as they are mutually exclusive.

Please let me know if you would like any changes! ✨

this prevents users from accidentally using both features, as they are mutually exclusive.

it also includes a fix to calm`rust-analyzer`, as it has an option to check all features at the same time.

i have that feature enabled in my editor by default, which caused around 100 errors and prevented "true" lints from showing in my IDE
in case the dependencies ever change, it's preferable to have user-facing features in here

this also defaults to using the `v4l2` module branch when both features are enabled, allowing for easier development when all features are being checked by the IDE.
this also has some minor formatting changes to abide by the CommonMark standard
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/v4l2/api.rs Outdated Show resolved Hide resolved
Comment on lines +80 to +81
#[cfg(all(feature = "v4l2", feature = "libv4l"))]
pub use v4l_sys;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not do what you've done in the second patch (but the other way around):

  1. Remove these two lines;
  2. Change the above to:
    -#[cfg(all(feature = "libv4l", not(feature = "v4l2")))]
    +#[cfg(feature = "v4l2")]
     pub use v4l2_sys as v4l_sys;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also prefer this, but I found that doing so breaks one of the features.

On the other hand, it doesn't occur in api.rs since we're not already re-exporting one of its modules. If you find a way to do it, though, please let me know!

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would match the choice in api.rs to prefer v4l2 code in the case that both features are contemporarily enabled, but it is less of an issue now that the comment has been removed.

Either way, this seems better to me:

diff --git a/src/lib.rs b/src/lib.rs
index 2272aeb..1237262 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -74,16 +74,12 @@ compile_error!(
     add `default-features = false` as a dependency key."
 );

-#[cfg(all(feature = "libv4l", not(feature = "v4l2")))]
-pub use v4l_sys;
-
-#[cfg(all(feature = "v4l2", not(feature = "libv4l")))]
-pub use v4l2_sys as v4l_sys;
-
-// calms down rust-analyzer when `rust-analyzer.cargo.features = "all"`,
-// though the `compile_error!()` above prevents this from being a valid,
+// calm down rust-analyzer when `rust-analyzer.cargo.features = "all"` by having only one `v4l_sys`
+// crate in scope, even though the `compile_error!()` above prevents this from being a valid,
 // compilable configuration.
-#[cfg(all(feature = "v4l2", feature = "libv4l"))]
+#[cfg(feature = "v4l2")]
+pub use v4l2_sys as v4l_sys;
+#[cfg(all(feature = "libv4l", not(feature = "v4l2")))]
 pub use v4l_sys;

 pub mod v4l2;

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
#[cfg(feature = "v4l-sys")]
#[cfg(all(feature = "libv4l", feature = "v4l2"))]
compile_error!(
"You may not enable both `v4l-sys` and `v4l2-sys` features at the same time.\
Copy link
Collaborator

Choose a reason for hiding this comment

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

You name these features but they're implicit from the optional dependencies, and "renamed" (but without "dep:...") to v4l2 and libv4l. I think we should specify exactly what the user is enabling, or expected ot enable:

Suggested change
"You may not enable both `v4l-sys` and `v4l2-sys` features at the same time.\
"You may not enable both `libv4l` and `v4l2` features at the same time.\

Comment on lines +80 to +81
#[cfg(all(feature = "v4l2", feature = "libv4l"))]
pub use v4l_sys;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would match the choice in api.rs to prefer v4l2 code in the case that both features are contemporarily enabled, but it is less of an issue now that the comment has been removed.

Either way, this seems better to me:

diff --git a/src/lib.rs b/src/lib.rs
index 2272aeb..1237262 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -74,16 +74,12 @@ compile_error!(
     add `default-features = false` as a dependency key."
 );

-#[cfg(all(feature = "libv4l", not(feature = "v4l2")))]
-pub use v4l_sys;
-
-#[cfg(all(feature = "v4l2", not(feature = "libv4l")))]
-pub use v4l2_sys as v4l_sys;
-
-// calms down rust-analyzer when `rust-analyzer.cargo.features = "all"`,
-// though the `compile_error!()` above prevents this from being a valid,
+// calm down rust-analyzer when `rust-analyzer.cargo.features = "all"` by having only one `v4l_sys`
+// crate in scope, even though the `compile_error!()` above prevents this from being a valid,
 // compilable configuration.
-#[cfg(all(feature = "v4l2", feature = "libv4l"))]
+#[cfg(feature = "v4l2")]
+pub use v4l2_sys as v4l_sys;
+#[cfg(all(feature = "libv4l", not(feature = "v4l2")))]
 pub use v4l_sys;

 pub mod v4l2;

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.

Unhelpful error message when compiling with default features off internal import issues with libv4l feature
2 participants