Only use necessary futures components

[onalante-msft]

Motivation

Dependents of kube-rs pull in all futures components, only some of which are used by kube-rs crates. futures is relatively compact, but it would still be helpful for compile times and dependency graph auditing to only pull in necessary components.

Solution

kube-rs only has direct need of futures-util (for extension traits, utility functions and structs, and macros) and futures-channel (for mpsc and oneshot). The dependencies have been adjusted to reflect this.

Note

I also modified the development dependencies in the same manner as the build dependencies, but it makes for slightly less ergonomic examples in the documentation.¹ If this is an issue, I can restore full futures as a development dependency.

Footnotes

1. I left in some tokio feature restrictions as well, but these have no effect on the code. I can restore the full feature list if that is preferred. ↩

[clux]

while i like the trimming of dependencies we don't use, the use of all their peripheral crates (when futures have a facade-crate model) makes our documentation look less crisp (can no longer just point to futures, docs need to point at one or more sub-crates) and it feels slightly wrong to push that hierarchy complexity onto us + users.

would it make sense to use futures directly without default features to achieve a similar result?

[onalante-msft]

makes our documentation look less crisp

Another wrinkle that I was not aware of was that rustdoc does not include development dependencies when generating the links, so even having futures as a development dependency is not sufficient to keep the documentation in its current form.

would it make sense to use futures directly without default features to achieve a similar result?

I think it would be fine, but I do not have a strong opinion either way. My PR was largely predicated on the above misconception about rustdoc behavior, and I completely agree with your feeling that it seems wrong to use the futures subcrates in official documentation.

Realistically, saving 4 dependencies by using the futures subcrates directly is not all that significant, so I will close this PR for the time being unless there are any that would really like to have this included. Nonetheless, thank you for your time in reviewing the changes.

[clux]

i do appreciate you taking the time to do this. was not aware that the ecosystem was so consistent in the futures ecosystem use (most tend to only use util + core, and occasionally channel) so i think there is real value in aligning.

I think it's actually possible to get around the documentation issue by just referring to futures even though the file we are documenting does not have it in scope. we can use the same trick we use internally by referring to a crate in dev-dependencies (and have full futures listed in dev-dependencies)

[onalante-msft]

I think it's actually possible to get around the documentation issue by just referring to futures

Interesting, I did not know that that was a possibility. I will give it a try and reopen the PR if that is alright with you.

[onalante-msft]

#900 (comment)

[onalante-msft]

It is possible to only require futures-util and futures-channel in dependents while preserving documentation links by restoring futures as a regular dependency gated behind a feature flag only used for documentation.¹

Footnotes

1. This is not quite what was recommended in https://github.com/kube-rs/kube-rs/pull/900#discussion_r868303411; I could not get rid of the warnings during my attempt to implement the suggestion. ↩

[nightkr]

Won't the docs-only feature break people who run cargo doc from their own workspaces?

[clux]

Won't the docs-only feature break people who run cargo doc from their own workspaces?

That's kind of already broken. You really need to run the big string in make doc that runs:

RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --lib --workspace --features=derive,ws,oauth,jsonpatch,client,derive,runtime,admission,k8s-openapi/v1_23 --open

which emulates the docs.rs flags we tell docs.rs to use: https://github.com/kube-rs/kube-rs/blob/c976f9ee7d3aab59b9fa2f017d01e986e0bb3afa/kube/Cargo.toml#L33-L36

EDIT: you'll still be able technically able to run cargo doc, but most features won't be visible/clickable from kube, and if we merge this, some futures links won't resolve either.

[clux]

This is not quite what was recommended in Only use necessary futures components #900 (comment); I could not get rid of the warnings during my attempt to implement the suggestion. leftwards_arrow_with_hook

Yeah, this is slightly worse with the extra feature. I didn't realise dev-deps didn't actually resolve in doc links because we relied on them to cross-link kube crates. It just happens to work for us because we also happen to build all the kube crates 🙃

However, I don't think this is actually working as is?
I tried it locally with make doc and the links in watcher does not resolve, even with the feature.

[onalante-msft]

I tried it locally with make doc and the links in watcher does not resolve, even with the feature.

That is unexpected... I just tried make doc on a clean copy of my branch, and everything resolved without issue. I am running Ubuntu 20.04 server; are you running on another OS?

[nightkr]

That's kind of already broken.

Not really. You won't get feature-is-required tags (which is slightly annoying), but cargo doc does answer "what is currently in my workspace, and how does it work?" perfectly.

[onalante-msft]

In that case, would it be acceptable to refer to futures_util for the crate links but keep futures in the doctests? This would obviate the need for the internal feature flag.

example.diff.txt

[clux]

In that case, would it be acceptable to refer to futures_util for the crate links but keep futures in the doctests? This would obviate the need for the internal feature flag.

Yeah, given the constraints I think it's probably fine to link to the futures_* subcrates in documentation.

[clux]

That is unexpected... I just tried make doc on a clean copy of my branch, and everything resolved without issue. I am running Ubuntu 20.04 server; are you running on another OS?

I don't think OS should really matter here. It might be down to rust versions, but i just updated nightly and ran the command in the makefilejustfile; just doc and the links in the watcher example ends up pointing to:

file:///home/clux/kube/kube-rs/target/doc/kube/runtime/futures_util::TryStream

whereas they should point to something under /target/doc/futures_util. not sure why that is. it looks correct to me, and futures_util is being built, it's there, i can browse to it in the docs directory.

This is with your latest force push. Beyond that, if the links would resolve i am all 👍 on this.

[nightkr]

and the links in the watcher example ends up pointing to:

I have ran into this before! IIRC it should be fixable by adding an otherwise useless (heh…) use futures_util; statement.

[clux]

Yep, tested with #[allow(unused_imports)] use futures_util; in watcher.rs. Works.

[onalante-msft]

I understand the issue now, sorry about the confusion. I cannot get the reference to futures_util::stream::select at kube-runtime/src/controller/mod.rs:239 to resolve, now that I can see the problem...

[clux]

yeah, that one seems to not work no matter what i try. it's not like select is even hidden behind any features 😕
we could maybe put it down to some of the black magic they use in their module system...

need to think about this a bit.

[onalante-msft]

I believe the root cause for this is rust-lang/rust#87048. When trying to resolve the link for futures_util::stream::select, the (original) function futures_util::stream::select::select hits this error condition. This is because !cache.access_levels.is_public(did) (incorrectly and unexpectedly) evaluates to true: it appears that futures_util::stream::select::select is not even added to the access_levels map.

Even more strangely, if I create a test crate with a similar module layout and re-export declaration

pub mod foo { // in folder
    mod bar { // in file
        pub fn bar() {}
    }
    pub use self::bar::bar;
}

I can refer to test_crate::foo::bar without problems.

In any case, I will try to come up with a workaround.

[clux]

Another possibly interesting point. The current local build seems to sometimes resolve better than what docs.rs does.
Case in point: WatchStreamExt::backoff resolves locally, but did not resolve in the last docs.rs build.

[onalante-msft]

One possible solution for the time being is to have use --document-private-items in the cargo doc invocation. That fixes the resolution issues for me, though it has the undesirable side effect of polluting the public documentation.

[clux]

This got left behind. Apologies. Closing in favour of smaller, but more specific feature tuning done in #1442