Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGING9241317208b7: Add a script for building and pushing automated commits
rABC9241317208b7: Add a script for building and pushing automated commits

Summary

This script provides a similar function to contrib/teamcity/build-configurations.sh and allows us to define various
pipelines that a bot can and is expected to run, given some trigger (such as a schedule). This initial version of the script
supports update-chainparams only for now, but others will soon be added.

To prep for this script's deployment, I've done the following:

Setup a machine to run bitcoind instances on mainnet and testnet where this script can be run.
Created a bot user 'abc-bot' on Phabricator.
Added Herald rules to allow abc-bot to push changes upstream without code review.

The script will be scheduled in Teamcity to run update-chainparams once per week.

Test Plan

git ch -b testbranch  # we assume we're not writing changes directly on master
./automated-commits.sh  # errors as expected

bitcoind &
PID1=$!
bitcoind --testnet &
PID2=$!
DIFF_NAME=update-chainparams ./automated-commits.sh  # succeeds, but as a dry-run

sudo kill $PID2
git ch testbranch
DIFF_NAME=update-chainparams ./automated-commits.sh  # fails, leaving uncommitted changes

git stash
git ch testbranch
git stash pop         # test with dirty source tree
bitcoind --testnet &
PID2=$!
DIFF_NAME=update-chainparams ./automated-commits.sh  # succeeds, but as a dry-run
sudo kill $PID1 $PID2

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

jasonbcox created this revision.Jan 22 2020, 23:55

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 22 2020, 23:55

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9111: Diff 15748.Jan 23 2020, 00:00

Does this make diffs automatically?

In D5045#122570, @deadalnix wrote:

Does this make diffs automatically?

Not phab diffs, but if you consider a git commit being a diff, then yes.

automated diff -> automated commit since 'diff' is ambiguous

jasonbcox retitled this revision from Add a script for building and pushing automated diffs to Add a script for building and pushing automated commits.Jan 30 2020, 23:16

jasonbcox edited the test plan for this revision. (Show Details)

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9213: Diff 15916.Jan 31 2020, 00:23

Mostly nit picking

contrib/source-control-tools/automated-commits.sh
12 ↗	(On Diff #15916)	Nit: consistency between `$DIFF_NAME` and `${DIFF_NAME}`
13 ↗	(On Diff #15916)	`DIFF_NAME` is a not a meaningful name, since it's not a diff (in the sense of phabricator) and you're not setting a name.
29 ↗	(On Diff #15916)	Dito
58 ↗	(On Diff #15916)	IDK how complicated it could be, but it would be nice to run the linters before committing to prevent the bot from adding some formatting errors that would need to be fixed by users.

This revision now requires changes to proceed.Feb 4 2020, 18:27

DIFF_NAME -> COMMIT_TYPE

IDK how complicated it could be, but it would be nice to run the linters before committing to prevent the bot from adding some formatting errors that would need to be fixed by users.

As we discussed offline, I think running the linter automatically hides underlying issues with our codegen. So instead, we can run
the linter without applying the patches and check for any issues. If there are, this build should fail, indicating that the
codegen needs fixing. The now updated diff does just that.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Fix parts of the new error message that didn't make sense.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9270: Diff 16020.Feb 4 2020, 23:39

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9271: Diff 16021.Feb 4 2020, 23:57

This doesn't seems to me like this is building capital toward more automation, but this is something.

Fabien requested changes to this revision.Feb 5 2020, 15:44

Fabien added inline comments.

contrib/source-control-tools/automated-commits.sh
62 ↗	(On Diff #16021)	This is brittle. If the linter doesn't display "OKAY" then you can expect the return code to be non zero.
63 ↗	(On Diff #16021)	Note that this will fail if there is an advice message. This is currently possible if you remove a boost include from the codebase for example, but is unlikely to happen in an automated commit.
64 ↗	(On Diff #16021)	Print the linter output in this case, that would make it easier to debug should a false positive happen.

This revision now requires changes to proceed.Feb 5 2020, 15:44

jasonbcox added inline comments.Feb 5 2020, 23:54

contrib/source-control-tools/automated-commits.sh
62 ↗	(On Diff #16021)	Surprisingly not true. The linter can give advice/warnings and still returns 0, which phabricator screams at during review, so I think the end result is the same as if there were linter errors. Checking for OKAY and also checking that OKAY is the only line seems to be the most robust way to ensure the autogen code is completing lint free.
63 ↗	(On Diff #16021)	See above comment.
64 ↗	(On Diff #16021)	It should be printed by the `set -x` at the top of the script when `LINT_OUTPUT` is set. Would you still prefer the extra redundancy?

Fabien added inline comments.Feb 6 2020, 07:25

contrib/source-control-tools/automated-commits.sh
62 ↗	(On Diff #16021)	After a quick test: Errors return code 2; warnings return code 1; autofixes and advices (and disabled of course) return code 0. Errors and warnings both prevent the `OKAY` line to be displayed. So in the end you can have the exact same check by looking the return code rather than grepping for OKAY, with some benefit: It is robust against arcanist output changes, It won't fail if there is the word OKAY in the linted code (mitigated by the line count).
63 ↗	(On Diff #16021)	Actually you can also prevent this failure by passing the `--severity=autofix` option to prevent the linter from outputting advices.
64 ↗	(On Diff #16021)	It's fine i just forgot +x does variable expansion.

Fabien added inline comments.Feb 8 2020, 17:04

contrib/source-control-tools/automated-commits.sh
63 ↗	(On Diff #16021)	After looking at D5227, it looks reasonable to avoid prevent linter advices, as the phabricator message (`Linter found an issue`) is confusing.