Need cargo doc --all-features test in CI/CD

[rrybarczyk]

Currently there is no checks to ensure the cargo documentation builds. To ensure no code changes break documentation, we need to ensure that cargo doc --all-features passes for every crate.

[plebhash]

this is not a comprehensive analysis (maybe more crates are broken), but I tried it locally and it seems template_distribution_sv2 is broken:

    Checking template_distribution_sv2 v1.0.3 (/Users/plebhash/develop/stratum/protocols/v2/subprotocols/template-distribution)
error[E0433]: failed to resolve: use of undeclared type `Vec`
   --> v2/subprotocols/template-distribution/src/new_template.rs:209:28
    |
209 |             let mut path = Vec::new();
    |                            ^^^ use of undeclared type `Vec`
    |
help: consider importing one of these items
    |
7   + use alloc::vec::Vec;
    |
7   + use crate::vec::Vec;
    |

error[E0433]: failed to resolve: use of undeclared type `Vec`
   --> v2/subprotocols/template-distribution/src/new_template.rs:230:34
    |
230 |             coinbase_tx_outputs: Vec::new().try_into().unwrap(),
    |                                  ^^^ use of undeclared type `Vec`
    |
help: consider importing one of these items
    |
7   + use alloc::vec::Vec;
    |
7   + use crate::vec::Vec;
    |

error[E0599]: no method named `try_into` found for struct `Vec<u8>` in the current scope
   --> v2/subprotocols/template-distribution/src/new_template.rs:195:66
    |
195 |         let coinbase_prefix: binary_sv2::B0255 = coinbase_prefix.try_into().unwrap();
    |                                                                  ^^^^^^^^
    |
   ::: /Users/plebhash/.rustup/toolchains/1.78-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/convert/mod.rs:611:8
    |
611 |     fn try_into(self) -> Result<T, Self::Error>;
    |        -------- the method is available for `Vec<u8>` here
    |
    = help: items from traits can only be used if the trait is in scope
    = note: 'core::convert::TryInto' is included in the prelude starting in Edition 2021
help: trait `TryInto` which provides `try_into` is implemented but not in scope; perhaps you want to import it
    |
1   + use core::convert::TryInto;
    |
help: there is a method `into` with a similar name
    |
195 |         let coinbase_prefix: binary_sv2::B0255 = coinbase_prefix.into().unwrap();
    |                                                                  ~~~~

Some errors have detailed explanations: E0433, E0599.
For more information about an error, try `rustc --explain E0433`.
error: could not compile `template_distribution_sv2` (lib) due to 3 previous errors
warning: build failed, waiting for other jobs to finish...

cc @jbesraa

[plebhash]

ok after digging a bit, it seems the error on template_distribution_sv2 is likely related to the no_std flag

AFAIU @Georges760 #1230 is yet to send a PR addressing that on subprotocols crates, so we should probably wait until this is sorted out before we proceed on enforcing --all-features for subprotocols

for now, subprotocols crates should be published without --all-features

[Georges760]

--all-features breaks a lot of protocols/v2 crates, even before #1230 merge.

for instance, codec_sv2 cargo build is broken with --all-features since a long time. Neither #1230 or #1238 or the next PR specific to codec_sv2 for no_std (ready for after #1238) will solve this issue, because it is not related to the std feature, but more to the with_serde and with_buffer_pool features.

During my no_std quest I noticed that and reported it somewhere, but didn't consider it as my main task, specially because i was told the with_serde feature will be deprecated.

[jbesraa]

this is not a comprehensive analysis (maybe more crates are broken), but I tried it locally and it seems template_distribution_sv2 is broken:

    Checking template_distribution_sv2 v1.0.3 (/Users/plebhash/develop/stratum/protocols/v2/subprotocols/template-distribution)
error[E0433]: failed to resolve: use of undeclared type `Vec`
   --> v2/subprotocols/template-distribution/src/new_template.rs:209:28
    |
209 |             let mut path = Vec::new();
    |                            ^^^ use of undeclared type `Vec`
    |
help: consider importing one of these items
    |
7   + use alloc::vec::Vec;
    |
7   + use crate::vec::Vec;
    |

error[E0433]: failed to resolve: use of undeclared type `Vec`
   --> v2/subprotocols/template-distribution/src/new_template.rs:230:34
    |
230 |             coinbase_tx_outputs: Vec::new().try_into().unwrap(),
    |                                  ^^^ use of undeclared type `Vec`
    |
help: consider importing one of these items
    |
7   + use alloc::vec::Vec;
    |
7   + use crate::vec::Vec;
    |

error[E0599]: no method named `try_into` found for struct `Vec<u8>` in the current scope
   --> v2/subprotocols/template-distribution/src/new_template.rs:195:66
    |
195 |         let coinbase_prefix: binary_sv2::B0255 = coinbase_prefix.try_into().unwrap();
    |                                                                  ^^^^^^^^
    |
   ::: /Users/plebhash/.rustup/toolchains/1.78-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/convert/mod.rs:611:8
    |
611 |     fn try_into(self) -> Result<T, Self::Error>;
    |        -------- the method is available for `Vec<u8>` here
    |
    = help: items from traits can only be used if the trait is in scope
    = note: 'core::convert::TryInto' is included in the prelude starting in Edition 2021
help: trait `TryInto` which provides `try_into` is implemented but not in scope; perhaps you want to import it
    |
1   + use core::convert::TryInto;
    |
help: there is a method `into` with a similar name
    |
195 |         let coinbase_prefix: binary_sv2::B0255 = coinbase_prefix.into().unwrap();
    |                                                                  ~~~~

Some errors have detailed explanations: E0433, E0599.
For more information about an error, try `rustc --explain E0433`.
error: could not compile `template_distribution_sv2` (lib) due to 3 previous errors
warning: build failed, waiting for other jobs to finish...

