Paths

Table of Contentst

Added release.sh script for generating a release commit
AbandonedPublic
Actions

Authored by jasonbcox on May 30 2019, 00:16.

Details

Reviewers

deadalnix
Fabien

Group Reviewers

Restricted Project

Summary

This diff lays the groundwork for release automation (T646) by adding a new script release.sh.

The current iteration of release.sh doesn't do much on its own. For now, it generates
a release commit containing only updates to the manpages and tags that commit. In the future,
it will do many of the automatible steps in doc/release-process.md (see TODOs in the script).

While release.sh is intended to be run by a release bot (currently WIP), it can be run manually as well.

Test Plan

# Show help
./contrib/release/release.sh -h
./contrib/release/release.sh --help

# Fail on unrecognized option
./contrib/release/release.sh -a
./contrib/release/release.sh -a somevalue

# Generates a Phabricator revision, like this: https://reviews.bitcoinabc.org/D3155
./contrib/release/release.sh

# All tests below this line should generate a Phabricator diff, but not a revision

# From the top-level:
./contrib/release/release.sh -d

# With a commit prefix:
./contrib/release/release.sh -d -p "[Release Bot] "

# Extra dry run while working on another branch:
git ch -b mybranch
./contrib/release/release.sh -e

# From another directory (not top-level):
cd src
../contrib/release/release.sh -e -b ../build

Diff Detail

Repository

rABC Bitcoin ABC

Branch

releasebot

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6068
Build 10185: Bitcoin ABC Buildbot (legacy)
Build 10184: arc lint + arc unit

Event Timeline

jasonbcox created this revision.May 30 2019, 00:16

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 30 2019, 00:16

jasonbcox edited the test plan for this revision. (Show Details)May 30 2019, 00:16

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fixed a bug with unrecognized options

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

jasonbcox edited the test plan for this revision. (Show Details)May 30 2019, 00:28

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6059: Diff 8998.May 30 2019, 00:31

Harbormaster completed remote builds in B6062: Diff 9001.May 30 2019, 00:41

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

jasonbcox planned changes to this revision.May 30 2019, 01:01

jasonbcox edited the test plan for this revision. (Show Details)May 30 2019, 01:14

Fixed a bug with revision messages not being complete while running the script non-dry-run

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6068: Diff 9007.May 30 2019, 01:32

Fabien requested changes to this revision.May 30 2019, 06:35

Fabien added inline comments.

contrib/release/release.sh
11	If you move the `help_message()` below the variables you can use `$BUILD_DIR` directly in the help message. Having an internal variable name in the help is not very useful.
20	If this is to be run by a bot, you may default to something less common to make it easier to run the script manually without impacting user's tree.
59	You might want to use `getopt` here, that would make it work with the `--key=value` syntax as well and make maintenance easier.
76	You'd better create the build dir and run `autogen.sh` and `configure` in the script. I don't know exactly what is the environment for this bot, but that would make sure that your build is under control.
79	Nit: use `[[:digit:]]+` instead of `[[:digit:]]*`
94	Nit: it is a good practice to surround the variable with {} when there is no space after the variable name, here `${PREFIX}` and `${VERSION}`.

This revision now requires changes to proceed.May 30 2019, 06:35

jasonbcox added inline comments.May 30 2019, 16:55

contrib/release/release.sh
59	My understanding is you can't do long and short names with getopt. This seems to work unless there is a technical reason to not?

Fabien added inline comments.May 30 2019, 17:46

contrib/release/release.sh
59	`getopts` can't, but `getopt` can: https://dustymabe.com/2013/05/17/easy-getopt-for-a-bash-script/ Sure your solution works, this is only minor usage improvement.

Rebase + address feedback

contrib/release/release.sh
11	I was actually intending to have it read as `$TOPLEVEL/build` to the user. But seeing as this confused rather than helped you, I guess that help failed. :) Do you think it's odd to have the help message change based on your working directory? I don't think I've seen this in help messages myself.
20	Good idea.
59	getopt also isn't portable: https://stackoverflow.com/a/12026302 Although this script's primary use case is a bot running on Debian Stretch, I could imagine it being used on OSX manually. This seemed to be the best approach. Let me know if I'm missing something or if the link is old information.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6099: Diff 9047.May 31 2019, 15:57

This seems to be an extremely convoluted way to launch one command that you could launch directly instead of using this. This does nothing to help the slow part of the process, where you need to submit a patch, get it reviewed land it. There are no guarantee that this will generate the proper man pages, because the manpage generated will be dependent on the system on which the software is built.

If the goal is to have this run by a bot, a stated in diff description, then this makes even less sense. Is the bot supposed to ask for review ? And what does it do next ? Some human has to take over ? If so this is not really helping.

I don't think this approach makes a lot of sense. I'd suggest to think through what the expected workflow will look like at the end and then start executing toward this. Right now, it doesn't looks like there is a clear direction this is going toward.

contrib/release/release.sh
107 ↗	(On Diff #9047)	Then it's not really a dry run, is it?

This revision now requires changes to proceed.May 31 2019, 17:24

In D3152#75120, @deadalnix wrote:

This seems to be an extremely convoluted way to launch one command that you could launch directly instead of using this. This does nothing to help the slow part of the process, where you need to submit a patch, get it reviewed land it. There are no guarantee that this will generate the proper man pages, because the manpage generated will be dependent on the system on which the software is built.

If the goal is to have this run by a bot, a stated in diff description, then this makes even less sense. Is the bot supposed to ask for review ? And what does it do next ? Some human has to take over ? If so this is not really helping.

I don't think this approach makes a lot of sense. I'd suggest to think through what the expected workflow will look like at the end and then start executing toward this. Right now, it doesn't looks like there is a clear direction this is going toward.

The idea is to configure the machine running the bot in such a way that generates the manpages we want. It's clearly better than what we have today which is different people generating manpages in different environments.

And yes, the bot will be opening a diff for review and automatically land it if it's green. If it's not green, someone can commandeer the diff and work from there. Once any bugs in the process have been worked out, I expect commandeering the bot's diffs to be a rare occurrence.

The direction this diff is headed is roughly stated here: T646

contrib/release/release.sh
107 ↗	(On Diff #9047)	It is. It creates a diff not attached to a revision. This is useful for debugging and seeing what it will look like on Phabricator without pinging people for review.