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: fix deprecations, MSRV tests. #30

Merged
merged 9 commits into from
Sep 7, 2023
Merged

Conversation

cpu
Copy link
Member

@cpu cpu commented Sep 1, 2023

Description

This branch resolves the remaining GitHub warnings from the actions-rs tasks. It also appears that the MSRV related tests weren't working properly in CI, masking some failures that were uncovered with the switch to dtolnay/rust-toolchain.

I recommend reviewing this commit-by-commit. The top-level highlights:

  1. replaces actions-rs with dtolnay/rust-toolchain.
  2. splits out a MSRV clippy job so we can customize it more easily.
  3. adds a MSRV-specific clippy alias to only test the --lib target.
  4. changes the project MSRV from 1.56 -> 1.64 based on our experiences trying to get a workable solution for cargo ndk and transitive deps.
  5. switches the MSRV clippy job to use cargo-ndk 2.12.7, the latest release compatible with a MSRV of 1.64.
  6. fixes some unnecessary lazy evaluations clippy findings.
  7. adds a cron trigger and support for the merge queue feature.

Resolves #26

.cargo/config.toml Outdated Show resolved Hide resolved
@complexspaces
Copy link
Collaborator

complexspaces commented Sep 1, 2023

Opening this as a draft because I've run out of time for this today and the MSRV build is still failing.

The remaining failure appears to be because memchr is using target_feature with aarch64, something that was only stabilized as of 1.61, despite them claiming to support 1.60:

This crate's minimum supported rustc version is 1.60.0.

EDIT: Filed an upstream issue about this: BurntSushi/memchr#136

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
The former is not maintained, and produces warnings in GitHub Actions
due to node-js deprecations.

The latter is used by the other Rustls ecosystems projects and
actively maintained.
Previously there was one job that ran clippy across all supported
operating systems for both the latest stable, and the MSRV Rust
toolchains.

This commit splits that job into two: one for stable and one for the
MSRV toolchain. This will make it easier to customize MSRV specific
settings in a subsequent commit.
This commit adds a variation of the existing `clippy-ci` alias that only
tests the `--lib` target. This is intended to be used when linting with
the MSRV, where we don't want dev dependencies and other non-essential
code being linted with the MSRV toolchain.
Unfortunately Rust 1.64.0 is the oldest toolchain version that we can
get working with cargo-ndk, which is required for building *ring* for
the Android targets.

The prior MSRV was not being tested correctly in CI and so wasn't
actually compatible with the crate as it stands today.
With an MSRV of 1.64.0 we can use the latest cargo-ndk release in the
v2.12.x stream when doing our MSRV testing for the Android target. The
3.0.x and 3.1.x streams require a more aggressive MSRV than we can
support at this time.
Clippy is flagging a couple instances of this:
```
error: unnecessary closure used with `bool::then`
   --> src/verification/android.rs:251:18
    |
251 |         .map(|o| (!o.is_null()).then(|| o))
    |                  ^^^^^^^^^^^^^^^----------
    |                                 |
    |                                 help: use `then_some(..)` instead: `then_some(o)`
    |
```

This commit applies the recommended fix.
This commit adds a cron schedule to the CI workflow so we can catch
things like new clippy breakages sooner.
This will allow using the merge queue feature.
@cpu cpu marked this pull request as ready for review September 7, 2023 14:34
@cpu cpu merged commit ce16ccc into rustls:main Sep 7, 2023
13 checks passed
@cpu cpu deleted the cpu-fix-ci-warns_dev branch September 7, 2023 16:11
@cpu
Copy link
Member Author

cpu commented Sep 7, 2023

Thanks for the reviews/input on this work folks. It ended up being hairier than I expected! 😅

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.

GitHub CI using deprecated actions
3 participants