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

RUST-1208 Future-proof features #1062

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Apr 3, 2024

RUST-1208 / RUST-1680 / RUST-1083

This PR:

  • removes the various bson-* features
  • adds the forward-compatibility feature
  • makes rustls and trust-dns optional


# Enable support for MONGODB-AWS authentication.
# This can only be used with the tokio-runtime feature flag.
aws-auth = ["reqwest"]
aws-auth = ["dep:reqwest"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the dep:foo format prevents the optional dependencies from automatically becoming their own feature flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice, I think the need to do this will go away in the 2024 edition rust-lang/cargo#12826

pub use resolver_config::ResolverConfig;
#[cfg(not(feature = "dns-resolver"))]
pub(crate) use resolver_config::ResolverConfig;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use of DNS resolution is ... not neatly encapsulated in the codebase, so to keep this from being a giant refactor PR I made the various machinery crate-internal when the feature is disabled rather than being feature-gated entirely.

bson-uuid-1 = ["bson/uuid-1"]
rustls-tls = ["dep:rustls", "dep:rustls-pemfile", "dep:tokio-rustls"]
openssl-tls = ["dep:openssl", "dep:openssl-probe", "dep:tokio-openssl"]
dns-resolver = ["dep:trust-dns-resolver", "dep:trust-dns-proto"]
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'm not sure whether giving this feature a generic name is the right way (treating the actual dependency as an internal detail) or if it should just explicitly be called "trust-dns-resolver" in case we want to add other resolver crates in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think I'm in favor of keeping it generic. I was curious what the status of trust-dns-resolver was and it looks like they've actually "rebranded" to a different crate (see here) that we might want to switch over to at some point. Keeping the name neutral will make it easier for us to swap out the internal dep.

Worst case, if we did want to introduce more resolver crates we could treat this flag as our "default" choice and then allow users to opt into a different one if they want to.

@abr-egn abr-egn marked this pull request as ready for review April 3, 2024 18:58
@abr-egn abr-egn requested a review from isabelatkinson April 3, 2024 18:58
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

lgtm!


# Enable support for MONGODB-AWS authentication.
# This can only be used with the tokio-runtime feature flag.
aws-auth = ["reqwest"]
aws-auth = ["dep:reqwest"]
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, I think the need to do this will go away in the 2024 edition rust-lang/cargo#12826

bson-uuid-1 = ["bson/uuid-1"]
rustls-tls = ["dep:rustls", "dep:rustls-pemfile", "dep:tokio-rustls"]
openssl-tls = ["dep:openssl", "dep:openssl-probe", "dep:tokio-openssl"]
dns-resolver = ["dep:trust-dns-resolver", "dep:trust-dns-proto"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think I'm in favor of keeping it generic. I was curious what the status of trust-dns-resolver was and it looks like they've actually "rebranded" to a different crate (see here) that we might want to switch over to at some point. Keeping the name neutral will make it easier for us to swap out the internal dep.

Worst case, if we did want to introduce more resolver crates we could treat this flag as our "default" choice and then allow users to opt into a different one if they want to.

@abr-egn abr-egn merged commit 8c62064 into mongodb:main Apr 5, 2024
8 of 11 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.

2 participants