bash-snippets 1.15.1 (new formula)

[alexanderepstein]

• Have you followed the guidelines for contributing?
• Have you checked that there aren't other open pull requests for the same formula update/change?
• Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
• Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

---

[ilovezfs]

Same review comments as here: #15107 (comment)

[alexanderepstein]

I'd like to see this use the upstream install script (looks like the script needs a non-interactive mode) and pass a --prefix= parameter

Does this mean you want to use the install.sh that I have with the repository to install this?

[ilovezfs]

@alexanderepstein yes, instead of a bin.install loop and a man.install.

[alexanderepstein]

I am not too proficient in Ruby, so after a little searching for running a bash script within a ruby script I came up with a result of %x( ./install.sh all ) would that be correct?

[ilovezfs]

system "./install.sh", "--prefix=#{prefix}", "all"

[alexanderepstein]

Does it look correct now? I got rid of the build without options because I am passing the all argument to the install script which installs all of the tools.

[ilovezfs]

@alexanderepstein yes. You'll need to teach the script to accept the prefix option, though :)

[alexanderepstein]

Do you mean that my install script should be able to be run as ./install.sh --prefix=all? If so I can add that to the script. Although right now the script can handle ./install.sh all would that work if I changed this system "./install.sh", "--prefix=#{prefix}", "all"
to system "./install.sh all"
or system "./install.sh", "all"

[ilovezfs]

@alexanderepstein --prefix=#{prefix} will be something like

--prefix=/usr/local/Cellar/bash-snippets/1.15.0

or

--prefix=/opt/local/brew/Cellar/bash-snippets/1.15.0

or

--prefix=/Users/alex/brew/Cellar/bash-snippets/1.15.0

So you'll get something like

./install.sh --prefix=/Users/alex/brew/Cellar/bash-snippets/1.15.0 all

and then need to parse out the prefix directory, and install the commands in $prefix/bin and the man page in $prefix/share/man/man1

[alexanderepstein]

I believe I added this correctly to my install script. I then updated the version and changed the sha256 for v1.15.1.

[ilovezfs]

@alexanderepstein cool, thanks! Now on to the other review comment :)

the test in the test do block should exercise and verify the software's functionality

Take a look at the jose and vim formulae for some good examples:
https://github.com/Homebrew/homebrew-core/blob/master/Formula/jose.rb#L27-L36
https://github.com/Homebrew/homebrew-core/blob/master/Formula/vim.rb#L124-L143

[alexanderepstein]

Will look into writing these. My build is failing because the directory that the file is trying to be copied over to does not exist. How can I create the directories before running my install script in Ruby?

[ilovezfs]

@alexanderepstein your script will need to make sure to create the directories first using mkdir -p $prefix/bin $prefix/share/man/man1. Also, you can't use echo in the script here since that will add a trailing new line character, which is the other reason the script is failing.

You can set the shebang at the top of the script to

#!/usr/bin/env bash -x

to debug it.

[alexanderepstein]

Would echo -n work (this prevents a new line)? And would it work to add something like Dir.mkdir '#{prefix}/bin' and Dir.mkdir '#{prefix}/share/man/man1' to the ruby script. It is easier for me to update the ruby script rather than change the installer and release a new version.

[ilovezfs]

Would echo -n work (this prevents a new line)?

Yes that should work.

And would it work to add something like Dir.mkdir '#{prefix}/bin' and Dir.mkdir '#{prefix}/share/man/man1'. It is easier for me to update the ruby script rather than change the installer and release a new version.

It will need to be part of the install script. (bin.mkpath and man1.mkpath would do it in the ruby but that's not OK for a new formula.)

[alexanderepstein]

I added the two script changes so the build should be successful now (if those were the only issues). And will still look into writing tests. I have 15 tools at the moment that are packaged in bash-snippets do i test all of them or only some?

[alexanderepstein]

Build still failed it seems like either I don't have the necessary permissions to create the directory or it isn't attaching the prefix before creating the directory

[ilovezfs]

@alexanderepstein I think this diff will fix it:

diff --git a/install.sh b/install.sh
index 614519f..5f28336 100755
--- a/install.sh
+++ b/install.sh
@@ -75,7 +75,7 @@ else manPath="/usr/local/man/man1" ;fi
 response=$( echo "$@" | grep -Eo "\-\-prefix")
 
 if [[ $response == "--prefix" ]];then
-  prefix=$(echo -n "$@" | sed  s/--prefix=//g | sed  s/all//g  )
+  prefix=$(echo "$@" | sed -e 's/--prefix=\(.*\) .*/\1/')
   mkdir -p $prefix/bin $prefix/share/man/man1
   for tool in "${tools[@]}"
   do

[alexanderepstein]

Working now with your fix 😄

[ilovezfs]

I have 15 tools at the moment that are packaged in bash-snippets do i test all of them or only some?

Maybe three of the most popular ones?

[alexanderepstein]

That works I will look into adding these tests as soon as possible

[ilovezfs]

Thanks @alexanderepstein!

[ilovezfs]

Thanks!

[alexanderepstein]

If i use assert_equal does that check the entire output ? Is there any assert that works only on the first line? The tools i want to test has output that changes (one is weather tool, currency tool etc..) However the first line remains consistent always.

[ilovezfs]

You can use assert_equal with shell_output("#{bin}/foo").lines.first or just assert_match to match a substring or regex.

[alexanderepstein]

Let me know if those tests are written correctly and work :)

[ilovezfs]

you can leave out each of the -h and -v tests.

[alexanderepstein]

Removed them

[ilovezfs]

@alexanderepstein note I've pushed a commit removing the .chomps for the ones using assert_match since it's not needed.

[alexanderepstein]

Noted. Thank you so much for all your help and working with me to get this into homebrew-core 😄

[ilovezfs]

Thanks for your first contribution to Homebrew, @alexanderepstein! Without people like you submitting PRs we couldn't run this project. You rock!

[ilovezfs]

Thank you so much for all your help and working with me to get this into homebrew-core

You're welcome!