diskonaut icon indicating copy to clipboard operation
diskonaut copied to clipboard

Add GitHub Actions (GHA) CICD workflow

Open rivy opened this issue 5 years ago • 4 comments

The PR adds a complete GHA CICD workflow:

  • includes multiple linux, macos, and windows platform builds and testing
  • includes automated packaging and publishing/deployment to GitHub Releases when commit is version tagged
  • includes Style testing (both cargo fmt and cargo clippy warnings); note: warnings add notations but don't break the workflow build
    • there are about 9 distinct cargo clippy warnings generated for the current project code
  • includes test for minimum rust supported version (aka, MinSRV, MSRV, MinRustV); easily removed if not desired
  • includes automatic CodeCov coverage reporting (if/when testing is included); also, easily removed if not desired
    • NOTE: several of the usual code coverage compiler flags cause test errors for this project so coverage here is using only -Zprofile, and coverage looks to be skewed (see notes below); I don't have the project knowledge to fix the test failures.

I've been doing some initial experimentation with your project, and I wanted to contribute some automation that might be helpful to you going forward. This contributed workflow is a slightly modified form based on other GHA CICD workflows that I've designed (and are in current use) for uutils/coreutils, bootandy/dust, Peltoche/lsd, sharkdp/bat, sharkdp/pastel.

I'm happy to make any changes that you would find useful as improvements.

Code coverage notes

For code coverage, the usual flag setting is RUSTFLAGS: '-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off -Zpanic_abort_tests -Cpanic=abort', but that combination causes compilation failure with:

error[E0463]: can't find crate for `proc_macro_error_attr`
   --> C:\Users\Roy\.cargo\registry\src\github.com-1285ae84e5963aae\proc-macro-error-1.0.2\src\lib.rs:266:9
    |
266 | pub use proc_macro_error_attr::proc_macro_error;
    |         ^^^^^^^^^^^^^^^^^^^^^ can't find crate

error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.
error: could not compile `proc-macro-error`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error[E0463]: can't find crate for `failure_derive`
  --> C:\Users\Roy\.cargo\registry\src\github.com-1285ae84e5963aae\failure-0.1.8\src\lib.rs:56:1
   |
56 | extern crate failure_derive;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't find crate

error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.
error: build failed

Using RUSTFLAGS: '-Zprofile -Ccodegen-units=1 -Clink-dead-code -Coverflow-checks=off', tests compile but only one test passes:

...

---- tests::cases::ui::zoom_into_small_files stdout ----
---- tests::cases::ui::zoom_into_small_files stderr ----
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: "tests run with disabled concurrency, automatic snapshot name generation is not supported.  Consider using the \"backtrace\" feature of insta which tries to recover test names from the call stack."', C:\Users\Roy\.cargo\registry\src\github.com-1285ae84e5963aae\insta-0.16.0\src\runtime.rs:863:22
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    tests::cases::ui::cannot_move_into_small_files
    tests::cases::ui::cant_delete_file_with_term_too_small
    tests::cases::ui::clear_selection_when_moving_off_screen_edges
    tests::cases::ui::delete_file
    tests::cases::ui::delete_file_no_confirmation
    tests::cases::ui::delete_file_press_n
    tests::cases::ui::delete_folder
    tests::cases::ui::delete_folder_no_confirmation
    tests::cases::ui::delete_folder_small_window
    tests::cases::ui::delete_folder_small_window_no_confirmation
    tests::cases::ui::delete_folder_with_multiple_children
    tests::cases::ui::delete_folder_with_multiple_children_no_confirmation
    tests::cases::ui::eleven_files
    tests::cases::ui::empty_folder
    tests::cases::ui::enter_folder
    tests::cases::ui::enter_folder_medium_width
    tests::cases::ui::enter_folder_small_width
    tests::cases::ui::enter_largest_folder_with_no_selected_tile
    tests::cases::ui::esc_to_go_up
    tests::cases::ui::files_with_size_zero
    tests::cases::ui::medium_width
    tests::cases::ui::minimum_tile_sides
    tests::cases::ui::move_down_and_enter_folder
    tests::cases::ui::move_left_and_enter_folder
    tests::cases::ui::move_right_and_enter_folder
    tests::cases::ui::move_up_and_enter_folder
    tests::cases::ui::noop_when_entering_file
    tests::cases::ui::noop_when_pressing_esc_at_base_folder
    tests::cases::ui::pressing_delete_with_no_selected_tile
    tests::cases::ui::small_files
    tests::cases::ui::small_files_with_x_as_zero
    tests::cases::ui::small_files_with_y_as_zero
    tests::cases::ui::small_width
    tests::cases::ui::small_width_long_folder_name
    tests::cases::ui::too_small_height
    tests::cases::ui::too_small_width_five
    tests::cases::ui::too_small_width_four
    tests::cases::ui::too_small_width_one
    tests::cases::ui::too_small_width_three
    tests::cases::ui::too_small_width_two
    tests::cases::ui::two_large_files_one_small_file
    tests::cases::ui::zoom_into_small_files

