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: use -pcoreutils when running clippy #6247

Merged

Conversation

cakebaker
Copy link
Contributor

@cakebaker cakebaker commented Apr 17, 2024

While looking at #6246 I wondered why those clippy warnings were not caught by our CI. The reason is probably that ${{ matrix.job.cargo-options }} is never defined and so I replaced it with ${{ steps.vars.outputs.CARGO_FEATURES_OPTION }}. I couldn't figure out the reason and my two attempts to make the CI fail on those warnings failed. And so this PR is just about removing the undefined ${{ matrix.job.cargo-options }}. It looks like the reason is that we select the utils for the specific platform and then run clippy on those packages (which don't contain the tests). By running clippy on the coreutils package with the --features flag, the tests are also checked by clippy.

@cakebaker cakebaker force-pushed the ci_clippy_use_cargo_features_option branch from b58484b to 92ee1f3 Compare April 17, 2024 06:03
@cakebaker
Copy link
Contributor Author

Changes since last push:

  • changed ${{ steps.vars.outputs.CARGO_FEATURES_OPTION }} to --all-features as the original approach didn't work

@cakebaker cakebaker marked this pull request as draft April 17, 2024 06:09
@cakebaker cakebaker force-pushed the ci_clippy_use_cargo_features_option branch from 92ee1f3 to 7c1232c Compare April 17, 2024 08:44
@cakebaker
Copy link
Contributor Author

Changes since last push:

  • removed --all-features

@cakebaker cakebaker changed the title ci: use CARGO_FEATURES_OPTION when running clippy ci: remove undefined matrix.job.cargo-options Apr 17, 2024
@cakebaker cakebaker marked this pull request as ready for review April 17, 2024 08:58
@cakebaker cakebaker force-pushed the ci_clippy_use_cargo_features_option branch from 7c1232c to f2ffb78 Compare April 17, 2024 14:19
@cakebaker
Copy link
Contributor Author

Changes since last push:

  • removed unused CARGO_FEATURES_OPTION from style_format job
  • used --features ${{ matrix.job.features }} -pcoreutils instead of ${{ steps.vars.outputs.CARGO_UTILITY_LIST_OPTIONS }} when running clippy
  • removed code used to set CARGO_UTILITY_LIST_OPTIONS

@cakebaker cakebaker changed the title ci: remove undefined matrix.job.cargo-options ci: use -pcoreutils when running clippy Apr 17, 2024
@cakebaker
Copy link
Contributor Author

Changes since last push:

@cakebaker cakebaker force-pushed the ci_clippy_use_cargo_features_option branch from 79e370d to de08536 Compare April 17, 2024 15:33
@cakebaker
Copy link
Contributor Author

Changes since last push:

  • moved unit tests from tests/by_util/test_env.rs to src/uu/env/src/env.rs
  • fixed the fixes from the previous push

@cakebaker cakebaker force-pushed the ci_clippy_use_cargo_features_option branch from de08536 to 347a387 Compare April 17, 2024 15:50
@cakebaker
Copy link
Contributor Author

Changes since last push:

  • fixed "the borrowed expression implements the required traits" warnings in the env tests introduced with the previous push

@uutils uutils deleted a comment from github-actions bot Apr 17, 2024
@uutils uutils deleted a comment from github-actions bot Apr 17, 2024
@uutils uutils deleted a comment from github-actions bot Apr 17, 2024
Copy link

GNU testsuite comparison:

GNU test failed: tests/cp/same-file. tests/cp/same-file is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/cp/cross-dev-symlink. tests/cp/cross-dev-symlink is passing on 'main'. Maybe you have to rebase?

@uutils uutils deleted a comment from github-actions bot Apr 17, 2024
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Does what it says on the tin, and evidently you "tested" it by finding actual problems in the code. Yay! :)

Do you know why the Windows CI fails? This doesn't look like a flake:

$ cargo nextest run --profile ci --hide-progress-bar --features=windows -p uucore -p coreutils
<SNIP downloading and compiling>
   Compiling rstest v0.19.0
   Compiling unindent v0.2.1
   Compiling hex-literal v0.4.1
    Finished `test` profile [unoptimized + debuginfo] target(s) in 1m 56s
error: creating test list failed

Caused by:
  for `coreutils::bin/coreutils`, command `'D:\a\coreutils\coreutils\target\debug\deps\coreutils-08f81a256ac17834.exe' --list --format terse` exited with code 0xc0000135: The specified module could not be found. (os error 126)
--- stdout:

--- stderr:

---
Error: Process completed with exit code 1.

@cakebaker
Copy link
Contributor Author

@BenWiederhake it looks like it was a temporary issue

Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Looks still good 👍

@BenWiederhake BenWiederhake merged commit c83cec7 into uutils:main Apr 20, 2024
64 of 65 checks passed
@cakebaker cakebaker deleted the ci_clippy_use_cargo_features_option branch April 21, 2024 08:35
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