Page MenuHomePhabricator

[build-configurations] Resurface the more informative error message when ABC_BUILD_NAME is not set
ClosedPublic

Authored by jasonbcox on Oct 4 2019, 21:00.

Details

Summary

D3992 ended up masking this error message. Since the custom error message is more informative, I decided to move set -u instead of removing the message entirely. Not having set -u set after simple error messages like this should have negligible effect.

Test Plan

Run:

./build-configurations.sh

Result before patch:

./build-configurations.sh: line 7: ABC_BUILD_NAME: unbound variable

Result after patch:

+ '[' -z '' ']'
+ echo 'Error: Environment variable ABC_BUILD_NAME must be set'
Error: Environment variable ABC_BUILD_NAME must be set
+ exit 1

Diff Detail

Repository
rABC Bitcoin ABC
Branch
better-error-msg
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7691
Build 13421: Bitcoin ABC Buildbot (legacy)
Build 13420: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Oct 4 2019, 21:00
deadalnix requested changes to this revision.Oct 6 2019, 17:54

This whole approach do not seem to make a lot of sense. The build build_names are not composable in any way, and as it is mandatory, it seems like it should be a parameters rather than an environment variable.

In any case, you can avoid the -u dance by conditionally setting the variable to an empty string and testing against this.

This revision now requires changes to proceed.Oct 6 2019, 17:54
jasonbcox updated this revision to Diff 13380.Oct 7 2019, 17:10

Fix according to feedback

Good point on the composability aspect. I started to notice issues arising from the current design while working on D4213. It's more clear to me now that a composable design would make it more robust in addition to providing the caller with more customization of the build.

deadalnix accepted this revision.Oct 7 2019, 17:31
This revision is now accepted and ready to land.Oct 7 2019, 17:31