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

ci: add cargo-check-external-types check #4914

Merged
merged 1 commit into from
Aug 24, 2022
Merged

ci: add cargo-check-external-types check #4914

merged 1 commit into from
Aug 24, 2022

Conversation

jdisanti
Copy link
Contributor

Motivation

This PR adds cargo-check-external-types to CI for #4902.

Solution

A new check-external-types CI step is added which pulls down a pinned nightly version of Rust, installs cargo-check-external-types, and then runs it against an external-types.toml config file which allow-lists types that are currently exposed in Tokio's public API.

If a PR exposes additional types that aren't listed in the config, then the CI job will output an error. To illustrate what this looks like, the following is the output if the libc entries are removed from external-types.toml:

error: Unapproved external type `libc::unix::uid_t` referenced in public API
  --> tokio/src/net/unix/ucred.rs:16:5
   |
16 |     pub fn uid(&self) -> uid_t {
   | ...
18 |     }␊
   |     ^
   |
   = in return value of `tokio::net::unix::UCred::uid`

error: Unapproved external type `libc::unix::gid_t` referenced in public API
  --> tokio/src/net/unix/ucred.rs:21:5
   |
21 |     pub fn gid(&self) -> gid_t {
   | ...
23 |     }␊
   |     ^
   |
   = in return value of `tokio::net::unix::UCred::gid`

error: Unapproved external type `libc::unix::pid_t` referenced in public API
  --> tokio/src/net/unix/ucred.rs:29:5
   |
29 |     pub fn pid(&self) -> Option<pid_t> {
   | ...
31 |     }␊
   |     ^
   |
   = in generic arg of `tokio::net::unix::UCred::pid`

3 errors emitted

@jdisanti
Copy link
Contributor Author

jdisanti commented Aug 15, 2022

It looks like the miri nightly version was never pinned, so #4903 didn't actually pin/test it at nightly-2022-07-25, and that specific nightly doesn't have the miri component. I'll see if I can find another nightly version that satisfies everything.

@carllerche
Copy link
Member

Well, I guess the bad news is we have leaked some libc types. The good news is they are all aliases for std types, so we should be able to redefine them and remove those failures.

For example:

pub type uid_t = i32;
pub type gid_t = u32;
pub type pid_t = i32;

This isn't 100% correct as it looks like uid_t / gid_t are defined differently depending on target_os and some other factors...

@jdisanti
Copy link
Contributor Author

Created #4916 to discuss and track removing the libc types from the public API. I'll add a TODO comment to the config file in this PR linking to that issue for now.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@taiki-e
Copy link
Member

taiki-e commented Aug 17, 2022

Also, since we have Windows-specific APIs, it would probably make sense to do this check for Windows as well.

@jdisanti
Copy link
Contributor Author

It looks like the Tokio Windows features have a raw pointer in the public API, which I hadn't added support for yet since we haven't used them in our projects. I'm going to take a pass on all the unimplemented!() blocks in the tool and make sure those are all taken care of so that this doesn't happen when iterating after it's added to CI.

@taiki-e
Copy link
Member

taiki-e commented Aug 17, 2022

@jdisanti: I just opened smithy-lang/smithy-rs#1643 to support raw ptr

@jdisanti
Copy link
Contributor Author

I released version 1.3 of cargo-check-external-types to fix a bug discovered around re-exporting external types. The changes since approval are just that version bump and rebasing.

@Darksonn
Copy link
Contributor

Thanks!

@Darksonn Darksonn merged commit d720770 into tokio-rs:master Aug 24, 2022
@Darksonn Darksonn added the A-ci Area: The continuous integration setup label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ci Area: The continuous integration setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants