Page MenuHomePhabricator

Added release.sh script for generating a release commit
AbandonedPublic

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

Fixed a bug with unrecognized options

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

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

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.

jasonbcox marked 4 inline comments as done.

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.

deadalnix requested changes to this revision.May 31 2019, 17:24

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

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.

contrib/release/release.sh
107 ↗(On Diff #9047)

I'm not saying that it is not useful, I'm saying that it is not a dry run.