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

Using flag.conflicts causes issues with set -u #559

Closed
thendricks0 opened this issue Oct 16, 2024 · 13 comments
Closed

Using flag.conflicts causes issues with set -u #559

thendricks0 opened this issue Oct 16, 2024 · 13 comments
Labels
bug Something isn't working

Comments

@thendricks0
Copy link

Bashly Version

Latest Docker Image

Description

Using flag.conflicts causes issues with set -u.
If I use strict mode or rather set -u and then try to use conflicting variables I get an unbound variable error since $key is unbound.

Contents of bashly.yml

flags:
  - long: --verbose
    short: -v
    help: Show verbose output
    conflicts: [--quiet]
  - long: --debug
    short: -d
    help: Show debug output
    conflicts: [--quiet]
  - long: --quiet
    short: -q
    help: Suppress output, this implies --force so all prompts are confirmed
    conflicts: [--verbose, --debug]

Reproduction Steps

Use the provided bashly config + settings.yml below, generate a script and then execute the script with -qd or -qv or -qdv.

# settings.yml

strict: true
# or use
strict: set -o nounset

Actual Behavior

parse_requirements will be generated like this:

parse_requirements() {
  # :command.fixed_flags_filter
  while [[ $# -gt 0 ]]; do
    case "${1:-}" in
      --version)
        version_command
        exit
        ;;

      --help | -h)
        long_usage=yes
        awps_usage
        exit
        ;;

      # :flag.case
      --verbose | -v)
        # :flag.conflicts
        if [[ -n "${args['--quiet']:-}" ]]; then
          printf "conflicting options: %s cannot be used with %s\n" "$key" "--quiet" >&2
          exit 1
        fi

        # :flag.case_no_arg
        args['--verbose']=1
        shift
        ;;

      # :flag.case
      --debug | -d)
        # :flag.conflicts
        if [[ -n "${args['--quiet']:-}" ]]; then
          printf "conflicting options: %s cannot be used with %s\n" "$key" "--quiet" >&2
          exit 1
        fi

        # :flag.case_no_arg
        args['--debug']=1
        shift
        ;;

      # :flag.case
      --quiet | -q)
        # :flag.conflicts
        for conflict in --verbose --debug; do
          if [[ -n "${args[$conflict]:-}" ]]; then
            printf "conflicting options: %s cannot be used with %s\n" "$key" "$conflict" >&2
            exit 1
          fi
        done

        # :flag.case_no_arg
        args['--quiet']=1
        shift
        ;;
...

Expected Behavior

But it should be generated like this:

parse_requirements() {
  # :command.fixed_flags_filter
  while [[ $# -gt 0 ]]; do
    key="$1"   # <----------- this!
    case "${key:-}" in
      --version)
        version_command
        exit
        ;;

      --help | -h)
        long_usage=yes
        awps_usage
        exit
        ;;

      # :flag.case
      --verbose | -v)
        # :flag.conflicts
        if [[ -n "${args['--quiet']:-}" ]]; then
          printf "conflicting options: %s cannot be used with %s\n" "$key" "--quiet" >&2
          exit 1
        fi

        # :flag.case_no_arg
        args['--verbose']=1
        shift
        ;;

      # :flag.case
      --debug | -d)
        # :flag.conflicts
        if [[ -n "${args['--quiet']:-}" ]]; then
          printf "conflicting options: %s cannot be used with %s\n" "$key" "--quiet" >&2
          exit 1
        fi

        # :flag.case_no_arg
        args['--debug']=1
        shift
        ;;

      # :flag.case
      --quiet | -q)
        # :flag.conflicts
        for conflict in --verbose --debug; do
          if [[ -n "${args[$conflict]:-}" ]]; then
            printf "conflicting options: %s cannot be used with %s\n" "$key" "$conflict" >&2
            exit 1
          fi
        done

        # :flag.case_no_arg
        args['--quiet']=1
        shift
        ;;
...
@thendricks0 thendricks0 added the bug Something isn't working label Oct 16, 2024
@DannyBen
Copy link
Owner

DannyBen commented Oct 16, 2024

I can't reproduce the problem. Using the provided bashly.yml and strict: true in the settings produces a cli that outputs the expected result:

$ ./cli -qd
conflicting options: -d cannot be used with --quiet

and shellcheck does not have any complaint:

Are you on bashly 1.2.2?

