Run checks against pull requests

[lukechilds]

Now checks will run against every feature branch and PR push instead of just pushes to master.

Resolves #21

[lukechilds]

Checks are really slow due to the QEMU builds. Kind of annoying because it'll just eat up all the jobs in the allowed concurrent job limit if we get a lot of pushes.

We can "skip" the QEMU jobs with

- name: Skip non native builds on non master pushes
  if: github.ref != 'master' && matrix.arch != 'amd64'
  run: exit 1

It resolves the issue of eating up all the concurrent jobs but technically that's a job fail so it pollutes check output with loads of "failed" jobs.

Massively reduces the usefulness of the checks though because the overall status will show as failed. We have to manually check all the amd64 builds passed to be sure nothing actually failed.

[lukechilds]

After some digging it seems like it's currently just not possible to conditionally skip a matrix job with GitHub Actions.

https://github.community/t5/GitHub-Actions/How-to-conditionally-include-exclude-items-in-matrix-eg-based-on/td-p/37871

It's a shame because you can do that on most other CI systems.

Looks like there was a "neutral" exit code in GitHub Action beta which would've allowed us to skip jobs. We could exit 78 which would kill the job but it would be treated as a neutral kill and not count as a fail.

https://github.community/t5/GitHub-Actions/GitHub-Actions-quot-neutral-quot-exit-code-is-incorrectly/td-p/29051

However it was removed because some programs could actually return that exit code in legitimate errors: https://twitter.com/ethomson/status/1163899559279497217

That tweet implies GitHub Actions still plans to support a neutral exit code but I can't find any documentation on it so doesn't look possible for now.

[lukechilds]

TL;DR: Options are:

• Run entire build system on all pushes, QEMU builds will eat up jobs
• Run only native builds, QEMU builds will show as failed on non-master pushes
• Clone test.yml and remove QEMU builds from matrix and run on all branches, means maintaining two basically identical Actions

😕

@meeDamian let me know how you want me to proceed.

[meeDamian]

That will cause the job to always be red on PRs, quite not ideal I'd say.

[meeDamian]

Clone test.yml and remove QEMU builds from matrix and run on all branches, means maintaining two basically identical Actions

I think that's by far the best solution.

[meeDamian]

Also maybe we can go with sth like:

on:
  pull_request:
    - '*'

And all arch-specific stuff can be removed too, making the workflow quite small, and nice.

[meeDamian]

Let's keep test as test, and add another one called pull-request?

[lukechilds]

So you don't wanna run tests on feature branches?

PRs only?

[lukechilds]

And all arch-specific stuff can be removed too, making the workflow quite small, and nice.

I intentionally left the two files as similar as possible so the git diffs should be almost identical.

e.g I've left the matrix.arch in even though it only has one entry.

My reasoning was that this makes it much easier to maintain them both and keep them in sync, we can just diff them and see that there are only the minimal trigger/matrix changes we want.

Happy to clean up the PR version if you want but this might make maintaining them in parallel going forward more difficult.

[meeDamian]

Without the arch stuff, the file becomes really, simple:

name: Build bitcoind on pull request

on: pull_request

jobs:
  build:
    name: Build Bitcoind
    runs-on: ubuntu-18.04
    env:
      DOCKER_BUILDKIT: 1

    strategy:
      fail-fast: false
      matrix:
        subver:
          - '0.16'
          - '0.17'
          - '0.18'
          - '0.19'

    steps:
      - uses: actions/checkout@v1.0.0

      - name: Build Bitcoind
        run: docker build  -t bitcoind  ${{matrix.subver}}/

      - name: Print Bitcoind version
        run: |
          docker run --rm  --entrypoint=uname  bitcoind  -a
          docker run --rm bitcoind --version

And because of that, simple-to-read might be better here than simple-to-compare?

[meeDamian]

So you don't wanna run tests on feature branches?

PRs only?

We don't have any, will be thinking about it once we do :P

And even if we do, if there's a PR open for it to master it'll build. So good 'nuff I reckon.

[meeDamian]

After you change workflow description, I'm happy with it :).

[lukechilds]

Fixed before you even commented 😛

Let's just wait for the builds to complete just to make sure I didn't bork anything.

[meeDamian]

Now I wonder why qemu-perf never runs 🤔

[lukechilds]

Does it not run on cron:

docker-bitcoind/.github/workflows/qemu-perf.yml

Line 5 in 574252c

- cron: '2 1 * * *'

https://github.com/lncm/docker-bitcoind/actions?query=workflow%3A%22Perf-check+qemu+versions%22

[meeDamian]

#27 ^^

[lukechilds]

So all these builds were triggered manually?

https://github.com/lncm/docker-bitcoind/actions?query=workflow%3A%22Perf-check+qemu+versions%22

[meeDamian]

Lol, no - I'm just a bonehead 😂.