Check each commit at least builds in CI #696

TheBlueMatt · 2020-09-15T18:40:15Z

No description provided.

julianknutsen · 2020-09-15T18:56:46Z

.github/workflows/build.yml

+          git rebase upstream/main
+      - name: For each commit, run cargo check (including in fuzz)
+        run: |
+          for COMMITHASH in `git log --format=format:%H upstream/main...HEAD`; do


Can this be a shell script that devs can run locally, too? Makes it easier to test as well.

A bit more complicated to scan the remotes, but, done!

codecov · 2020-09-15T19:23:12Z

Codecov Report

Merging #696 into main will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #696      +/-   ##
==========================================
- Coverage   91.92%   91.87%   -0.05%     
==========================================
  Files          35       35              
  Lines       20082    20082              
==========================================
- Hits        18461    18451      -10     
- Misses       1621     1631      +10

Impacted Files	Coverage Δ
lightning/src/ln/functional_tests.rs	`96.96% <0.00%> (-0.18%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 343aacc...4d9532a. Read the comment docs.

julianknutsen

Added my feedback about how to make this a bit cleaner from a dev point of view. I have a bash alias that sort of does these things so I had some preexisting thoughts on what this might look like longer term.

julianknutsen · 2020-09-15T19:55:14Z

ci/check-each-commit.sh

@@ -0,0 +1,24 @@
+#!/bin/sh


Here are the use cases as I see them:

As a developer, I want to know that every patch in my stack builds and passes "all?" tests against some remote before I push to my branch so I don't waste time later and have to backtrack when CI finally catches up to tell me I made a mistake.

As a CI tool, I want to verify every commit against main and throw an error if a single commit doesn't build so future rebase and git bisect can work cleanly.

From a developer point of view, I would love to use this tool against my own upstream branches for dependant PRs and in cases where my branch might not be in the most hygienic state, but I still get a lot of value in making sure everything builds with the current state of the world.

I also don't really want rebase done for me since I would like to control when those actions happen in the event I am in a non-clean state.

What about an API like this that allows the users to control the remotes that are used? Makes the auto-complete go away, too.

CI:
Make sure all commits pass against main. I'm not sure the CI context w.r.t. remotes available, but this is the idea.
check-each-commit.sh main

Dev:
As a dev I want to see if my current work in progress is clean against origin
check-each-commit.sh origin/main

As a dev I want to see if my current work in progress is clean against a dependant branch
check-each-commit.sh origin/pre-existing-dependant-branch

As a dev I want to see if my current work in progress is clean against upstream/main and ready for PR.
git rebase upstream/main - manual
check-each-commit.sh upstream/main

Hmm, I suppose we could just always do a git log from a given point, but the script as-is doesn't rebase for you, it just makes sure you're built directly on top of upstream/main.

Errr, ok, it wasn't supposed to.

julianknutsen · 2020-09-15T19:57:59Z

ci/check-each-commit.sh

+git rebase $UPSTREAM/main
+for COMMITHASH in `git log --format=format:%H $UPSTREAM/main...HEAD`; do
+	git checkout $COMMITHASH
+	cargo check


This doesn't play nicely with rustup default as a dev. I think using the MSRV directly +1.30.0 will be the most useful in catching syntax features that make new code work, but old code fails.

But fuzz only works on +stable? So maybe just using that directly will catch "most" things.

Not all cargos have +X support, only rustup, so we can't really use it without adding a ton of complexity. Nothing really wrong with using a given version, either, though.

julianknutsen · 2020-09-15T19:58:41Z

ci/check-each-commit.sh

+git rebase $UPSTREAM/main
+for COMMITHASH in `git log --format=format:%H $UPSTREAM/main...HEAD`; do
+	git checkout $COMMITHASH
+	cargo check


Calling these manually, for now, seems OK. When more work is done to make the build/test workflow cleaner with some wrapper scripts this can just be converted.

julianknutsen · 2020-09-15T20:07:42Z

ci/check-each-commit.sh

+for COMMITHASH in `git log --format=format:%H $UPSTREAM/main...HEAD`; do
+	git checkout $COMMITHASH
+	cargo check
+	cd fuzz && cargo check --features=stdin_fuzz && cd ..


Making fuzz opt-in with a flag might be a good idea to make this easy on developers. This gets into longer-than-a-coffee-break territory with just a few patches in a stack.

Hmm, it really shouldnt - cargo check does some caching, so while the first commit might take a minute, the later ones should all be pretty much instant. More importantly, breaking fuzz compile is one of the most common issues, so it'd be kinda bad to default it off.

Ah yes. I read this as running the fuzz tests. I completely agree that these should be built.

julianknutsen

I did some manual testing of this to see how it goes in my workflow.

After successful completion, this leaves you in a detached head state. Anything that can be done about that?

ci/check-each-commit.sh

TheBlueMatt · 2020-09-15T22:03:12Z

After successful completion, this leaves you in a detached head state. Anything that can be done about that?

Heh, gotta bring back rebase :)

TheBlueMatt · 2020-09-15T22:04:11Z

ci/check-each-commit.sh

+for COMMITHASH in `git log --reverse --format=format:%H $1...HEAD`; do
+	git checkout $COMMITHASH
+	cargo check
+	cd fuzz && cargo check --features=stdin_fuzz && cd ..


Heh, well that got rewritten anyway....test the current version?

julianknutsen

Two minor things and this is good to go. I tested it locally on my branches against upstream as well as dependent branches against origin and it does what it says.

ci/check-compiles.sh

julianknutsen · 2020-09-15T22:25:42Z

ci/check-compiles.sh

@@ -0,0 +1,5 @@
+#!/bin/sh
+set -e
+set -x


The current progress is hidden as the rebase progresses. I tested a few things locally and I think something like git log -1 --oneline at the top here is enough to let me know where we are in the stack.

I also really like the side-effect of being able to fix my issue in the middle of a rebase instead of having to create a temp branch, fix, rebase.

Hmm, if it fails it should leave it where it was when it failed, no?

Yes, when it fails it leaves it in the rebase where I can make changes which is great.

When it doesn't fail it is still nice to see the progress which explains the --oneline

julianknutsen

ACK 4d9532a

All my local testing went well and it passed in CI.

TheBlueMatt force-pushed the 2020-09-ci-check-commits branch 5 times, most recently from 7870e1c to bd44fdb Compare September 15, 2020 18:53

julianknutsen reviewed Sep 15, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-09-ci-check-commits branch 2 times, most recently from 559b10a to 8434d06 Compare September 15, 2020 19:13

TheBlueMatt force-pushed the 2020-09-ci-check-commits branch 2 times, most recently from 392c12c to 8ad9d42 Compare September 15, 2020 19:31

julianknutsen suggested changes Sep 15, 2020

View reviewed changes

julianknutsen reviewed Sep 15, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-09-ci-check-commits branch 2 times, most recently from df813fe to 5caf6eb Compare September 15, 2020 20:10

julianknutsen reviewed Sep 15, 2020

View reviewed changes

ci/check-each-commit.sh Outdated Show resolved Hide resolved

TheBlueMatt force-pushed the 2020-09-ci-check-commits branch 2 times, most recently from 74373a0 to 131b883 Compare September 15, 2020 22:03

julianknutsen reviewed Sep 15, 2020

View reviewed changes

julianknutsen suggested changes Sep 15, 2020

View reviewed changes

Check each commit at least builds in CI

4d9532a

TheBlueMatt force-pushed the 2020-09-ci-check-commits branch from 131b883 to 4d9532a Compare September 15, 2020 23:24

julianknutsen approved these changes Sep 15, 2020

View reviewed changes

TheBlueMatt merged commit 26ee7e6 into lightningdevkit:main Sep 16, 2020

Check each commit at least builds in CI #696

Check each commit at least builds in CI #696

Uh oh!

Conversation

TheBlueMatt commented Sep 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julianknutsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianknutsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheBlueMatt commented Sep 15, 2020

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianknutsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianknutsen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Sep 15, 2020 •

edited

Loading