Page MenuHomePhabricator

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

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



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



Result before patch:

./ 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

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

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