Add absolute path for lib gm #1363
Conversation
JonathanLorimer
requested review from
adizere,
ancazamfir and
romac
as code owners
|
Thank you for your contribution Jonathan! Could you please update As for reviewing this, I'd like to kindly ask Greg to have a look. |
There was a problem hiding this comment.
Hi @JonathanLorimer , thanks for your contribution! This is an interesting use-case that I didn't think about. I wonder why you chose LIB_GM_PATH (which fixes the file name to lib-gm) instead of simply LIB_GM (which allows you to override the filename too). Since it reminds me of the LD_LIBRARY_PATH thingies, I'm happy with it. I hope we won't have to add other libraries.
Dissecting bash/sh scripts into readable parts is hard. (and annoying because of all these edge-cases) I'm happy that this code is useful to you and I hope we can keep it up-to-date and useful.
I made one small suggestion to fix an edge-case that can cause an error but apart from that, the code looks good.
scripts/gm/bin/gm
Outdated
| SCRIPT_0="${0:-$HOME/.gm/bin/gm}" | ||
| export SCRIPT_DIR="${SCRIPT_0%%gm}" | ||
| export LOCAL_LIB_GM="${SCRIPT_DIR}lib-gm" | ||
| if [ -f "$LOCAL_LIB_GM" ]; then | ||
| . "$LOCAL_LIB_GM" | ||
| elif [ -f "$HOME/.gm/bin/lib-gm" ]; then | ||
| . "$HOME/.gm/bin/lib-gm" | ||
| elif [ -f "$LIB_GM_PATH/lib-gm" ]; then |
There was a problem hiding this comment.
I had a hunch that the impression that HOME is always set (which might not be true) might lead to other similar expectations.
The code looks good, but if lib-gm is not available and LIB_GM_PATH is not set, it will return
./gm: line 14: LIB_GM_PATH: unbound variable
instead of the expected
ERROR: could not find lib-gm, exiting...
It's an easy fix, we just need a default for LIB_GM_PATH. Here's one example:
| elif [ -f "$LIB_GM_PATH/lib-gm" ]; then | |
| elif [ -f "${LIB_GM_PATH:-}/lib-gm" ]; then |
There was a problem hiding this comment.
Oh wow, good catch. I confirmed this fix works and made the changes. In cosmos.nix I am running shellcheck on these scripts and I am surprised this didn't get caught. I guess it isn't always required to default vars in case they are unset? Anyways, glad you spotted this.
Co-authored-by: Greg Szabo <16846635+greg-szabo@users.noreply.github.com>
* add absolute path from env var * testing * quick fix * remove echo * minor bash fix * Update scripts/gm/bin/gm Co-authored-by: Greg Szabo <16846635+greg-szabo@users.noreply.github.com> Co-authored-by: Greg Szabo <16846635+greg-szabo@users.noreply.github.com>
Description
This is a tiny pr that adds the possibility to provide lib gm via the environment variable
$LIB_GM_PATH. This helps me close informalsystems/cosmos.nix#34 by allowing me to wrap thegmexecutable and provide all the runtime dependencies (namelystomlandsconfig). This wrapping, means that thegmexecutable is no longer located next tolib-gm, and I don't want to force the user to move lib-gm into their$HOMEdirectory. Therefore I need a third option, to provide an absolute path tolib-gm. I hope that this change isn't too intrusive!closes #1365
For contributor use:
unclog.docs/) and code comments.Files changedin the Github PR explorer.