cc @jbesraa

I dont think there is a problem here. prop_test feature requires std and we have no_std feature. if you run cargo b --all--features you enable prop_test and no_std (and with_serde) features, which means std is not available but prop_test need it

[Georges760]

I dont think there is a problem here. prop_test feature requires std and we have no_std feature. if you run cargo b --all--features you enable prop_test and no_std (and with_serde) features, which means std is not available but prop_test need it

Thats is exactly why usually crates don't have a no_std feature, but a std by default instead, so the --all-features bring std along others. Whereas no_std is achieved by --no-default-features + some no_std compatible features carefully selected.

That's what I called the "no_std logic" being inverted in our crates. And all my PR removed these no_std features in profit of std by default enabled (if needed only) feature. I am not yet to the subprotocols level, but it will have to be done too.

[jbesraa]

Subprotocols don't need any std feature as they are only defining structs and not doing any impl or writing any function. prop_test which is a testing tool should probably be removed in favor of fuzzy testing in a separate crate(good example https://github.com/lightningdevkit/rust-lightning/tree/main/fuzz).

[Georges760]

The subprotocols dont need any std feature as they are only defining structs and not doing any impl or writing any function. prop_test which is a testing tool should probably be removed in favor of fuzzy testing in a separate crate(good example https://github.com/lightningdevkit/rust-lightning/tree/main/fuzz).

I was blindly talking about a std feature in the subprotocols crates (didn't look closely at them yet), because there is an existing no_std feature and the "logic" I was expressing above.

But yes, if they are "only defining structs and not doing any impl or writing any function" the std dependency can easily be replaced by alloc (usually for String and Vec most of the time), so the no_std features should not be needed in the first place, and the crate can be #![no_std] by nature. Only the prop_test could need std (if they survive any fuzzy test replacement), why not, by using #![cfg_attr(not(feature = "pro_test"), no_std)] or better #![cfg_attr(not(feature = "std"), no_std)] +

[features]
default = ["std"]
std = []
prop_test = ["std"]

So --all-features will do prop_test with std available, and --no-default-features --features prop_test will still have std...

[jbesraa]

why are we expecting cargo build --all-features to work? In general I don't think it should necessarily work

[plebhash]

why are we expecting cargo build --all-features to work? In general I don't think it should necessarily work

nACK

as stated on the issue description:

ideally we should enforce cargo publish --all-features on .github/workflows/release-libs.yaml to make sure Rust Docs are published with all features enabled, otherwise we're not showing all the APIs that each crate provides

this is being implemented in the second commit of #1313

edit: I had forgotten that all protocols crates have the following enabled:

[package.metadata.docs.rs]
all-features = true

[jbesraa]

Hmm requiring each crate to build with --all-features flag sounds a very hard requirement for me, and that's what cargo publish --all-features is doing. The downside here IMO is much bigger than adding documentation for mostly a single useful feature(the buffer one), because every other feature is just about serde/prop_test and both for internal use..

While we could try and make sure each crate build with --all-features but to me it feels counterintuitive as features are sometimes contradictory(no_std/std, etc..).

You might be able to achieve what you want with this: https://vadosware.io/post/getting-features-to-show-up-in-your-rust-docs

The advantage of always making sure --all-features builds is that it validates that almost EVERY combination works. In our case, where we only have almost a single feature(and probably wont add any in the near future) I think it is not that a of a big deal.

With all of the above said, once we remove no_std/std, no_serde/serde the --all-feature should probably work and we wont break it but adding all of this to the ci feels like too much noise.

[Shourya742]

Hmm requiring each crate to build with --all-features flag sounds a very hard requirement for me, and that's what cargo publish --all-features is doing. The downside here IMO is much bigger than adding documentation for mostly a single useful feature(the buffer one), because every other feature is just about serde/prop_test and both for internal use..

While we could try and make sure each crate build with --all-features but to me it feels counterintuitive as features are sometimes contradictory(no_std/std, etc..).

You might be able to achieve what you want with this: https://vadosware.io/post/getting-features-to-show-up-in-your-rust-docs

I agree with @jbesraa . Currently, the features in our protocol crates are mutually exclusive. Some feature sets cannot compile or coexist, which goes against Rust's standards. The primary issue is with serde and no_serde. Once we address that, we can achieve compatible build scenarios with all features enabled and could integrate cargo hack in our CI.

[Georges760]

#1315 is proposing all 4 subprotocols crates no_std support by actually removing the existing no_std feature as the 4 crates are indeed never depending on std anywhere.

This will not solve the --all-features question above, but at least one less (useless) feature make the question easier to understand...

[plebhash]

I should correct some innacurate statements. We're not running cargo publish --all-features on CI, nor do we need to.

protocols crates have the following on their Cargo.toml:

[package.metadata.docs.rs]
all-features = true

this already already the equivalent of doing cargo publish --all-features on release-libs.yaml

Hmm requiring each crate to build with --all-features flag sounds a very hard requirement for me, and that's what cargo publish --all-features is doing.

ideally we want to catch these problems sooner rather than later

otherwise, we will always have to do last-minute PRs on releases to publish crates

---

overall, I see how all-features can be overkill in some ways

the main point of this should be that for all relevant crate features:

• we enforce Rust Docs builds without issues (while covering all relevant feature flags for the specific crate)
• we enforce Rust Docs is published without issues (while covering all relevant feature flags for the specific crate)

[plebhash]

I just created #1318 to simplify the [package.metadata.docs.rs] approach

with this PR, while publishing we only enable features that are relevant for API usage

---

with regards to the request on this original issue, the proposed CI would enforce cargo doc --features X on the features listed on #1318 instead of --all-features