Page MenuHomePhabricator

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

Authored by jasonbcox on Fri, Oct 4, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Fri, Oct 4, 21:00
deadalnix requested changes to this revision.Sun, Oct 6, 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.Sun, Oct 6, 17:54
jasonbcox updated this revision to Diff 13380.Mon, Oct 7, 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.Mon, Oct 7, 17:31
This revision is now accepted and ready to land.Mon, Oct 7, 17:31