Skip to content
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.

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

Build the docs with test.sh #858

Merged
merged 3 commits into from Mar 12, 2022

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Mar 8, 2022

We currently build the docs as a separate CI job, we can however just do it as part of the Tests job using the nightly toolchain.

Conditionally build the docs based on a DO_DOCS env var.

Note, uses --cfg docsrs so can only be built run with nightly toolchain.

  • Patch 1: Fixes the incorrect file naming ci.sh -> test.sh in CONTRIBUTING.md.
  • Patch 2 - 4: Do trivial cleanup of test.sh.
  • Patch 5: Does the fix described above.

Resolves: #850

The test script is incorrectly named in our contributor docs. Fix it up
with the correct name.
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.

Should we really test docs on stable and not nightly with the docsrs feature?

@tcharding
Copy link
Member Author

Oh yes, cheers, that is much better. I'll patch rust-secp256k1 as well.

@tcharding
Copy link
Member Author

Changes in force-push:

  • Add RUSTDOCFLAGS="--cfg docsrs" to cargo docs build command
  • Add DO_DOCS=true to nightly toolchain instead of stable as required

Also re-worded PR description.

Some code has only two spaces of indentation, we favour 4 in bash
scripts.
We currently build the docs as a separate CI job, we can however just do
it as part of the `Tests` job using the nightly toolchain.

Conditionally build the docs based on a `DO_DOCS` env var.

Note, uses `--cfg docsrs` so can only be built run with nightly toolchain.
@tcharding
Copy link
Member Author

tcharding commented Mar 8, 2022

Changes in rebase: Remove the pinning changes because they were incorrect (and not anything to do with this PR).

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 0d36455

apoelstra added a commit to rust-bitcoin/rust-secp256k1 that referenced this pull request Mar 9, 2022
0fd07ad Improve CI pipeline (Tobin Harding)

Pull request description:

  We have unnecessary runs of the `test.sh` script. We can simplify the CI pipeline and at the same time improve the docs build by using `--cfg docsrs`.

  - Remove the `wasm` job, replace it by enabling the `DO_WASM` env var for the stable toolchain run in the `Tests` job.
  - Add `--cfg docrs` flag to the docs build and set the `DO_DOCS` env var as part of the nightly toolchain run in `Tests` job.

  The end result is one less run of the `test.sh` script and better test coverage.

  Idea came from @Kixunil when reviewing rust-bitcoin/rust-bitcoin#858, thanks.

ACKs for top commit:
  apoelstra:
    ACK 0fd07ad

Tree-SHA512: 063493ce03aa8cef5d7fc7636f3bfaaeff5c918d7076473bac23313082e8357d5282fcaf4d76a3dc5b0650e7ee43fa9d2b738f79863be7f24f2acf32f99da4b1
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 0d36455

@dr-orlovsky dr-orlovsky added the ci label Mar 12, 2022
Copy link
Collaborator

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK 0d36455

@dr-orlovsky dr-orlovsky merged commit a8c9ea6 into rust-bitcoin:master Mar 12, 2022
@tcharding tcharding deleted the 850-test-script branch March 15, 2022 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local tests script for contribution not executing cargo doc
4 participants