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

build: remove service-* from default features #4311

Merged
merged 12 commits into from
Mar 5, 2024

Conversation

xxchan
Copy link
Contributor

@xxchan xxchan commented Mar 4, 2024

#4310

some implications:

  • now cargo test cannot compile. Have to cargo test --features tests. This is a little bad. Maybe we can consider other ways e.g., gate unit tests with cfg(feature).
  • Clippy produced some lints.

I think the root cause is just the drawbacks of using feature flags, but less related with changing the default features.

@xxchan xxchan requested a review from Xuanwo as a code owner March 4, 2024 07:51
@github-actions github-actions bot requested a review from G-XD March 4, 2024 07:51
"services-webhdfs",
"services-azfile",
]
default = ["rustls"]
Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

@xxchan xxchan marked this pull request as draft March 4, 2024 07:54
core/Cargo.toml Outdated
Comment on lines 55 to 57
"services-azblob",
"services-memory",
"services-http",
Copy link
Contributor Author

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.

@xxchan
Copy link
Contributor Author

xxchan commented Mar 4, 2024

Other problems fixed. The only remaining issue is how to enable features for C bindings 🤔

BTW, does the released bindings contain all features?

@Xuanwo
Copy link
Member

Xuanwo commented Mar 4, 2024

The only remaining issue is how to enable features for C bindings 🤔

Most of C binding users will write their own Makefile. We can define some envs and read them in build.rs, but it may need another PR to implement. Do you have interest?

@Xuanwo
Copy link
Member

Xuanwo commented Mar 4, 2024

In this PR, I think it's fine to add our default features in C binding directly:

opendal = { version = "0.45.1", path = "../../core", features = [
"layers-blocking",
] }

Comment on lines -132 to -133
// Deny unused qualifications.
#![deny(unused_qualifications)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because fuzz test failed to compile. quite weird. Anyway I don't think there's need to use the deny level.. warn is enough.

image

@xxchan xxchan marked this pull request as ready for review March 5, 2024 03:47
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks!

@Xuanwo Xuanwo merged commit f914dc3 into apache:main Mar 5, 2024
200 checks passed
@xxchan
Copy link
Contributor Author

xxchan commented Mar 5, 2024

main CI failed. Let me fix it

@Xuanwo
Copy link
Member

Xuanwo commented Mar 5, 2024

main CI failed. Let me fix it

Thanks a lot!

@Xuanwo
Copy link
Member

Xuanwo commented Mar 5, 2024

Maybe it's fine to enable memory service by default?

@xxchan
Copy link
Contributor Author

xxchan commented Mar 5, 2024

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. 🤔

@xxchan
Copy link
Contributor Author

xxchan commented Mar 5, 2024

Oh, but memory service has very few code. Sounds better then.

@xxchan
Copy link
Contributor Author

xxchan commented Mar 5, 2024

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)

@Xuanwo
Copy link
Member

Xuanwo commented Mar 5, 2024

I think we still need to consider what services to enable when release 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.

(BTW, I noticed that there's no much doc about how to enable features for bindings)

Yes, we need to improve this part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants