Skip to content

feat: set and shopts directives - #929

Merged
andreynering merged 2 commits into
masterfrom
set-and-shopts
Jan 14, 2023
Merged

andreynering merged 2 commits into
masterfrom
set-and-shopts

Conversation

@pd93

@pd93 pd93 commented Nov 10, 2022 •

Copy link
Copy Markdown
Member

Summary

This PR adds 2 new directives:

  • set - []string - Equivalent of the POSIX set builtin
  • shopts - []string - Equivalent of the Bash shopt builtin

Both of these directives are available at global, task and command level. See example Taskfile below:

version: '3'

# Global-level - Set for all commands in all tasks
set: [e] # Shorthand flags work for set too
shopts: [globstar]

tasks:
  default:
    # Task-level - Set for all command in this task
    set: [pipefail]
    shopts: [globstar]
    cmds:
      # Command-level - Set for just this command
      - cmd: shopt
        shopts: [globstar]
      - cmd: set -o
        set: [allexport]

Considerations

  • mvdan/sh allows us to set POSIX shell options via interp.Params(). However, from what I can tell, there is no equivalent for Bash options. This means that this PR is currently prepending the shopt builtin before each command where shopts are specified. I'm not a huge fan of this and have been looking at contributing a change to mvdan/sh to add a new interp.Shopts() function or similar. I'd love some input from @mvdan on whether this is a good approach.
  • Originally, I implemented both directives as a map[string]bool so that you could enable/disable opts at different levels. However, I came to the conclusion that this level of complexity wasn't really necessary as all the currently supported shell options are disabled by default anyway. Implementing as a []string still allows us to enable at each level and I think this covers most use cases. It also keeps the Taskfile looking clean as we can inline the list with [].
  • I did also consider merging the two directives as none of the option names conflict. However, this seems confusing and would require us to maintain a list of both inside Task so that we know how to pass them into the interpreter. I think its easier just to keep them separate.
  • The names set and shopts are definitely up for debate. I wasn't sure about the plurality for example...

Currently supported shell options

POSIX (set)

If you have gosh installed, you can find these by running gosh -c "set -o".

Option Name Short Name Default
allexport a off
errexit e off
noexec n off
noglob f off
nounset u off
xtrace x off
pipefail off

Bash (shopt)

If you have gosh installed, you can find these by running gosh -c "shopt".

Option Name Default
expand_aliases off
globstar off
nullglob off

Don't have gosh?

If you don't have gosh installed, you can use the following Taskfile to get these lists:

version: '3'

tasks:
  default:
    cmds:
      - set -o
      - shopt

@pd93 pd93 linked an issue Nov 10, 2022 that may be closed by this pull request

@andreynering andreynering left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

First look.

I liked the direction here, but adding interp.BashOpts to mvdan/sh first would be great IMO.

Comment thread internal/execext/exec.go Outdated
Comment on lines +58 to +61
// Manually adding bash opts before each command as there is no interp.Shopts
if len(opts.BashOpts) > 0 {
s = append(s, fmt.Sprintf("shopt -s %s", strings.Join(opts.BashOpts, " ")))
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree on contributing to mvdan/sh adding a interp.BashOpts option. I think it may be even worth waiting for it to be released before merging this here (but if it takes too long, it's fine to merge as is and adapt later).

Comment thread internal/execext/exec.go Resolved
Comment thread internal/execext/exec.go Resolved
@pd93

pd93 commented Jan 7, 2023 •

Copy link
Copy Markdown
Member Author

@andreynering I've fixed the issues you raised, but haven't had the time to look into the codebase over at mvdan/sh yet. I've opened an issue (mvdan/sh#962) over there instead. Hopefully I (or someone else) will have the time to work on this soon. In the meantime, if you're happy to merge this as-is, I can open another issue to track the improvements once (if) the functionality is added upstream.

@pd93
pd93 requested a review from andreynering January 7, 2023 19:57
@pd93
pd93 marked this pull request as ready for review January 7, 2023 20:01
- Rename `shopts` to `shopt`
- Move `UniqueJoin` to a more appropriate package
- Write documentation
- Add entry to README

@andreynering andreynering left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, let's get this merged and we can improve once mvdan/sh#962 gets implemented.

I've renamed shopts to shopt for consistency with the proper builtin. Also added documentation and README entry.

Thanks again!

@andreynering
andreynering merged commit 1c1be68 into master Jan 14, 2023
@andreynering
andreynering deleted the set-and-shopts branch January 14, 2023 19:41
pinpox added a commit to pinpox/lollypops that referenced this pull request Jan 15, 2023
@marco-m

marco-m commented Jan 16, 2023

Copy link
Copy Markdown
Contributor

I love

tasks:
  default:
    # Task-level - Set for all command in this task
    set: [pipefail]

thanks!!!

@pd93

pd93 commented Jan 16, 2023

Copy link
Copy Markdown
Member Author

Yes, let's get this merged and we can improve once mvdan/sh#962 gets implemented.

I've renamed shopts to shopt for consistency with the proper builtin. Also added documentation and README entry.

@andreynering Thanks for tidying this up. I forgot to add the docs/schema etc. when marking as ready. I've opened #984 to track the improvements as discussed.

Comment thread docs/docs/usage.md
builtins. This can be added at global, task or command level.

```yaml
version: '2'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@pd93 I think that this is an error and should be instead:

version: '3'

?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@marco-m-pix4d good spot! yes, this should be version: '3'. I'll get this updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for working with shell options

4 participants