The only place the generated script has the variable $key in it is here:

  # :command.parse_requirements_while
  while [[ $# -gt 0 ]]; do
    key="$1"
    case "$key" in
      # :flag.case

and I don't see how it can be unbound.


As a side note: All of the tests and example tests run in strict mode, including this conflicts example.

@thendricks0
Copy link
Author

I use the latest bashly image as an alias.
I just created a new project using bashly init, pasted in the flags of the ticket and set the strict option in settings.yml to true.
Then I immediately get the unbound variable error:

$ ./cli -qdv download
./cli: line 299: key: unbound variable

This is how the parse_requirements function is generated:

# :command.parse_requirements
parse_requirements() {
  # :command.fixed_flags_filter
  while [[ $# -gt 0 ]]; do
    case "${1:-}" in
      --version)
        version_command
        exit
        ;;

      --help | -h)
        long_usage=yes
        cli_usage
        exit
        ;;

      # :flag.case
      --verbose | -v)
        # :flag.conflicts
        if [[ -n "${args['--quiet']:-}" ]]; then
          printf "conflicting options: %s cannot be used with %s\n" "$key" "--quiet" >&2
          exit 1
        fi

        # :flag.case_no_arg
        args['--verbose']=1
        shift
        ;;

      # :flag.case
      --debug | -d)
        # :flag.conflicts
        if [[ -n "${args['--quiet']:-}" ]]; then
          printf "conflicting options: %s cannot be used with %s\n" "$key" "--quiet" >&2
          exit 1
        fi

        # :flag.case_no_arg
        args['--debug']=1
        shift
        ;;

      # :flag.case
      --quiet | -q)
        # :flag.conflicts
        for conflict in --verbose --debug; do
          if [[ -n "${args[$conflict]:-}" ]]; then
            printf "conflicting options: %s cannot be used with %s\n" "$key" "$conflict" >&2
            exit 1
          fi
        done

        # :flag.case_no_arg
        args['--quiet']=1
        shift
        ;;

      *)
        break
        ;;

    esac
  done

$key is definitely unbound here.

The docker image uses bashly 1.2.2, I checked and the header of my generated script also says so

@thendricks0
Copy link
Author

thendricks0 commented Oct 16, 2024

As a side note: All of the tests and example tests run in strict mode, including this conflicts example.

Your conflicts example only works as long as there are no commands set in the bashly.yml

As soon as I add commands the script is generated with the unbound variable:

name: download
help: Sample application to demonstrate the use of conflicting flags
version: 0.1.0

flags:
- long: --cache
  help: Enable cache
  # Running --cache with --no-cache is not permitted
  conflicts: [--no-cache]
- long: --no-cache
  help: Disable cache
  # Running --no-cache with --cache or with --fast is not permitted
  conflicts: [--cache, --fast]
- long: --fast
  help: Run faster
  # Make sure to add the conflicting flags in both flags
  conflicts: [--no-cache]

commands:
- name: test
  alias: t
  help: Example to show parse_requirements is bugged
$ ./download --cache --no-cache
./download: line 206: key: unbound variable

This is where the fixed_flags_filter is generating the case where the key var is not bound:

Without commands, the flags are generated using parse_requirements_while

And as soon as commands are added, all flags are generated with the fixed_flags_filter. Dunno if this is intended because commands can have flags or if there is a deeper issue here. But adding the key=$1 assignment within fixed_flags_filter.gtx would probably fix this issue.

@DannyBen
Copy link
Owner

Confirmed.
Your original reproduction steps did not reproduce, but using the last bashly.yml, I can see the issue.

Will be fixed.

@thendricks0
Copy link
Author

Thanks!

@DannyBen
Copy link
Owner

In fact, the problem is deeper and more specific.
This error occurs only when there is conflicts in the "fixed args" - meaning, when flags are defined alongside commands.

For example, this works:

commands:
- name: test
  alias: t
  help: Example to show parse_requirements is bugged

  flags:
  - long: --fast
    short: -f
    conflicts: [--slow]
  - long: --slow
    short: -s
    conflicts: [--fast]

The fixed args while uses $1 directly without setting $key.
Hopefully will be fixed in the next hour or so, it would help if you can test and verify once it is.

@thendricks0
Copy link
Author

Sure! I'm currently re-writing a script using bashly and so far it has been a good experience and I actually need this to work, so I'll test as soon as you can provide a fix for it.

A little offtopic here, but I wondered if you know where I can find more libs for bashly? Using the github search doesn't provide very good results since "bashly" is fuzzy matched against "bash" so I get way too many results, google doesn't provide any good results. I thought since your project had 2k stars there would be more libs around, since I'm really missing some good logging library.
I wrote my own but I'm not really satisfied with it tbh.

@DannyBen
Copy link
Owner

DannyBen commented Oct 16, 2024

Happy to hear it, and if something does not work as you expect, I am happy to help in your migration (issues / discussions are both fine).

As for libs for bashly - I don't know if there are any, but as you probably know by now - these are just functions that you place in src/lib - so you can search for general functions, or ask one of the AI chats to write you that function, and there you have it.

@DannyBen
Copy link
Owner

This should be fixed now in #560, merged to master, and deployed to the docker edge tag.

You can alias like this:

$ docker pull dannyben/bashly:edge
$ alias bashly-edge='docker run --rm -it --user $(id -u):$(id -g) --volume "$PWD:/app" dannyben/bashly:edge'

@thendricks0
Copy link
Author

thendricks0 commented Oct 16, 2024

Works!

$ ./cli -qv test
conflicting options: -q cannot be used with --verbose
$ ./cli -qd test
conflicting options: -q cannot be used with --debug

@DannyBen
Copy link
Owner

Excellent. I want to fix #561 and then I will release (probably later today).

Thanks for the report.

@thendricks0
Copy link
Author

Great! Thanks a lot for the quick fix!

@DannyBen
Copy link
Owner

Version 1.2.3 is live with this fix.
Thanks again for reporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants