Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#61] Command line parsing options #63

Closed
wants to merge 1 commit into from

Conversation

fluca1978
Copy link
Collaborator

Exploit getopt(1) to get command line options availability. The idea is to do a first parse at the very beginning of the script, so that options can be detected and internal and environmental variables can be set accordingly.
Then the command line parsing continues as before.

Added a new section to the documentation about variables and command line flags to illustrate how both can be combined and used.

Close #61

Exploit getopt(1) to get command line options availability.
The idea is to do a first parse at the very beginning of the script,
so that options can be detected and internal and environmental
variables can be set accordingly.
Then the command line parsing continues as before.

Added a new section to the documentation about variables and command
line flags to illustrate how both can be combined and used.

Close theory#62
Copy link
Owner

@theory theory left a comment

Choose a reason for hiding this comment

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

Left some editing comments for the docs, but then tried to run it on my Mac and it utterly failed. Seems the version of getopts that comes on macOS is quite dated. No support for long options, for example.

Digging around, I found some many suggestions to use the Bash getopt builtin, as in the answers to this SO question. No support for long arguments there, either, and options must come right after the command name and not anywhere else. I did some fiddling and got this to work:

diff --git a/bin/pgenv b/bin/pgenv
index 1ca5b91..17992d8 100755
--- a/bin/pgenv
+++ b/bin/pgenv
@@ -21,56 +21,44 @@ set -E
 # to `pgenv`, that is made at the end of the script.
 #
 
-# evaluate arguments via getop
-GETOPT_EVALUTAION=$(getopt -o 'r:R:vc:w:D:' --long 'root:,config-root:,verbose,config-file:,config-file-write:,--pgdata:' -n 'pgenv' -- "$@")
-if [ $? -ne 0 ]; then
-    echo "Cannot parse command line" >&2
-    pgenv_help
-    exit 1
-fi
-
-eval set -- "$GETOPT_EVALUTAION"
-unset GETOP_EVALUTAION
-
-
-# parse the options
-do_parse='true'
-while [ ! -z $do_parse ]; do
-    case "$1" in
-	'-r'|'--root')
-	    PGENV_ROOT=$2
+# evaluate arguments via the bash getopts builtin.
+while getopts 'r:R:vc:w:D:' o; do
+    case "${o}" in
+	r)
+	    PGENV_ROOT="$OPTARG"
 	    shift
 	    shift
 	    ;;
-	'-R'|'--config-root')
-	    PGENV_CONFIG_ROOT=$2
+	R)
+	    PGENV_CONFIG_ROOT="$OPTARG"
 	    shift
 	    shift
 	    ;;
-	'-v'|'--verbose')
+	v)
 	    PGENV_DEBUG=true
 	    shift
 	    ;;
-	'-c'|'--config-file')
-	    PGENV_CONFIGURATION_FILE=$2
+	c)
+	    PGENV_CONFIGURATION_FILE="$OPTARG"
 	    shift
 	    shift
 	    ;;
-	'-w'|'--config-file-write')
-	    PGENV_WRITE_CONFIGURATION_FILE_AUTOMATICALLY=$2
+	w)
+	    PGENV_WRITE_CONFIGURATION_FILE_AUTOMATICALLY="$OPTARG"
 	    shift
 	    shift
 	    ;;
-	'-D'|'--pgdata')
-	    PG_DATA=$2
+	D)
+	    PG_DATA="$OPTARG"
 	    shift
 	    shift
 	    ;;
 	'--')
 	    # end of the getopt parsing
 	    # resume normal command line parsing from here
-	    do_parse=''
+        break
 	    shift
+        shift
 	    ;;
 	*)
 	    echo "Command line parsing error: unrecognized token $1" >&2
@@ -79,11 +67,6 @@ while [ ! -z $do_parse ]; do
     esac
 done
 
-
-
-
-
-
 # Set PGENV_ROOT only if not already set.
 if [ -z "$PGENV_ROOT" ]; then
     PGENV_ROOT=$(dirname $(dirname $0))

But ugh what a mess. This is when I start to wonder whether we'd be better off rewriting this thing in a proper programming language.

Anyway, I'm not sure what to do here. Our options are:

  1. Use the getopt Bash builtin and allow only short args between pgenv and before any command.
  2. Use a simpler form of getopts without long arguments. I fiddled with this a bit and couldn't figure out how to get it to work.
  3. Use getopts as you have it here, but require people to install a newer version (I am not finding one to install on macOS rn)
  4. Write our own option parser, perhaps something like in this SO answer or embed this script
  5. Rewrite this thing in Perl or Ruby and use a proper (default) option library like Getopt::Long --- too much work!
  6. Don't bother with options for now.

Thoughts?

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@fluca1978
Copy link
Collaborator Author

1. Use the `getopt` Bash builtin and allow only short args between `pgenv` and before any command.

Seems limited to me.

2. Use a simpler form of `getopts` without long arguments. I fiddled with this a bit and couldn't figure out how to get it to work.

Again, seems limited to me.

3. Use `getopts` as you have it here, but require people to install a newer version (I am not finding one to install on macOS rn)

I would not like to introduce such a dependency, especially if it is not simple to get it installed!

4. Write our own option parser, perhaps something like in [this SO answer](https://stackoverflow.com/a/21071480) or embed [this script](/ko1nksm/getoptions#embedding-into-a-file)

Could be possible, but I don't know if it is worth. So far, the getopt stuff has been introduced with the purpose to keep under control the number of environment variables to create. I suspect, at this point, using environment variables is the simplest choice.

5. Rewrite this thing in Perl or Ruby and use a proper (default) option library like [Getopt::Long](https://metacpan.org/pod/Getopt::Long) --- too much work!

This is something I would I have been thinking for a while. Writing in Perl seems the right approach to me, also because of your great experience in it. And it will get rid of a few edges, like the whole PostgreSQL version parsing stuff, or the configuration arrays.
It should not be an hard job, and it can be done in parallel with the original pgenv Bash script. But it requires time!

6. Don't bother with options for now.

Well, this is the easiest one, and it means going back to environment variables. So far we have five external variables that can be used to instrument pgenv, assuming we are going to double it in the long running, it is still a number of variables we can take under control. In this case, I would drop this pull request and start over to implement the PGENV_WRITE_CONFIGURATION_FILE_AUTOMATICALLY part only, that is from where all this work spawned.

@theory
Copy link
Owner

theory commented Sep 4, 2023

Agreed, best to just do the PGENV_WRITE_CONFIGURATION_FILE_AUTOMATICALLY bit now and put some thought into the future development. I, too, would like to rewrite it in Perl, but, yeah, TIME.

@fluca1978 fluca1978 closed this Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants