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

Style: Reformat & Check Shell Code With `shfmt` #2230

Merged
merged 9 commits into from Oct 14, 2018

Conversation

Projects
None yet
2 participants
@sanssecours
Contributor

sanssecours commented Sep 8, 2018

Checklist

  • I ran all tests locally and everything went fine.
  • The documentation regarding the switch from checkbashism to shfmt should be up to date.
@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Sep 10, 2018

Contributor

Please rebase.

Are we sure that shfmt implements everything checkbashism does?

It sounds a bit heavyweight to require to install shfmt. How can someone format their shell scripts so that our build server accepts it?

Contributor

markus2330 commented Sep 10, 2018

Please rebase.

Are we sure that shfmt implements everything checkbashism does?

It sounds a bit heavyweight to require to install shfmt. How can someone format their shell scripts so that our build server accepts it?

@sanssecours

This comment has been minimized.

Show comment
Hide comment
@sanssecours

sanssecours Sep 11, 2018

Contributor

Are we sure that shfmt implements everything checkbashism does?

I am pretty sure it does not. However shfmt also does not print false positives as far as I can tell.

It sounds a bit heavyweight to require to install shfmt.

The package requires 3.7MB on my machine:

brew info shfmt
#> shfmt: stable 2.5.1 (bottled), HEAD
#> Autoformat shell script source code
#> https://github.com/mvdan/sh
#> /usr/local/Cellar/shfmt/2.5.1 (5 files, 3.7MB) *
#>   Poured from bottle on 2018-08-24 at 19:11:48
#> From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/shfmt.rb
#> ==> Dependencies
#> Build: go ✘
#> ==> Options
#> --HEAD
#> 	Install HEAD version
#> ==> Analytics
#> install: 575 (30d), 1471 (90d), 3413 (365d)
#> install_on_request: 573 (30d), 1468 (90d), 3408 (365d)
#> build_error: 0 (30d)

. That is about two thirds of the space clang-format requires (6.0MB). I would not consider this “heavyweight”.

How can someone format their shell scripts so that our build server accepts it?

I think the easiest option is to just install shfmt and then call

shfmt -s -sr filename

. If you want you can also install a plugin to reformat the code directly inside your editor. I know there is no Debian package for shfmt yet:

Packages are available for Arch, CRUX, Homebrew, NixOS and Void.

. However, I do not think installing a current version of Go (1.9+) and then calling

go get -u github.com/mvdan/sh/cmd/shfmt

is that big of a problem. Maybe you can also install one of the shfmt binaries directly, then you do not need to install Go at all.

Contributor

sanssecours commented Sep 11, 2018

Are we sure that shfmt implements everything checkbashism does?

I am pretty sure it does not. However shfmt also does not print false positives as far as I can tell.

It sounds a bit heavyweight to require to install shfmt.

The package requires 3.7MB on my machine:

brew info shfmt
#> shfmt: stable 2.5.1 (bottled), HEAD
#> Autoformat shell script source code
#> https://github.com/mvdan/sh
#> /usr/local/Cellar/shfmt/2.5.1 (5 files, 3.7MB) *
#>   Poured from bottle on 2018-08-24 at 19:11:48
#> From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/shfmt.rb
#> ==> Dependencies
#> Build: go ✘
#> ==> Options
#> --HEAD
#> 	Install HEAD version
#> ==> Analytics
#> install: 575 (30d), 1471 (90d), 3413 (365d)
#> install_on_request: 573 (30d), 1468 (90d), 3408 (365d)
#> build_error: 0 (30d)

. That is about two thirds of the space clang-format requires (6.0MB). I would not consider this “heavyweight”.

How can someone format their shell scripts so that our build server accepts it?

I think the easiest option is to just install shfmt and then call

shfmt -s -sr filename

. If you want you can also install a plugin to reformat the code directly inside your editor. I know there is no Debian package for shfmt yet:

Packages are available for Arch, CRUX, Homebrew, NixOS and Void.

. However, I do not think installing a current version of Go (1.9+) and then calling

go get -u github.com/mvdan/sh/cmd/shfmt

is that big of a problem. Maybe you can also install one of the shfmt binaries directly, then you do not need to install Go at all.

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Sep 12, 2018

Contributor
go get -u github.com/mvdan/sh/cmd/shfmt
package github.com/mvdan/sh/cmd/shfmt: cannot download, $GOPATH not set. For more details see: go help gopath

I would rather discuss in the next meeting if this dependency is acceptable for everyone.

Contributor

markus2330 commented Sep 12, 2018

go get -u github.com/mvdan/sh/cmd/shfmt
package github.com/mvdan/sh/cmd/shfmt: cannot download, $GOPATH not set. For more details see: go help gopath

I would rather discuss in the next meeting if this dependency is acceptable for everyone.

@sanssecours

This comment has been minimized.

Show comment
Hide comment
@sanssecours

sanssecours Sep 12, 2018

Contributor
go get -u github.com/mvdan/sh/cmd/shfmt
package github.com/mvdan/sh/cmd/shfmt: cannot download, $GOPATH not set. For more details see: go help gopath

You can download shfmt binaries for common operating systems here. For example, to install the tool on a 64 Bit x86 computer running Linux just download the binary from here and put it into a directory that is part of your PATH. You do not need to install Go for the tool to work at all. I successfully tested that this works in one of the latest commits.

I would rather discuss in the next meeting if this dependency is acceptable for everyone.

Works for me, although I do not think it is that big of a problem. Especially since a lot of contributors do not work on Elektra’s Shell code at all.

Contributor

sanssecours commented Sep 12, 2018

go get -u github.com/mvdan/sh/cmd/shfmt
package github.com/mvdan/sh/cmd/shfmt: cannot download, $GOPATH not set. For more details see: go help gopath

You can download shfmt binaries for common operating systems here. For example, to install the tool on a 64 Bit x86 computer running Linux just download the binary from here and put it into a directory that is part of your PATH. You do not need to install Go for the tool to work at all. I successfully tested that this works in one of the latest commits.

I would rather discuss in the next meeting if this dependency is acceptable for everyone.

Works for me, although I do not think it is that big of a problem. Especially since a lot of contributors do not work on Elektra’s Shell code at all.

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Sep 12, 2018

Contributor

Ok, the binaries work for me.

And you are right, not many people are modifying shell code.

Nevertheless, it is better to discuss it if new tools are required to develop on Elektra.

Btw. The build servers seem to fail now.

Contributor

markus2330 commented Sep 12, 2018

Ok, the binaries work for me.

And you are right, not many people are modifying shell code.

Nevertheless, it is better to discuss it if new tools are required to develop on Elektra.

Btw. The build servers seem to fail now.

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Oct 11, 2018

Contributor

As discussed: please keep the bashism checks.

Contributor

markus2330 commented Oct 11, 2018

As discussed: please keep the bashism checks.

sanssecours added some commits Sep 7, 2018

sanssecours added some commits Sep 7, 2018

@markus2330 markus2330 merged commit 0a1b010 into ElektraInitiative:master Oct 14, 2018

2 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Oct 14, 2018

Contributor

Thank you!

Contributor

markus2330 commented Oct 14, 2018

Thank you!

@sanssecours sanssecours deleted the sanssecours:🐚 branch Oct 15, 2018

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