-
Notifications
You must be signed in to change notification settings - Fork 503
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
build: remove service-*
from default features
#4311
Conversation
"services-webhdfs", | ||
"services-azfile", | ||
] | ||
default = ["rustls"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure whether it's a good idea to remove rustls
, so just keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
core/Cargo.toml
Outdated
"services-azblob", | ||
"services-memory", | ||
"services-http", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This affects a little dev experience. Now cargo test
without --features tests
cannot compile.
Other problems fixed. The only remaining issue is how to enable features for C bindings 🤔 BTW, does the released bindings contain all features? |
Most of C binding users will write their own |
In this PR, I think it's fine to add our default features in C binding directly: Lines 40 to 42 in a510291
|
// Deny unused qualifications. | ||
#![deny(unused_qualifications)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
main CI failed. Let me fix it |
Thanks a lot! |
Maybe it's fine to enable memory service by default? |
That's acceptable to me. But I think it's suboptimal, since memory service isn't frequently used in production. 🤔 |
Oh, but memory service has very few code. Sounds better then. |
But what problems does it solve? Do you mean we just need memory service for CI? 🤔 I think we still need to consider what services to enable when release bindings. (BTW, I noticed that there's no much doc about how to enable features for bindings) |
They are determined by the owners of the bindings. Up to now, most bindings opt to include all services except for a few special ones.
Yes, we need to improve this part. |
#4310
some implications:
cargo test
cannot compile. Have tocargo test --features tests
. This is a little bad. Maybe we can consider other ways e.g., gate unit tests withcfg(feature)
.I think the root cause is just the drawbacks of using feature flags, but less related with changing the default features.