For bug 3707 we added our own option parser in order to get the subcommand handling we wanted. However it also adds a number of other behaviours we want from our argument parser, such as sorting and surrogate detection. We should therefore use this parser in all our tools to give a consistent behaviour. The current offenders are: * tl-config/hivetool (getopt) * tl-env (getopt) * tl-ldap-certalias (getopt) * tl-limit-printers (getopt) * tl-mount-localdrives (getopt) * tl-notify (optparse) * tl-session-param (getopt) * tl-setup (optparse) * tl-sso-password (getopt) * tl-sso-token-passphrase (getopt) * tl-support (getopt) * tl-umount-localdrives (getopt) We should probably also add an automatic test once these are fixed that makes sure we don't use the "wrong" parser in any future code.
I'm looking through the commits for tl-env, and the change in option parser introduces a change in behavior that would require users to revise how they use the script. This is in cases where the command, tl-env is supposed to run, is also provided with its own arguments and options. For example if you try to run ls -la through tl-env, with the old getopt-parser you can simply run > tl-env ls -la and the options "-la" will be assumed to belong to ls. With the new usage of our option parser, that extends optparse.OptionParser, the very same call will result in "-l" being interpreted as belonging to tl-env (which has no such option). To work around this, one can provide the command with options and arguments but add "--" before the command. > tl-env -- ls -la This change in behavior could have unforeseen implications for our users, e.g. arguments for sub commands silently being interpreted as arguments to tl-env etc. Hence, we should look at preserving the old behavior when using the new OptionParser.
The reason this issue occurs with optparse is that it by default supports interspersed arguments, where it assumes that simple options that take no arguments can come in any order [1], which means that it will not stop parsing on the first non-option argument. Hence, as with the ls example in comment 6, it would then assume that -l is to be interpreted as an option to tl-env. [1] https://docs.python.org/3/library/optparse.html#querying-and-manipulating-your-option-parser Thankfully, optparse provides a method for turning this behavior off, which we can use to get the behavior we had with the getopt parser while still using our optparse implementation.
Tested running tl-config (hivetool under the hood) after swapping to our option parser, and the behaviour is equivalent to 4.16.0 except for some minor changes: * To show the help, flag '-?' has been replaced with flag '-h'. This is more in line with both our other scripts and general Linux syntax. * To show the version number, flag '-v' has been removed, so '--version' is now the only flag to show the version number. Other scripts that use our option parser don't allow '-v', so it is better to be consistent with our other scripts. Both of these flags should be rare to use in scripts, so breaking backwards compatibility here should have very limited impact. Note that the swap to using ThinLinc's option parser in hivetool means that hivetool no longer is independent from ThinLinc. Below is a list of the commands I tested on both 4.16.0 and 4.16.0post: https://www.cendio.com/resources/docs/tag-devel/html/man/tl-config.1.html > tl-config -a shadowing > tl-config --recursive -a /shadowing > tl-config --recursive --all-entries /shadowing > tl-config -r /opt/thinlinc/etc/conf.d/shadowing.hconf -Ra / > tl-config --root /opt/thinlinc/etc/conf.d/shadowing.hconf -Ra / > tl-config /shadowing/shadowing_mode="wooo" > > # -R/--recursive must come before -a/--all-entries > tl-config -aR /shadowing > tl-config -a --recursive /shadowing > # Error output for both calls: > # /shadowing: No such parameter > # R: Folder not found > > # Output when using -e/--eval does not include variable value > tl-config -e /shadowing/shadowing_mode="nootnoot" > # Output: /shadowing/shadowing_mode='' > tl-config --eval /shadowing/shadowing_mode="nootnoot" > # Output: /shadowing/shadowing_mode='' > > tl-config -E /vsmserver > tl-config -x -E /shadowing > tl-config --export -E /shadowing > > # Purging duplicate params I put in vsmserver.hconf > tl-config -p /opt/thinlinc/etc/conf.d/vsmserver.hconf > tl-config --purge /opt/thinlinc/etc/conf.d/vsmserver.hconf > > # Added new params in test.hconf, can list them after import > tl-config -i test.hconf > tl-config --import test.hconf > > tl-config -h > tl-config --help > tl-config --version > > tl-config -? > tl-config -v > # No longer works, flags have been removed/replaced > # Output: > # Usage: hivetool [options] [type:]parameter[=value] ... > # > # hivetool: error: no such option: -v
tl-ldap-certalias has the following options available: * -v, --verbose * -d, --debug * -s, --simulate * -h, --help * --version Tested the following combinations, and the output was identical to how it was in 4.16.0: > tl-ldap-certalias -s > tl-ldap-certalias -d > tl-ldap-certalias -v > tl-ldap-certalias '-s -d' > tl-ldap-certalias '-s -d -v' > tl-ldap-certalias '-s -v' Also ran test_5880 [1] on both versions and got the same results. [1] https://git.cendio.se/cendio/test_5880
tl-session-param has the following options available: * -a, --all-entries * -e, --eval * -h, --help * --version We tested the following on 4.16.0 and 4.16.0post: In 4.16.0, the examples from the man page are printed as well. Otherwise, the output is identical: > /opt/thinlinc/bin/tl-session-param The following commands have the same output in both versions: > /opt/thinlinc/bin/tl-session-param -a / > /opt/thinlinc/bin/tl-session-param -e LANG=/client_params/capabilities/client_lang > /opt/thinlinc/bin/tl-session-param /client_ip > /opt/thinlinc/bin/tl-session-param -e LANG=/client_params/capabilities/client_lang -a / This command has a regression since 4.16.0, in 4.16.0 we get: > /opt/thinlinc/bin/tl-session-param /client_ip -e LANG=/client_params/capabilities/client_lang > 10.47.1.80 > No such parameter as -e and in 4.16.0post we get: > LANG=en_GB.UTF-8 > 10.47.1. Seems like the argument (/client_lang) has to come last for it to work: > /opt/thinlinc/bin/tl-session-param -e LANG=/client_params/capabilities/client_lang -a / /client_ip Just as in comment #14, the '-?' flag has been replaced with '-h' and '--help'. Overall, the script looks good and behaves better now than in 4.16.0.
