sharkdp / pastel

A command-line tool to generate, analyze, convert and manipulate colors
Apache License 2.0
5.08k stars 102 forks source link

Add GHA CICD #120

Closed rivy closed 4 years ago

rivy commented 4 years ago

This is a GitHub Actions workflow for CICD that I created for a couple of other repositories.

I've also added in automated debian packaging / deployment (see example at https://github.com/rivy/rs.pastel/releases/tag/v0.7.0.2).

Additional targets are easily added as well.

Fixes: #119

rivy commented 4 years ago

I added in several commits fixing cargo clippy lint complaints so that the style test builds pass.

They are currently logically separated so that you can more easily evaluate the refactoring decisions, but I can easily squash them into one if you'd prefer.

rivy commented 4 years ago

Oh, also, it's 5 to 10 times faster than Travis or AppVeyor. ⚡️

rivy commented 4 years ago

I think GHA is a complete solution and that AppVeyor and Travis can be discarded. But, I'd let this percolate for a few commits/PRs before removing them to make sure there's no problem or unmet need.

sharkdp commented 4 years ago

This was a quick conversion from another repository. I'll try to trim it down for you and update the PR tomorrow.

Thank you!

Oh, also, it's 5 to 10 times faster than Travis or AppVeyor.

That sounds fantastic :-)

rivy commented 4 years ago

Ok, I've slimmed the CICD code down (a bit). I think what's left is necessary, but please let me know if you disagree or any of it is unclear.

