Skip to content

[ci] fix docsrs error for bdk crate - #1011

Merged
notmandatory merged 1 commit into
bitcoindevkit:masterfrom
notmandatory:fix/docsrs
Jul 4, 2023
Merged

[ci] fix docsrs error for bdk crate#1011
notmandatory merged 1 commit into
bitcoindevkit:masterfrom
notmandatory:fix/docsrs

Conversation

@notmandatory

@notmandatory notmandatory commented Jun 20, 2023

Copy link
Copy Markdown
Member

Description

Fixes #955

Notes to the reviewers

Changelog notice

none

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@notmandatory notmandatory self-assigned this Jun 20, 2023
@notmandatory
notmandatory force-pushed the fix/docsrs branch 5 times, most recently from a8fe510 to 789cb99 Compare June 20, 2023 19:06
@notmandatory
notmandatory marked this pull request as ready for review June 20, 2023 19:11
@notmandatory notmandatory added this to the 1.0.0-alpha.1 milestone Jun 20, 2023
@thunderbiscuit
thunderbiscuit self-requested a review June 20, 2023 19:13

@thunderbiscuit thunderbiscuit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK 789cb99.

Comment thread crates/bdk/Cargo.toml Outdated
name = "mnemonic_to_descriptors"
path = "examples/mnemonic_to_descriptors.rs"
required-features = ["all-keys"]
doc-scrape-examples = true

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've never heard of this feature before. Interested to see how this works. Unfortunately our new examples are in their own crates so this won't work there.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

At least the bdk crate still has some code in it's own examples, and in the future it would be nice to put some simple examples in all the crates just to get the code added to the docs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nevermind, I removed the doc-scrape-examples feature, can add back later if we want it.

- name: Build docs
run: cargo doc --no-deps
env:
RUSTDOCFLAGS: '--cfg docsrs -Dwarnings'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

shouldn't you keep -Dwarnings.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The -Dwarnings isn't in the command that docs.rs uses so I left it off. I did confirm that with the new command string I added catches the feature(doc_cfg) issue when run locally or remotely. Also I can't see where in the docs that -Dwarnings is a flag that rustdoc cares about.
https://doc.rust-lang.org/cargo/commands/cargo-rustdoc.html

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correction: it looks like --cfg docsrs and -Dwarnings are both flags passed through to rustc. I'm pretty sure other jobs like clippy will fail first, but doesn't hurt to add it here also. I added it back as part of the build.rustdocflags and force pushed.

@notmandatory notmandatory Jun 21, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I rolled back all changes except the one I think really mattered which was adding #![cfg_attr(docsrs, feature(doc_cfg))] to bdk crate lib.rs.

If that's not enough to fix docs.rs I'll take another stab at it for the next alpha.

@notmandatory

Copy link
Copy Markdown
Member Author

Ugh. Now with the -Dwarnings I seem to be getting a new error that I can't repro running the same command as on CI locally. Will keep looking into it tomorrow.

@notmandatory
notmandatory force-pushed the fix/docsrs branch 10 times, most recently from e365778 to f82be73 Compare June 22, 2023 02:38
@notmandatory

Copy link
Copy Markdown
Member Author

@LLFourn I simplified this down to just what should be needed to fix the original issue that was breaking the docs.rs rustdocs. Please approve if OK with you, this is the last thing before we can cut the alpha.1 release.

@LLFourn LLFourn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM thanks @notmandatory

@notmandatory
notmandatory merged commit 81c7613 into bitcoindevkit:master Jul 4, 2023
@notmandatory notmandatory mentioned this pull request Jul 4, 2023
24 tasks
@notmandatory
notmandatory deleted the fix/docsrs branch May 26, 2025 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

docs don't build on docs.rs for 1.0.0-alpha.0

3 participants