Skip to content

Conversation

mdqst
Copy link

@mdqst mdqst commented May 22, 2025

noticed a bug in the must_get_env function where the error message was showing the value reference (${!VAR_VALUE}) instead of the actual variable name (${VAR_NAME}).

fixed it so that the error now correctly displays the name of the missing environment variable.

@mdqst mdqst requested a review from jakubgs as a code owner May 22, 2025 10:38
@status-github-bot-v2 status-github-bot-v2 bot moved this to CONTRIBUTOR in Pipeline for QA May 22, 2025
Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see any issue with that function as it is right now in develop:

~/work/status-mobile develop 51s
 > bash

[jakubgs@lilim:~/work/status-mobile]$ eval "$(sed -n '/function must_get_env/,+8p' scripts/build-android.sh)"

[jakubgs@lilim:~/work/status-mobile]$ must_get_env TEST
No required env variable: TEST
exit

@mdqst
Copy link
Author

mdqst commented May 22, 2025

jakubgs hey, thanks for checking!

the problem is that in the error message you use ${!VAR_VALUE}, which expands to the variable’s value, not its name. so when the var is missing, it prints an empty string instead of the variable name.

If you run:

unset TEST_VAR
must_get_env TEST_VAR

the error shows

No required env variable:

without the name.

Changing it to just $1 will print the missing variable name correctly:

echo -e "${RED}No required env variable:${RST} ${BLD}$1${RST}" 1>&2

that way it shows:

No required env variable: TEST_VAR

@mdqst mdqst requested a review from jakubgs May 22, 2025 11:10
@@ -3,16 +3,17 @@
# Needed to fail on must_get_env()
set -e

GIT_ROOT=$(cd "${BASH_SOURCE%/*}" && git rev-parse --show-toplevel)
GIT_ROOT=$(cd "$(dirname "${BASH_SOURCE[0]}")" && git rev-parse --show-toplevel)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is that better?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is that better?

is better in terms of clarity, correctness, and consistency, especially in more complex scripts or when the script could be sourced.

if [[ -n "${VAR_VALUE}" ]]; then
echo "${VAR_VALUE}"
return
fi
echo -e "${RED}No required env variable:${RST} ${BLD}${!VAR_VALUE}${RST}" 1>&2
echo -e "${RED}No required env variable:${RST} ${BLD}${VAR_NAME}${RST}" 1>&2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ! trick works because VAR_VALUE is not a variable but rather a reference to a var created via declare -n:

      -n	make NAME a reference to the variable named by its value

Which means that VAR_VALUE is a reference to $1. We can test this:

 > TEST=something
 > VAR_NAME=TEST
 > declare -n VAR_REF=$VAR_NAME
 > echo $VAR_REF
something
 > echo ${!VAR_REF}
TEST

Your version is clearer, but the result is the same. What I would like to understand is why for you it doesn't expand into the name of variable from the nameref.

@mdqst mdqst requested a review from jakubgs May 23, 2025 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: CONTRIBUTOR
Development

Successfully merging this pull request may close these issues.

2 participants