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

feat(CI): add external type check #2957

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ethanburrell
Copy link

Adding audit for issue #2925.

Uses the nightly build to uses rustdoc to create a diff with the exported types.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on!

Cargo.toml Outdated
@@ -45,6 +45,7 @@ libc = { version = "0.2", optional = true }
socket2 = { version = "0.4", optional = true }

[dev-dependencies]
cargo-check-external-types = "0.1.2"
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to make the dependency only installed in the CI workflow? So that most local dev doesn't need to download the extra dep?

@ethanburrell
Copy link
Author

@seanmonstar Changed pipeline to install the dependency within the pipeline.

@seanmonstar
Copy link
Member

Looks like the new job failed, can't find:

Unable to resolve action actions-rs/install@v1, unable to find version v1

uses: actions/checkout@v1

- name: Install Rust
uses: actions-rs/toolchain@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

This action is unmaintained. It seems like people are coalescing around dtolnay/rust-toolchain as an alternative solution.

runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the latest version of this action. Older versions of this action depend on node versions that are deprecated in Github's runners.

Suggested change
uses: actions/checkout@v1
uses: actions/checkout@v3

uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: nightly
Copy link
Contributor

Choose a reason for hiding this comment

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

We should pin to a specific combination of nightly version/cargo-check-external-types that's known to work. The newest nightly version might introduce a new version of rustdoc JSON output that's incompatible with the version expected by cargo-check-external-type.

It'd be nicer if we could install the toolchain version listed in their toolchain file, but that's probably not necessary to get this PR in.

override: true

- name: Install cargo-check-external-types
uses: actions-rs/install@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue as actions-rs/toolchain- I'd consider dtolnay/install instead.

use-tool-cache: true

- name: check-external-types
uses: actions-rs/cargo@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue as the other actions-rs/* actions- I think here we can just invoke cargo manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants