Add GHA CICD #120

rivy · 2020-03-10T21:44:25Z

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

adds GitHub-Actions CICD workflow (includes arm-linux, x86-linux, macos, and windows builds/testing)
includes automatic deployment to Github Releases when commit is version tagged (see example at https://github.com/rivy/rs.pastel/releases/tag/v0.7.0.1; note: fully automatically generated)

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 · 2020-03-10T21:48:04Z

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 · 2020-03-10T22:44:05Z

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

sharkdp

Thank you very much for your contribution!

In general, this looks great! 👍

That being said, I would really prefer the CI/CD file to be as simple as possible. At the moment, there is A LOT of code and some of it doesn't seem to be strictly necessary. There are also a lot of commented-out lines which I would prefer to be removed, if possible.

I know that you already put a lot of work into this, but I need to consider the fact that I might have to maintain this myself in the future. And for now, I'm not very familiar with GitHub Actions (although it sounds like a great idea to use this in the future).

Should we remove the AppVeyor/Travis CI files? Is there anything that is not supported in GitHub Actions?

.github/workflows/CICD.yml

rivy · 2020-03-17T04:50:49Z

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 · 2020-03-17T21:46:29Z

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 · 2020-03-18T04:09:15Z

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 · 2020-03-18T04:29:14Z

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 · 2020-03-18T04:52:30Z

... 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 · 2020-03-20T20:07:58Z

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 · 2020-03-23T00:16:02Z

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 · 2020-04-24T13:16:06Z

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!

src/ansi.rs

sharkdp · 2020-04-24T13:18:28Z

src/distinct.rs

-                    result.closest_pair.0
-                } else {
-                    if self.rng.gen() {
+                #[allow(clippy::collapsible_if)]


similar here

sharkdp · 2020-04-24T13:20:25Z

src/lib.rs

@@ -868,7 +871,7 @@ impl ColorScale {
        let same_position = self
            .color_stops
            .iter_mut()
-            .find(|c| position.value() == c.position.value());
+            .find(|c| (position.value() - c.position.value()).abs() < std::f64::EPSILON);


no!! the == check on floats is actually desired here. Using an epsilon check would be wrong.

This is one of the reasons why I don't like to have clippy warnings checked by default.

Commit dropped (reverted).

src/parser.rs

sharkdp · 2020-04-24T13:22:05Z

src/types.rs

@@ -15,7 +15,7 @@ impl Hue {

    /// Return a hue value in the interval [0, 360].
    pub fn value(self) -> Scalar {
-        if self.unclipped == 360.0 {
+        if (self.unclipped - 360.0).abs() < std::f64::EPSILON {


Same here. Please leave the == check in place. This can lead to wrong results (angles larger 360.0 degrees)

Commit dropped (reverted).

rivy · 2020-04-28T03:27:46Z

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 · 2020-04-28T09:42:30Z

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

Oh 😄

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 · 2020-04-29T01:15:30Z

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 · 2020-04-29T08:41:21Z

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 👍

rivy · 2020-05-01T16:19:37Z

I'll have an updated PR pushed later tonight.

…times)

… * MinSRV to '1.40.0' - `.as_deref()` was stabilized in rust v1.40.0; * requires MinSRV => '1.40.0' - ref: <https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1400-2019-12-19>

…names)

…for windows)

…uired stable == rust v1.43.0+)

…nightly toolchain ## [why] Code coverage must currently use some unstable features in nightly rust builds. The nightly builds are, by definition, unstable and subject to frequent breaking changes. To prevent CI build breakage, the toolchain is pinned to a specific known working set. Note: (maint!) this will require periodic review until code coverage is more fully implemented/integrated into Rust and moved into the stable channel. - refs: <mozilla/grcov#427>, <newsboat/newsboat#916>

rivy · 2020-05-02T19:40:48Z

Ok, it's up.

rebased onto master
local (successful) GHA test run
minor change to Code Coverage

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)

cleanup/polish of issues discussed above

sharkdp

@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?

sharkdp · 2020-05-24T16:37:44Z

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

rivy · 2020-05-24T21:20:46Z

@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 · 2020-05-24T21:21:56Z

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 · 2020-05-24T22:21:58Z

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

sharkdp · 2020-05-25T18:53:28Z

The release process worked just fine. Thanks again.

sharkdp reviewed Mar 15, 2020

View reviewed changes

.github/workflows/CICD.yml Outdated Show resolved Hide resolved

rivy force-pushed the add.gha-cicd branch from 52302ae to bdd394c Compare March 18, 2020 04:09

rivy force-pushed the add.gha-cicd branch from bdd394c to 0caebc6 Compare March 18, 2020 04:55

sharkdp reviewed Apr 24, 2020

View reviewed changes

src/ansi.rs Outdated Show resolved Hide resolved

sharkdp reviewed Apr 24, 2020

View reviewed changes

src/distinct.rs Outdated

result.closest_pair.0

} else {

if self.rng.gen() {

#[allow(clippy::collapsible_if)]

Copy link

Owner

sharkdp Apr 24, 2020

Choose a reason for hiding this comment

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

similar here

sharkdp reviewed Apr 24, 2020

View reviewed changes

src/parser.rs Show resolved Hide resolved

sharkdp reviewed Apr 24, 2020

View reviewed changes

rivy force-pushed the add.gha-cicd branch from 0caebc6 to b89bee8 Compare April 29, 2020 01:15

rivy added 6 commits May 1, 2020 12:08

Maint/CI ~ add GitHub-Actions CI (aka GHA)

d73fdcb

refactor/polish ~ fix cargo clippy complaint (unreadable_literal)

e2f963b

refactor/polish ~ fix cargo clippy complaint (or_fun_call)

6f469df

refactor/polish ~ fix cargo clippy complaint (fatal/approx_constant)

5065e61

refactor/polish ~ fix cargo clippy complaint (redundant-static-life…

e026137

…times)

refactor/polish ~ fix cargo clippy complaint (writeln_empty_string)

ca53c6e

rivy added 3 commits May 1, 2020 12:08

refactor/polish ~ fix cargo clippy complaint (useless_format)

c620ca6

refactor/polish ~ fix cargo clippy complaint (collapsible_if)

eebb4e3

refactor/polish ~ fix cargo clippy complaint (unused import)

41866e0

rivy force-pushed the add.gha-cicd branch from b89bee8 to 446f83f Compare May 2, 2020 19:27

rivy added 13 commits May 2, 2020 14:38

refactor/polish ~ fix cargo clippy complaint (option_as_ref_deref);…

816fc21

… * MinSRV to '1.40.0' - `.as_deref()` was stabilized in rust v1.40.0; * requires MinSRV => '1.40.0' - ref: <https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1400-2019-12-19>

refactor/polish ~ *allow* cargo clippy complaint (many_single_char_…

8663a88

…names)

refactor/polish ~ *allow* cargo clippy complaint (identity_op)

baa3a71

refactor/polish ~ *allow* cargo clippy complaint (float_cmp)

3c14b2b

Maint/CI ~ add debian packaging to GHA:CICD

cc41cc9

Maint/CI ~ add additional CICD platform targets

5853787

Maint/CI ~ add CICD code coverage (grcov to CodeCov.io)

c9c583e

Maint/CI ~ cleanup/polish GHA:CI (remove comments & unused vars)

b89bd72

Maint/CI ~ cleanup/polish GHA:CI (remove unused 'features' support)

54431ef

Maint/CI ~ cleanup/polish GHA:CI (remove unneeded cargo fmt gating …

ca1f14e

…for windows)

Maint/CICD ~ remove GHA:'Style' job

1ebecc9

Maint/CICD ~ update i686-pc-windows-gnu build target to 'stable' (req…

2b9763c

…uired stable == rust v1.43.0+)

rivy force-pushed the add.gha-cicd branch from 446f83f to 231106d Compare May 2, 2020 19:38

rivy requested a review from sharkdp May 2, 2020 19:51

sharkdp approved these changes May 24, 2020

View reviewed changes

sharkdp merged commit a45a7f5 into sharkdp:master May 24, 2020

sharkdp mentioned this pull request May 25, 2020

Github actions integration sharkdp/bat#978

Closed

rivy deleted the add.gha-cicd branch May 25, 2020 22:14

sharkdp mentioned this pull request Jun 25, 2020

Shell completions are no longer bundled #129

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GHA CICD #120

Add GHA CICD #120

rivy commented Mar 10, 2020 •

edited

Loading

rivy commented Mar 10, 2020

rivy commented Mar 10, 2020

sharkdp left a comment

rivy commented Mar 17, 2020

sharkdp commented Mar 17, 2020

rivy commented Mar 18, 2020 •

edited

Loading

rivy commented Mar 18, 2020

rivy commented Mar 18, 2020

sharkdp commented Mar 20, 2020

rivy commented Mar 23, 2020 •

edited

Loading

sharkdp commented Apr 24, 2020 •

edited

Loading

sharkdp Apr 24, 2020

sharkdp Apr 24, 2020 •

edited

Loading

rivy Apr 29, 2020

sharkdp Apr 24, 2020 •

edited

Loading

rivy Apr 29, 2020

rivy commented Apr 28, 2020

sharkdp commented Apr 28, 2020

rivy commented Apr 29, 2020 •

edited

Loading

sharkdp commented Apr 29, 2020

rivy commented May 1, 2020

rivy commented May 2, 2020 •

edited

Loading

sharkdp left a comment •

edited

Loading

sharkdp commented May 24, 2020

rivy commented May 24, 2020 •

edited

Loading

sharkdp commented May 24, 2020

rivy commented May 24, 2020

sharkdp commented May 25, 2020

Add GHA CICD #120

Add GHA CICD #120

Conversation

rivy commented Mar 10, 2020 • edited Loading

rivy commented Mar 10, 2020

rivy commented Mar 10, 2020

sharkdp left a comment

Choose a reason for hiding this comment

rivy commented Mar 17, 2020

sharkdp commented Mar 17, 2020

rivy commented Mar 18, 2020 • edited Loading

rivy commented Mar 18, 2020

rivy commented Mar 18, 2020

sharkdp commented Mar 20, 2020

rivy commented Mar 23, 2020 • edited Loading

sharkdp commented Apr 24, 2020 • edited Loading

sharkdp Apr 24, 2020

Choose a reason for hiding this comment

sharkdp Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

rivy Apr 29, 2020

Choose a reason for hiding this comment

sharkdp Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

rivy Apr 29, 2020

Choose a reason for hiding this comment

rivy commented Apr 28, 2020

sharkdp commented Apr 28, 2020

rivy commented Apr 29, 2020 • edited Loading

sharkdp commented Apr 29, 2020

rivy commented May 1, 2020

rivy commented May 2, 2020 • edited Loading

sharkdp left a comment • edited Loading

Choose a reason for hiding this comment

sharkdp commented May 24, 2020

rivy commented May 24, 2020 • edited Loading

sharkdp commented May 24, 2020

rivy commented May 24, 2020

sharkdp commented May 25, 2020

rivy commented Mar 10, 2020 •

edited

Loading

rivy commented Mar 18, 2020 •

edited

Loading

rivy commented Mar 23, 2020 •

edited

Loading

sharkdp commented Apr 24, 2020 •

edited

Loading

sharkdp Apr 24, 2020 •

edited

Loading

sharkdp Apr 24, 2020 •

edited

Loading

rivy commented Apr 29, 2020 •

edited

Loading

rivy commented May 2, 2020 •

edited

Loading

sharkdp left a comment •

edited

Loading

rivy commented May 24, 2020 •

edited

Loading