Genesis sub-command support added #8
Conversation
Added support for genesis subcommand that exists in 0.47+
There was a problem hiding this comment.
I made some comments but I didn't say this yet: this is one of the most mature submissions from a third party in a while. Thank you for doing your due diligence and writing the new code in the same style as the rest of the code is.
I have two requests for modifications, detailed in the comments:
- If possible, let's not use
awkandcut. - I'm ok with using
sort -V, but please make it an explicit requirement inenforce_requirementsso the user gets informed if the app is missing.
Very nice PR, kudos!
bin/lib-gm
Outdated
| # Returns empty string if long is not supported | ||
| get_sdk_version() { | ||
| GAIAD_BINARY="$(get_gaiad_binary "$1")" | ||
| RESULT="$("$GAIAD_BINARY" version --long | grep "cosmos_sdk_version" | awk '{print $2}' | cut -c 2-)" |
There was a problem hiding this comment.
I'm very particular about introducing new dependencies and awk was not on the list yet. (Neither was cut, as it turns out.) Let me think about this for a second, and see if we could find a way to not introduce new dependencies.
Also, a different note: gaiad and possibly other binaries too print the version to stderr instead of stdout so grep will not catch it.
How does this sound?
| RESULT="$("$GAIAD_BINARY" version --long | grep "cosmos_sdk_version" | awk '{print $2}' | cut -c 2-)" | |
| INTERIM="$("$GAIAD_BINARY" version --long 2>&1 | grep "cosmos_sdk_version")" | |
| RESULT="${INTERIM##cosmos_sdk_version: v}" |
One of these days we'll have to rewrite this app in some decent language instead of POSIX shell... (I actually started that, I just couldn't deploy a working version yet.) Dependencies kill us...
There was a problem hiding this comment.
I've used sed instead which was already checked in enforce_requirements:
RESULT="$("$GAIAD_BINARY" version --long 2>&1 | sed -n '/cosmos_sdk_version/ s/cosmos_sdk_version: v\(.*\)$/\1/p')"
bin/lib-gm
Outdated
| GAIAD_BINARY="$(get_gaiad_binary "$1")" | ||
| RESULT="$($GAIAD_BINARY | grep -q 'gentx' || echo "genesis")" | ||
| else | ||
| HIGHER_VERSION=$(echo "${GENESIS_BREAKING_VERSION}\n${SDK_VERSION}" | sort -Vr | head -1) |
There was a problem hiding this comment.
Very smart way of comparing version numbers and also the only way I found that's halfway decent. The -V parameter is only available in newer versions of sort so I try not to rely on it.
Also, this is yet another new dependency that we have to make sure is accessible.
So, here's my suggestion:
Option A:
Add a quick test in enforce_requirements to make sure sort -V is available. Something like:
test "$(echo "a\nb" | sort -Vr | grep b)" = "b" || exit_with_error "missing sort -V"
Then we can rely on it safely. (The main issue with not testing it is that if something is missing and we didn't tell the user about it, the script might fail without error messages. So we test and inform.)
Option B:
Maybe we should get away from using --long in general, since not all binaries have it. The fallback method is fairly safe, even if it's a bit crude.
I understand that a lot of work went into checking the SDK version and I actually struggle with this a lot in other build processes too.
I don't mind either way, we just have to make sure it's straightforward for the end user.
There was a problem hiding this comment.
I've went with Option A, and added the following test:
if [ -z "$(which sort || echo)" ]; then
exit_with_error "missing sort, please install it"
fi
if ! test "$(echo "a\nb" | sort -Vr | sed -n '1p')" = "b"; then
exit_with_error "missing sort -V, please install a newer version of sort"
fiThere was a problem hiding this comment.
There is a shellcheck complaint here, SC2028. I can solve it by replacing the echo with printf. I'm guessing it'd be ok to assume all shells have printf installed?
…d' can do the job
|
I tested this with I will leave it to @greg-szabo to approve and merge the PR. |
| if [ -z "$(which sort || echo)" ]; then | ||
| exit_with_error "missing sort, please install it" | ||
| fi | ||
| if ! test "$(echo "a\nb" | sort -Vr | sed -n '1p')" = "b"; then |
There was a problem hiding this comment.
Ok, fair point, sed is in the list of requirements. But I really want to get rid of it one of these days.
Interestingly, we use grep extensively but it wasn't added to the requirements for some reason.
I guess, one of these days I'll open a "reduce requirements" issue.
| # Returns empty string if --long is not supported | ||
| get_sdk_version() { | ||
| GAIAD_BINARY="$(get_gaiad_binary "$1")" | ||
| RESULT="$("$GAIAD_BINARY" version --long 2>&1 | sed -n '/cosmos_sdk_version/ s/cosmos_sdk_version: v\(.*\)$/\1/p')" |
Description
This PR introduces support for the
genesissub-command added in CosmosSDK 0.47, addressing an issue that caused binaries built with CosmosSDK 0.47+ to fail when starting in gaid manager.closes: #7
Changes
Two new functions have been added to lib-gm:
get_sdk_versionandget_genesis_cmd_prefix.get_sdk_versionThis function retrieves the version of the Cosmos SDK used to build the binary by utilizing the
version --longsub-command. If--longis not supported by the binary, it returns an empty string.get_genesis_cmd_prefixThis function returns "genesis" if the Cosmos SDK version is
>=0.47.0. If the Cosmos SDK version is an empty string, it follows a similar strategy used for handling thetendermintsub-command, returning "genesis" if "gentx" is not listed as an available sub-command when running the binary. In all other cases, it returns an empty string.Commit Message / Changelog Entry
The
CHANGELOG.mdhas been updated accordingly. Maintainer edits are enabled, allowing for easy modifications if there's disagreement with any design choices above. For instance, theget_sdk_versioncommand might not be necessary, though it could prove useful in the future, especially if an SDK-version flag is added to the config. No strong preference is held.Due to trial and error, the number of commits is high. The following commit message is suggested for when this PR is squash merged: