5

The shell script I'm trying to use keeps giving this error:

$ ./script.sh: line 2: [: missing `]' 
grep: ]: No such file or directory

The line is part of a section trying to check if a particular process is going to have a file locked:

COUNTER=0
while [ ps aux | grep "[r]elayevent.sh" ] && [ "$COUNTER" -lt 10 ]; do
    sleep 3
    let COUNTER+=1
done

Obviously I've checked that the brackets all pair up correctly - which looks fine to me. Also the common white space around the condition issue doesn't apply.

What am I missing here?

Prvt_Yadav
  • 5,882
Kiwi Cam
  • 61
  • 2
    Note for the future. Whenever you get a syntax error, run - don't walk - straight to https://shellcheck.net/ and paste in the block of your code that exhibits the issue. – Chris Davies Feb 06 '19 at 08:37
  • You should use flock command to implement locks in scripts. – dnt Feb 06 '19 at 10:03

2 Answers2

10

The error is that you should remove first [ because of you want to check the exit status then use command directly.

The Wiki pages of the Shellcheck tool have an explanation for this (issue SC1014):

[ .. ] is not part of shell syntax like if statements. It is not equivalent to parentheses in C-like languages, if (foo) { bar; }, and should not be wrapped around commands to test.

[ is just regular command, like whoami or grep, but with a funny name (see ls -l /bin/[). It's a shorthand for test.

If you want to check the exit status of a certain command, use that command directly.

If you want to check the output of a command, use "$(..)" to get its output, and then use test or [/[[ to do a string comparison:

Also use ps aux | grep -q "[r]elayevent.sh" so that you will get the exit status silently instead of printing anything to stdout.

Or you can use pgrep and direct it's output to /dev/null.

Use second condition first because it will be more efficient for the last case.

So final script will be like:

#!/bin/bash
COUNTER=0

while [ "$COUNTER" -lt 10 ] && ps aux | grep -q "[r]elayevent.sh"   ; do


sleep 3

let COUNTER+=1




done

Or

#!/bin/bash
COUNTER=0

while [ "$COUNTER" -lt 10 ] && pgrep  "[r]elayevent.sh" >/dev/null  ; do


sleep 3

let COUNTER+=1




done
Community
  • 1
Prvt_Yadav
  • 5,882
  • 4
    You probably want to add -q when calling grep, too. Otherwise it prints the matching lines. – Mikel Feb 06 '19 at 04:21
  • @Mikel thanks, it will silently give status instead of printing anything. – Prvt_Yadav Feb 06 '19 at 04:25
  • You should mention where that quote is from. Also, grep -q "[r]elayevent.sh" doesn't make much sense, and ps | grep is usually better replaced with pgrep; but the let COUNTER+=1 is fine in bash, zsh and ksh39 (((..)) is not a bit more portable) -- though the whole script doesn't make much sense ;-)). –  Feb 06 '19 at 04:59
  • 1
    I would swap the tests around to avoid running grep on the last iteration. – Kusalananda Feb 06 '19 at 07:31
  • That is more efficient. – Prvt_Yadav Feb 06 '19 at 07:49
  • That's perfect, works exactly as I wanted. Thanks so much for the thorough explanation. – Kiwi Cam Feb 07 '19 at 19:04
4

You can't have a pipe inside [ ... ]. It's also better to use pgrep than to try to parse the output of ps:

count=0
while [ "$count" -lt 10 ] && pgrep relayevent.sh >/dev/null; then
    sleep 3
    count=$(( count + 1 ))
done

BSD systems could use pgrep -q ... instead of pgrep ... >/dev/null to discard the actual output of pgrep, just as with ordinary grep (we're only interested in the exit status).

Note how we don't put the pgrep command within [ ... ]. That's because we're not interested in its output, only its exit status. With [ ... ] you commonly compare strings or numbers. The [ ... ] will result in an exit status that is zero (true) or non-zero (false), just like the pgrep execution.

However, this does not check for any locking mechanisms, only whether a particular process is running or not.

If you are trying to get only a single instance of a script running, then it's better to do something like this (assuming the EXIT trap is executed whenever the script is terminating orderly):

lockdir=dir.lock

if mkdir "$lockdir"; then
    trap 'rmdir "$lockdir"' EXIT
else
    echo 'Only one instance of this script allowed' >&2
    exit 1
fi

With a number of tries and sleep:

lockdir=dir.lock

count=0
while [ "$count" -lt 10 ]; then
    if mkdir "$lockdir"; then
        trap 'rmdir "$lockdir"' EXIT
        break
    else
        echo 'Locked. Sleeping...' >&2
        sleep 3
    fi

    count=$(( count + 1 ))
done

if [ "$count" -eq 10 ]; then
    echo 'Giving up.' >&2
    exit 1
fi

Related:

Kusalananda
  • 333,661