test result: FAILED. 1 passed; 42 failed; 0 ignored; 0 measured; 0 filtered out

Using RUSTFLAGS: '-Zprofile -Clink-dead-code -Coverflow-checks=off', tests compile but two test failures occur:

failures:
tests::cases::ui::delete_file_no_confirmation
tests::cases::ui::delete_folder_no_confirmation

test result: FAILED. 41 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out

rivy avatar Oct 04 '20 03:10 rivy

Hey @rivy - I'm sorry it's taking me some time to get to this. Going through some busy days. I promise to review this soon.

imsnif avatar Oct 07 '20 17:10 imsnif

No worries. I appreciate the response, but take your time. I'm in no rush. Let me know if you need changes.

rivy avatar Oct 07 '20 18:10 rivy

Hey @rivy - thanks for your patience! I'm finally getting to this. :)

Because of the nature of this change, I think it's going to require some back-and-forth between us. I hope you're up to it, and I promise not to take so long to reply from now on.

So a few things:

  1. I'm not a big fan of test coverage reporting, in all honesty. While it's a good tool for manually checking what one might be missing in their tests, I feel that publishing it can create blind-spots and overconfidence. So I think I'd rather not include it.
  2. Elsewhere, I've seen that it's possible to trigger releases manually rather than with a commit regex. I greatly prefer that approach because it gives me finer control over the release process. Would changing it be a great deal of work?
  3. I really like that the releases would include prebuilt binaries for windows and mac. I feel that would be very meaningful to new users wanting to try out diskonaut. I think I would prefer to have just 1 file per OS in order to make things simpler for those looking to download the right version. What do you think?
  4. I'm aware of the clippy warnings unfortunately. :/ That's why I didn't include the check originally when releasing diskonaut. Some of them aren't very trivial to fix iirc. Could we include this check in a disabled mode somehow and when I or someone else gets to fixing them we can enable it? (If you're feeling adventurous and want to fix them, I'd be happy to guide you through what I think needs to be done - but no stress of course).

Otherwise, this looks really cool and I'm looking forward to using it when releasing. Thanks for doing this!

imsnif avatar Oct 15 '20 08:10 imsnif

Sorry for the year-long delay! Your reply got lost in the stack and I just saw it recently when I was reviewing old PRs.

I've gone ahead and made changes following your requests.

  1. I'm not a big fan of test coverage reporting, in all honesty. While it's a good tool for manually checking what one might be missing in their tests, I feel that publishing it can create blind-spots and overconfidence. So I think I'd rather not include it.

Disabled using comments in a specific and described commit so that it's there if you want to re-enable it in the future.

As an aside, there is also a style spell-check, using cspell, that I've left in (again, disabled by comments) for the possibility that you might want to use it in the future. It would require some configuration work on your part.

  1. Elsewhere, I've seen that it's possible to trigger releases manually rather than with a commit regex. I greatly prefer that approach because it gives me finer control over the release process. Would changing it be a great deal of work?

This workflow already supports controlled (ie, manual) releases via tags. It's not gated on commit matches but upon tag matches. If you don't push a release tag (ie, matches /[vV]?[0-9].*/), no release will be deployed. So, you will always have full control of the releases.

Using GHA workflow dispatch to enable manually triggering the CICD workflow via UI would require an involved design refactor and, likely, a lot of redundant code without adding any capabilities.

I do think you'll find the current implementation will meet your request.

  1. I really like that the releases would include prebuilt binaries for windows and mac. I feel that would be very meaningful to new users wanting to try out diskonaut. I think I would prefer to have just 1 file per OS in order to make things simpler for those looking to download the right version. What do you think?

I added a "withhold" parameter to the build matrix which suppresses publishing those marked packages for release. The configuration offered here publishes the i686 (x32) packages (as more widely usable) for Windows and Linux while suppressing the respective x86_64 (x64) packages; see https://github.com/rivy/rs.diskonaut/releases. Alternatively, you could just comment out the configurations in the job matrix to avoid building those packages altogether. But, IMO, building and testing those configurations makes your project much more robust, so I'd leave them to build/test.

  1. I'm aware of the clippy warnings unfortunately. :/ That's why I didn't include the check originally when releasing diskonaut. Some of them aren't very trivial to fix iirc. Could we include this check in a disabled mode somehow and when I or someone else gets to fixing them we can enable it? (If you're feeling adventurous and want to fix them, I'd be happy to guide you through what I think needs to be done - but no stress of course).

My OCD was tingling and wouldn't let me leave the warnings in, so I fixed them all. The code now compiles without warnings and cargo clippy --all-targets has no complaints. The fixes are in separate commits so that you can review them easily.

I also rebased the changes on top of the current main branch.

Hope you like it! Let me know if you need help with any changes.

I'll keep a better eye out this time.

rivy avatar Oct 10 '21 19:10 rivy

I guess we missed each other... closing.

rivy avatar Oct 29 '22 14:10 rivy