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 lots of Github Actions CI jobs to check sanity #57

Merged
merged 7 commits into from
Jun 11, 2024
Merged

Add lots of Github Actions CI jobs to check sanity #57

merged 7 commits into from
Jun 11, 2024

Conversation

faern
Copy link
Member

@faern faern commented May 29, 2024

This crate has gone without any CI at all for a long time. We have no unit tests in this code currently. But we can and should lots of other stuff:

  • Linting
  • Formatting
  • Git commit message style
  • Does it build?
  • Does it have known vulnerabilities in dependencies?

This change is Reviewable

Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion


.github/workflows/build-and-test.yml line 71 at r1 (raw file):

      - name: Compile with minimal versions
        run: cargo +stable build --all-targets

Here I tried a slightly different approach than what I merged in mullvad/mnl-rs#14 and mullvad/pfctl-rs#89. I realized doing the cargo build step with nightly would be less stable. Nightly causes issues more frequently than stable. Only reason this job needs nightly at all is to do the -Z minimal-versions part.

Stable Rust is already available in the Github actions containers. So installing it is basically free anyway. The only reason I explicitly install it above instead of relying on it being installed is that when a new Rust version is shipped, it takes a while before the Github Actions containers are upgraded. By explicitly doing this we make sure we always use the same stable toolchain across the different CI jobs.

If we agree this is better I'll submit this improvement to the other two repositories as well.

@faern faern force-pushed the add-ci branch 2 times, most recently from 8cb9fcc to 4bc453f Compare June 4, 2024 07:26
@faern faern force-pushed the add-ci branch 2 times, most recently from d1ef44f to 4a84e9c Compare June 11, 2024 08:18
@faern faern marked this pull request as ready for review June 11, 2024 08:18
@faern faern requested a review from Serock3 June 11, 2024 08:18
Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 13 files reviewed, 1 unresolved discussion (waiting on @Serock3)


.github/workflows/build-and-test.yml line 46 at r2 (raw file):

      - name: Generate documentation
        if: matrix.rust == 'stable'
        run: RUSTDOCFLAGS="--deny warnings" cargo doc

FYI: The reason I decided to set RUSTDOCFLAGS here, but RUSTFLAGS is set up in the env section of the file is that RUSTDOCFLAGS is only relevant for this single cargo invocation, while the RUSTFLAGS is relevant to set for all the other jobs in this file. If we decide to move the rustdoc check to another workflow file, it would be harder to remember moving RUSTDOCFLAGS if it was specified somewhere completely different. Specifying RUSTFLAGS globally saves us from duplication, but the same is not true for the docs flag.

Copy link

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 13 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faern)


.github/workflows/build-and-test.yml line 71 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Here I tried a slightly different approach than what I merged in mullvad/mnl-rs#14 and mullvad/pfctl-rs#89. I realized doing the cargo build step with nightly would be less stable. Nightly causes issues more frequently than stable. Only reason this job needs nightly at all is to do the -Z minimal-versions part.

Stable Rust is already available in the Github actions containers. So installing it is basically free anyway. The only reason I explicitly install it above instead of relying on it being installed is that when a new Rust version is shipped, it takes a while before the Github Actions containers are upgraded. By explicitly doing this we make sure we always use the same stable toolchain across the different CI jobs.

If we agree this is better I'll submit this improvement to the other two repositories as well.

This seems like a sound argument to me


.github/workflows/build-and-test.yml line 41 at r2 (raw file):

      - name: Test
        run: cargo test

Should the tests not be run with the --locked flag to prevent cargo test from rebuilding of a bunch of deps?


.github/workflows/build-and-test.yml line 46 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

FYI: The reason I decided to set RUSTDOCFLAGS here, but RUSTFLAGS is set up in the env section of the file is that RUSTDOCFLAGS is only relevant for this single cargo invocation, while the RUSTFLAGS is relevant to set for all the other jobs in this file. If we decide to move the rustdoc check to another workflow file, it would be harder to remember moving RUSTDOCFLAGS if it was specified somewhere completely different. Specifying RUSTFLAGS globally saves us from duplication, but the same is not true for the docs flag.

Alright


.github/workflows/cargo-audit.yml line 27 at r2 (raw file):

      # This avoids significant maintenance work that provide no benefits.
      # We only need to make sure there is any compatible dependency without a known issue
      - run: cargo update

Is it necessary to run cargo update when we are not providing the file: Cargo.lock argument?


.github/workflows/linting.yml line 7 at r2 (raw file):

    paths:
      - .github/workflows/linting.yml
      - '**/*.rs'

I don't suppose we want to re-run clippy on changes to Cargo.toml? Update a dependency could cause a clippy lint to trigger from e.g. a deprecation, right?

Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Serock3)


.github/workflows/linting.yml line 7 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I don't suppose we want to re-run clippy on changes to Cargo.toml? Update a dependency could cause a clippy lint to trigger from e.g. a deprecation, right?

Deprecation is a rustc warning, not a clippy warning. Right? Those warnings should be emitted by cargo build

It's only used in examples, and upgrading it is required to make the CI
build, due to bugs in ipnetwork 0.16
We realized that libraries should probably not check for CVEs.
It will generate too many false positives and provide very little value.
It's up to downstream *program* developers to select exact versions
of transitive dependencies. If it ends up being that no version of one
of our dependencies is safe/works, then that program developer must
report to this library that we should probably consider
upgrading/replacing that dependency with something better.
@faern faern merged commit b951ed2 into main Jun 11, 2024
8 of 9 checks passed
@faern faern deleted the add-ci branch June 11, 2024 15:21
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.

2 participants