8000 Check for rustdocs build warnings in CI by tcharding · Pull Request #1404 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Check for rustdocs build warnings in CI #1404

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 21, 2022
Merged

Check for rustdocs build warnings in CI #1404

merged 2 commits into from
Nov 21, 2022

Conversation

tcharding
Copy link
Member
@tcharding tcharding commented Nov 20, 2022 •
edited by apoelstra
Loading

Currently we do not fail the CI script if the docs build throws warnings, since we are a group of super anal, easily triggered, code cleanliness obsessed devs this causes a mild rash to develop on the lower back [0]. We can easily fix this by checking for build warnings in CI.

[0] - Amusingly my rash has been playing up since Friday but I thought I'd fixed the warnings in an open PR someplace so I was ignoring it, seeing Kixunil's issue this morning prompted me to fix it :)

Fix #1403

@tcharding
Fix rustdocs build warnings
b014f0f 
Building the docs throws a bunch of warnings of form

  warning: unclosed HTML tag ...

Add code ticks to remove the warnings.
Kixunil
Kixunil previously approved these changes Nov 20, 2022
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 8847475e636bf536c2c6083634b505d1eb75be59

If CI succeeds.

@tcharding
Copy link
Member Author

Agh, had to use --all-features to get the docs to build, without it (using explicit feature list) we get

error: too many file operands

@tcharding
Fail CI if docs build throws warnings
108a1f7 
Convert all rustdocs build warnings to errors using `-D warnings` and
exit the script with 1 to signal the error.

While we are at it use `--all-features` instead of explicit feature list
when building docs. Without this the docs build fails with:

  error: too many file operands
@tcharding
Copy link
Member Author

I had used <bash command> | exit 1 instead of <bash command> || exit 1 :)

@Kixunil
Copy link
Collaborator
Kixunil commented Nov 20, 2022

I think --features requires them to be comma-separated.

I had used <bash command> | exit 1 instead of <bash command> || exit 1 :)

Lol, that was quite a confusing.

@tcharding
Copy link
Member Author

I think --features requires them to be comma-separated.

Yeah I cannot work out how the variable interpolation is working in the CI script, there is another example of it that works when it looks to me like it shouldn't

cargo build --verbose --features="no-std $FEATURES" --no-default-features

On the command line this has to use comma separation as you say?

Kixunil
Kixunil approved these changes Nov 21, 2022
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 108a1f7

@Kixunil
Copy link
Collaborator
Kixunil commented Nov 21, 2022

No clue about that, I'm going to sleep now. :D

apoelstra
apoelstra approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 108a1f7

@apoelstra apoelstra merged commit 1e4b5e3 into rust-bitcoin:master Nov 21, 2022
@tcharding tcharding deleted the 11-21-ci-rustdocs branch November 22, 2022 02:54
@Kixunil
Copy link
Collaborator
Kixunil commented Dec 22, 2022

This fix either doesn't work or broke again at some point. There are doc warnings again.

tcharding
tcharding commented Dec 23, 2022
View reviewed changes
bitcoin/contrib/test.sh
@@ -81,7 +81,7 @@ cargo run --example taproot-psbt --features=bitcoinconsensus

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

Choose a reason for hiding this comment

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

We have set -ex so this "fix" did not add any new behaviour to the script :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think -D doesn't work for some reason.

@tcharding
Copy link
Member Author

This fix either doesn't work or broke again at some point. There are doc warnings again.

Are you still seeing warnings?

I'm on "comit: 46eda5f Merge #1499: Add from_bytes(Vec<u8>) to ScriptBuf"

And RUSTDOCFLAGS="--cfg docsrs" cargo +nightly rustdoc --all-features -- -D rustdoc::broken-intra-doc-links shows no warnings, what command are you using?

@Kixunil
Copy link
Collaborator
Kixunil commented Dec 23, 2022

Commit 46eda5f
Command: cargo +nightly doc --open
Output:

warning: unresolved link to `absolute::LockTime`
   --> bitcoin/src/blockdata/locktime/absolute.rs:211:11
    |
211 |     /// [`absolute::LockTime`].
    |           ^^^^^^^^^^^^^^^^^^ no item named `absolute` in scope
    |
    = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default

warning: unresolved link to `relative::LockTime`
  --> bitcoin/src/blockdata/locktime/relative.rs:37:32
   |
37 |     /// Returns true if this [`relative::LockTime`] is satisfied by either height or time.
   |                                ^^^^^^^^^^^^^^^^^^ no item named `relative` in scope

warning: unresolved link to `relative::LockTime`
  --> bitcoin/src/blockdata/locktime/relative.rs:66:11
   |
66 |     /// [`relative::LockTime`].
   |           ^^^^^^^^^^^^^^^^^^ no item named `relative` in scope

warning: unresolved link to `relative::LockTime`
   --> bitcoin/src/blockdata/locktime/relative.rs:105:32
    |
105 |     /// Returns true if this [`relative::LockTime`] is satisfied by [`Height`].
    |                                ^^^^^^^^^^^^^^^^^^ no item named `relative` in scope

warning: unresolved link to `relative::LockTime`
   --> bitcoin/src/blockdata/locktime/relative.rs:131:32
    |
131 |     /// Returns true if this [`relative::LockTime`] is satisfied by [`Time`].
    |                                ^^^^^^^^^^^^^^^^^^ no item named `relative` in scope

warning: `bitcoin` (lib doc) generated 5 warnings

But no warnings with the docsrs flag. WTF.

@Kixunil
Copy link
Collaborator
Kixunil commented Dec 23, 2022

Oh, found it.

@Kixunil
Copy link
Collaborator
Kixunil commented Dec 23, 2022

See #1504

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bunch of doc warnings
3 participants
@tcharding @Kixunil @apoelstra
0