I replaced the commented Code Coverage section with a new alternate (working) version (using grcov) that tests *nix, macos, and windows (combining the coverage output; see https://codecov.io/gh/rivy/rs.pastel/commits).

I left a couple of "cleanup/polish" commits for the GHA:CI code because you might want to preserve them instead (especially, leaving the 'features' support might make a future self happier if you ever add any features).

I'm happy to squash down any/all of the cargo clippy complaint fix commits, as well, just leaving them separated so that you can review the changes associated with each complaint type (for discussion or alternate refactor).

Notably, all steps together, which now includes style checks and code coverage runs, take 8 minutes to complete (for comparison the Travis run takes 25 min, and the AppVeyor run was taking about 20 minutes).

The GHA:CI runs (from my repo) are available for review at https://github.com/rivy/rs.pastel/actions.

rivy commented 4 years ago

Looks like I have a bug in the way I'm testing MinSRV and that rust v1.35 doesn't like ...as_deref(). I'll fix both tomorrow.

rivy commented 4 years ago

... or tonight, I guess. 😄

To use .as_deref(), fixing a cargo clippy complaint, would raise the MinSRV to v1.40.

Instead, I'm using #[allow(clippy::option_as_ref_deref)] to fix the complaint. If you prefer using .as_deref() and raising the MinSRV to 1.40, let me know and I'll change it.

sharkdp commented 4 years ago

Ok, I've slimmed the CICD code down (a bit). I think what's left is necessary, but please let me know if you disagree or any of it is unclear.

That still seems like a massive amount of code/configuration. We currently have a ~90 line file .travis.yml file and we would move to a 320 line CICID.yml file.

It seems to do a lot more than our .travis.yml file, but I would really appreciate if we could start small and build up from there.

For example, there are a whole lot of echo … lines in this CICID.yml file. Are they required?

rivy commented 4 years ago

I can remove the Style checks, the MinSRV build/test, and all code coverage which would take the line count down to 218 lines. But I think it's valuable functionality that shouldn't be removed.

The echo ... lines are how outputs are propagated to subsequent CICD steps. They are doubled in the Initialize workflow variables step so that the reviewer can evaluate/observe the final variable values used by the build/test steps. I think they need to stay.

And, in looking at the Travis build, it's really about the same size as this CICD script (which has significantly more functionality and speed). The Travis build uses several scripts in the ./ci directory which add another 230 lines to the size of the build script, just bundled in a library directory.

I'm happy to run through what the CICD.yml is doing and why, in a step-wise manner, so that you can decide what you do and don't want to include. But, IMO, the current version has reasonable code size for the functionality (which is all useful).

sharkdp commented 4 years ago

Thank you for your work. I'd like to get this merged!

I can remove the Style checks, the MinSRV build/test, and all code coverage which would take the line count down to 218 lines. But I think it's valuable functionality that shouldn't be removed.

For now, I would really prefer a version without style checks and without clippy checks. I really love both cargo fmt and cargo clippy, but I disagree with the fact that they need to be enforced in CI. It's enough for me to run them manually from time to time. I don't want each and every PR and push to the repo to potentially fail because of trivial things detected by some static analysis tool.

Don't get me wrong. I would love to have such checks if this would be a high-performance library or something security relevant, etc.. But it's just a innocent little command line application which would rather profit from having low maintenance PRs/pushes.

As for MinSRV build/test... I don't even know what that is?

The echo ... lines are how outputs are propagated to subsequent CICD steps. They are doubled in the Initialize workflow variables step so that the reviewer can evaluate/observe the final variable values used by the build/test steps. I think they need to stay.

ok

And, in looking at the Travis build, it's really about the same size as this CICD script (with significantly more functionality and speed). The Travis build uses several scripts in the ./ci directory which add another 230 lines to the size of the build script, just bundled in a library directory.

Fair point!

rivy commented 4 years ago

For now, I would really prefer a version without style checks and without clippy checks. I really love both cargo fmt and cargo clippy, but I disagree with the fact that they need to be enforced in CI. It's enough for me to run them manually from time to time. I don't want each and every PR and push to the repo to potentially fail because of trivial things detected by some static analysis tool.

Don't get me wrong. I would love to have such checks if this would be a high-performance library or something security relevant, etc.. But it's just a innocent little command line application which would rather profit from having low maintenance PRs/pushes.

Fair enough, I'll yank the "Style" jobs from the CI.

As for MinSRV build/test... I don't even know what that is?

Sorry for the jargon ... "MinSRV" == "minimum supported rust version". It is debatable whether yoiu want to have this as a signal you want to send to other devs. If you don't want it, I can yank it as well.

I'll plan to give this a look again tomorrow or the day after and update based on your feedback.

sharkdp commented 4 years ago

Sorry for the jargon ... "MinSRV" == "minimum supported rust version".

Oh :smile:

It is debatable whether yoiu want to have this as a signal you want to send to other devs

No - a test for min. supported Rust version is great. Thanks!

rivy commented 4 years ago

Ok, I've dropped the 'Style' job, and removed the commit altering the float comparisons.

I've included some other minor updates for code coverage and i686-pc-windows-gnu builds as well.

I've grouped the *allow* commits together and I can drop them wholesale with the caveat that clippy will complain. The 15fb1fb commit can be dropped with no clippy complaint if I simultaneously raise the MinSRV to 1.40.0, which can be done easily.

All jobs are passing.

Let me know how you want me to handle the *allow* ... commits, and then I'll squash the commits (plan would be to squash down to about two or three commits), and re-force-push the PR.

sharkdp commented 4 years ago

I've grouped the *allow* commits together and I can drop them wholesale with the caveat that clippy will complain.

Let's leave the allow parts in. I guess I just have to get used to it.

The 15fb1fb commit can be dropped with no clippy complaint if I simultaneously raise the MinSRV to 1.40.0, which can be done easily.

I'm fine with raising to 1.40, thanks!

I'll squash the commits (plan would be to squash down to about two or three commits)

You don't have to, if it's up to me. I like your fine-grained commits :+1:

rivy commented 4 years ago

I'll have an updated PR pushed later tonight.

rivy commented 4 years ago

Ok, it's up.

toolchain has to be a nightly; pinned to minimize disruptions (as occurred just recently); will be a maintenance issue going forward; convert to "stable" when rust starts to include the necessary support in that toolchain (follow grcov repo for ongoing state)

sharkdp commented 4 years ago

I'm just now starting to realize how cool this is :sunglasses:

rivy commented 4 years ago

@rivy Thank you so much for all of this work! Is there anything else I need to set up in order to make the automatic deployment work?

No, it should just auto-deploy to releases section any time you push a "version tag", meaning any tag starting with a number (with an optional leading v), to the repository. The regex match is [Vv]?[0-9].*.

If it doesn't work, ping me and I'll look at it.

sharkdp commented 4 years ago

No, it should just auto-deploy to releases section any time you push a "version tag", meaning any tag starting with a number (with an optional leading v). The regex match is [Vv]?[0-9].*.

If it doesn't work, ping me and I'll look at it.

Thank you! I'll try to tag a new release soon.

rivy commented 4 years ago

If you have trouble with code coverage breaking the build, I submitted a new PR to alleviate that issue (#126).

sharkdp commented 4 years ago

The release process worked just fine. Thanks again.