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

Add rustdoc CI check #5038

Closed
calebcartwright opened this issue Oct 22, 2021 · 3 comments · Fixed by #5041
Closed

Add rustdoc CI check #5038

calebcartwright opened this issue Oct 22, 2021 · 3 comments · Fixed by #5041
Labels
good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted

Comments

@calebcartwright
Copy link
Member

Now that the builds in rust-lang/rust are documenting rustfmt as well (rust-lang/rust#87119), we should update our build process to ensure we're not introducing anything that will trigger warnings since such warnings will be hard errors in rust-lang/rust, and they won't show until bors tests the changes (refs rust-lang/rust#90087)

Other than this check needing to be added as part of a GitHub Actions workflow and executed on PRs, it doesn't really matter to me which file the job is defined within nor how it's implemented.

We won't be able to run it exactly the same in our CI as it's executed in rust-lang/rust CI, but should be able to get sufficiently close. The specific args/flags utilized in the compiler build can be derived from here

Note that the rustdoc flags can be provided as an environment variable (e.g. RUSTDOCFLAGS="--document-private items..."), and be sure to include -D warningsto match the behavior in the compiler builds. Other than that the command should be something simple along the lines ofcargo doc -p rustfmt-nightly -p rustfmt-config_proc_macro` along with the rest of the other flags/args

@calebcartwright calebcartwright added good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted labels Oct 22, 2021
@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2021

Would adding another job to .github/workflows/linux.yaml work, or would you prefer a new workflow file be created? something like this:

  rustdoc_check:
    runs-on: ubuntu-latest
    name: (${{ matrix.target }}, nightly)
    strategy:
      max-parallel: 1
      fail-fast: false
      matrix:
        target: [
          x86_64-unknown-linux-gnu,
        ]
    steps:
    - name: checkout
      uses: actions/checkout@v2

      # Run build
    - name: install rustup
      run: |
        curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > rustup-init.sh
        sh rustup-init.sh -y --default-toolchain none
        rustup target add ${{ matrix.target }}

    - name: run cargo doc
      env:
        RUSTFLAGS: -D warnings
        RUSTDOCFLAGS: --document-private-items --enable-index-page --show-type-layout --generate-link-to-definition -Zunstable-options
      run: cargo doc -Zskip-rustdoc-fingerprint --no-deps -p rustfmt-nightly -p rustfmt-config_proc_macro

@calebcartwright
Copy link
Member Author

That looks good to me, though we don't need to worry about a matrix-based job for the doc check.

Will defer to you on which file, I don't really have a preference. One of the motivating factors for separating out the linux/windows/mac jobs into separate workflows is that it makes status reporting and status badges more feasible for the platforms individually. Beyond that I think the same file vs. new file for any subsequent jobs will just boil down to whatever we think will work best. I guess we do already have the integration function in a separate file/workflow, so maybe this warrants a separate workflow/file for pattern consistency?

@ytmimi
Copy link
Contributor

ytmimi commented Oct 23, 2021

That looks good to me, though we don't need to worry about a matrix-based job for the doc check.

I added the matrix because I wanted to be consistent with the way the linux test jobs were setup, but good to note!

I guess we do already have the integration function in a separate file/workflow, so maybe this warrants a separate workflow/file for pattern consistency?

I like the idea of placing the Doc CI job into its own file. It also feels more consistent to me. I'll open a PR soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants