p2pderivatives / rust-bitcoin-coin-selection

10 stars 5 forks source link

Move crate into rust-bitcoin #9

Closed tcharding closed 1 year ago

tcharding commented 1 year ago

Hi,

Some things to consider for this to be brought over into rust-bitcoin. These are my observations only, I do not speak authoritatively for the rust-bitcoin project. Also, forgive me if these things have been mentioned/discussed already.

Bonus Points

// Coding conventions.
#![deny(non_upper_case_globals)]
#![deny(non_camel_case_types)]
#![deny(non_snake_case)]
#![deny(unused_mut)]
#![deny(missing_docs)]

(Please note I did not study the logic closely, these comments are all just surface things).

Thanks man!

yancyribbens commented 1 year ago

@tcharding

Use #[cfg_attr(docsrs, doc(cfg(feature = "rand)))] anywhere that uses #[cfg(feature = "rand")] (adds a little badge to docs saying requires feature X, saves explicitly mentioning this in rustdoc)

About your comment here, when I open the docs page with:

cargo doc --open --features="rand"

It doesn't appear any different with or without #[cfg_attr(docsrs, doc(cfg(feature = "rand)))] AFAIKT. Am I missing something?

tcharding commented 1 year ago

We need to set docsrs using the RUSTDOCFLAGS env var, here is an example - you could throw the same in the ci script in #10 if you wanted.


# Build the docs if told to (this only works with the nightly toolchain)
if [ "$DO_DOCS" = true ]; then
    RUSTDOCFLAGS="--cfg docsrs" cargo rustdoc --all-features -- -D rustdoc::broken-intra-doc-links -D warnings || exit 1
fi

In my shell environment I have an alias defined

build-docs='RUSTDOCFLAGS="--cfg docsrs" cargo +nightly rustdoc --all-features -- -D rustdoc::broken-intra-doc-links'

--cfg docsrs tells the compiler to create and set to true docsrs.

We use the same trick to define bench and use it to feature guard benchmark modules. The string "docsrs" and "bench" are made up i.e., they are just identifiers, one can use anything. FTR we set bench using (note the env var is different)

RUSTFLAGS='--cfg=bench' cargo bench
yancyribbens commented 1 year ago

Hi @tcharding

I'm a little confused, shouldn't this be

# Build the docs if told to (this only works with the nightly toolchain)
if [ "$DO_DOCS" = true ]; then
    RUSTDOCFLAGS="--cfg docsrs" cargo +nightly rustdoc --all-features -- -D rustdoc::broken-intra-doc-links -D warnings || exit 1
fi

Notice I added +nightly. Unfortunetly rustup insn't playing well with my current package manager so I can't test this locally using the cargo +nightly toolchain at the moment. Without using +nightly I see the following:

error[E0658]: `#[doc(cfg)]` is experimental
  --> src/lib.rs:31:20
   |
31 | #[cfg_attr(docsrs, doc(cfg(feature = "rand")))]
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #43781 <https://github.com/rust-lang/rust/issues/43781> for more information

error[E0658]: `#[doc(cfg)]` is experimental
  --> src/lib.rs:48:20
   |
48 | #[cfg_attr(docsrs, doc(cfg(feature = "rand")))]
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #43781 <https://github.com/rust-lang/rust/issues/43781> for more information

error: Compilation failed, aborting rustdoc

For more information about this error, try `rustc --explain E0658`.
error: could not document `rust-bitcoin-coin-selection
yancyribbens commented 1 year ago

@tcharding I was able to get the +nightly toolchain to work, although one other observation is that it will only work if we add:

#![feature(doc_cfg)]

as the error message warns.

tcharding commented 1 year ago

My bad, you are correct. We have in rust-bitcoin in lib.rs: #![cfg_attr(docsrs, feature(doc_cfg))].

+nightly is ok to have in there (its there in the current test.sh in rust-bitcoin) but its redundant because we only ever enable DO_DOCS in CI when using a nightly toolchain. I removed it when pasting for you to try and simplify things, didn't work :)

tcharding commented 1 year ago

FTR we use nightly for building docs and also for running rustfmt.

yancyribbens commented 1 year ago

@tcharding https://github.com/p2pderivatives/rust-bitcoin-coin-selection/pull/12. Look ok? Thanks!