Improve test coverage for docs build #1505

tcharding · 2022-12-23T22:53:25Z

Currently the docs build commands in hashes and bitcoin differ, they should be the same.

Add a command cargo doc to improve coverage e.g., recently we botched the feature guarding but since CI only runs cargo rustdoc with custom compiler conditional set we didn't catch it.

Done after seeing: #1504 and CI should fail on this PR until 1504 is in.

Kixunil · 2022-12-23T23:37:09Z

bitcoin/contrib/test.sh

+    # We use rustdoc so that we can check for broken links.
+    RUSTDOCFLAGS="--cfg docsrs" cargo +nightly rustdoc --all-features -- -D rustdoc::broken-intra-doc-links -D warnings
+    # This, in unison with the command above, checks that we feature guarded docs imports correctly.
+    cargo +nightly doc


If we're doing this we should still have all features and deny warnings, just docsrs disabled. And probably test it on stable instead.

Yes and face palm for --all-features and deny warnings. For test on stable, seems reasonably, will require more thinking though since DO_DOCS is only run on nightly, perhaps we need to agree on what is expected from each toolchain since this is similar to the discussion on features only working on some toolchains (#1498 (comment))

I think just adding +stable should work? It's a bit ugly hack, yes. Or just split it up into DO_DOCSRS and DO_DOCS.

Unless I'm missing something the stable toolchain won't be installed on the VM when we use DO_DOCS. I used your DO_DOCSRS idea, adding a DO_DOCS for stable builds.

Kixunil · 2022-12-23T23:37:22Z

hashes/contrib/test.sh

+    # We use rustdoc so that we can check for broken links.
+    RUSTDOCFLAGS="--cfg docsrs" cargo +nightly rustdoc --all-features -- -D rustdoc::broken-intra-doc-links -D warnings
+    # This, in unison with the command above, checks that we feature guarded docs imports correctly.
+    cargo +nightly doc


Same as above.

tcharding · 2022-12-28T00:04:31Z

@apoelstra if this merges it will likely mean you have to modify your test scripts if you are testing docs builds locally before merging.

Kixunil

ACK 43e3d2b

Didn't notice cargo +stable doc vs cargo doc +stable

tcharding · 2022-12-28T23:48:00Z

My bad, fixed.

Kixunil · 2022-12-30T21:34:43Z

There are conflicts now.

tcharding · 2022-12-31T23:58:48Z

Rebase only, no other changes.

apoelstra · 2023-01-01T16:31:38Z

Thanks for the heads up! I'm not currently testing docs but I should start.

apoelstra

ACK fb23a94

Kixunil

Shouldn't this contain changes to CI?

tcharding · 2023-01-09T01:18:47Z

Shouldn't this contain changes to CI?

Oops, thanks man. Now includes CI changes.

tcharding · 2023-01-09T02:45:31Z

Now includes additional patch to fix docs build warnings.

Kixunil · 2023-01-09T05:59:35Z

bitcoin/contrib/test.sh

 if [ "$DO_DOCS" = true ]; then
-    RUSTDOCFLAGS="--cfg docsrs" cargo +nightly rustdoc --all-features -- -D rustdoc::broken-intra-doc-links -D warnings || exit 1
+    cargo +stable doc --all-features


Shouldn't this have -D warnings as well?

-D warnings is a cargo rustdoc option not a cargo doc option (I learned while doing this PR).

I think you can still put it into RUSTDOCFLAGS. cargo doc literally runs rustdoc inside.

TIL, that's awesome I didn't realise. I changed both invocations to use cargo doc to make it uniform. While testing I noticed we do not have a contrib/test.sh file for internals, will add.

Kixunil · 2023-01-13T06:27:49Z

Looks like #1545 must go in first.

We already use `set -ex`, adding an explicit exit on command fail is redundant.

Cargo emits various warnings when building the docs: warning: this URL is not a hyperlink As suggested, add angle brackets to create automatic links.

Currently the docs build commands in `hashes` and `bitcoin` differ, they should be the same. Add a command `cargo doc` to improve coverage e.g., recently we botched the feature guarding but since CI only runs `cargo rustdoc` with custom compiler conditional set we didn't catch it. Run docs in CI using nightly and stable toolchains as required.

tcharding · 2023-01-16T02:57:51Z

Rebase only, no other changes.

apoelstra

ACK 6dfacd1

apoelstra

ACK 41f2dcf

tcharding · 2023-02-10T00:18:06Z

No changes since your ack of fb23a94 except for rebasing @Kixunil. If you get a second can you ack this one again please.

Kixunil

ACK 41f2dcf

tcharding mentioned this pull request Dec 23, 2022

Change #[cfg(docsrs)] to #[cfg(doc)] on use #1504

Merged

Kixunil requested changes Dec 23, 2022

View reviewed changes

tcharding force-pushed the 12-24-ci-docs branch from 553d139 to 69571bd Compare December 28, 2022 00:02

tcharding force-pushed the 12-24-ci-docs branch from 69571bd to 43e3d2b Compare December 28, 2022 00:07

Kixunil previously approved these changes Dec 28, 2022

View reviewed changes

tcharding force-pushed the 12-24-ci-docs branch from 43e3d2b to 8a6e52a Compare December 28, 2022 23:47

tcharding force-pushed the 12-24-ci-docs branch 2 times, most recently from 3892d2f to 44e56f9 Compare December 29, 2022 00:56

tcharding force-pushed the 12-24-ci-docs branch from 44e56f9 to fb23a94 Compare December 31, 2022 23:58

apoelstra previously approved these changes Jan 1, 2023

View reviewed changes

Kixunil reviewed Jan 2, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via 2235e7e January 9, 2023 01:18

tcharding force-pushed the 12-24-ci-docs branch from fb23a94 to 2235e7e Compare January 9, 2023 01:18

tcharding force-pushed the 12-24-ci-docs branch 2 times, most recently from 19bf3ac to 48df5ad Compare January 9, 2023 02:45

Kixunil reviewed Jan 9, 2023

View reviewed changes

tcharding force-pushed the 12-24-ci-docs branch from 48df5ad to 6dfacd1 Compare January 12, 2023 20:29

Kixunil added the Blocked Some other work is required to make this possible label Jan 13, 2023

tcharding added 3 commits January 16, 2023 13:56

ci: Remove redundant || exit

96e8a08

We already use `set -ex`, adding an explicit exit on command fail is redundant.

hashes: Use automatic link

b4c14a4

Cargo emits various warnings when building the docs: warning: this URL is not a hyperlink As suggested, add angle brackets to create automatic links.

tcharding force-pushed the 12-24-ci-docs branch from 6dfacd1 to 41f2dcf Compare January 16, 2023 02:57

Kixunil removed the Blocked Some other work is required to make this possible label Jan 16, 2023

This comment was marked as duplicate.

Sign in to view

apoelstra approved these changes Jan 16, 2023

View reviewed changes

Kixunil approved these changes Feb 10, 2023

View reviewed changes

apoelstra merged commit 9615dd1 into rust-bitcoin:master Feb 10, 2023

tcharding deleted the 12-24-ci-docs branch February 13, 2023 19:50

tcharding added Tests aggregate release notes mention labels Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve test coverage for docs build #1505

Improve test coverage for docs build #1505

tcharding commented Dec 23, 2022 •

edited

Kixunil Dec 23, 2022

tcharding Dec 26, 2022

Kixunil Dec 27, 2022

tcharding Dec 28, 2022

Kixunil Dec 23, 2022

tcharding commented Dec 28, 2022

Kixunil left a comment

tcharding commented Dec 28, 2022

Kixunil commented Dec 30, 2022

tcharding commented Dec 31, 2022

apoelstra commented Jan 1, 2023

apoelstra left a comment

Kixunil left a comment

tcharding commented Jan 9, 2023

tcharding commented Jan 9, 2023

Kixunil Jan 9, 2023

tcharding Jan 9, 2023 •

edited

Kixunil Jan 11, 2023

tcharding Jan 12, 2023

Kixunil commented Jan 13, 2023

tcharding commented Jan 16, 2023

This comment was marked as duplicate.

apoelstra left a comment

apoelstra left a comment

tcharding commented Feb 10, 2023

Kixunil left a comment

Improve test coverage for docs build #1505

Improve test coverage for docs build #1505

Conversation

tcharding commented Dec 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Dec 28, 2022

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented Dec 28, 2022

Kixunil commented Dec 30, 2022

tcharding commented Dec 31, 2022

apoelstra commented Jan 1, 2023

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented Jan 9, 2023

tcharding commented Jan 9, 2023

Choose a reason for hiding this comment

tcharding Jan 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil commented Jan 13, 2023

tcharding commented Jan 16, 2023

This comment was marked as duplicate.

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Feb 10, 2023

Kixunil left a comment

Choose a reason for hiding this comment

tcharding commented Dec 23, 2022 •

edited

tcharding Jan 9, 2023 •

edited