kinow commented on code in PR #607:
URL: https://github.com/apache/opennlp/pull/607#discussion_r1641987177
##########
opennlp-brat-annotator/src/main/bin/brat-annotation-service:
##########
@@ -21,6 +21,28 @@
# may be inadvertantly placed in any output files if
# output redirection is used.
+# determine OPENNLP_HOME - $0 may be a symlink to OpenNLP's home
+PRG="$0"
+
+while [ -h "$PRG" ] ; do
+ ls=`ls -ld "$PRG"`
+ link=`expr "$ls" : '.*-> \(.*\)$'`
+ if expr "$link" : '/.*' > /dev/null; then
+ PRG="$link"
+ else
+ PRG="`dirname "$PRG"`/$link"
+ fi
+done
Review Comment:
Thank you @veita
I asked as I read the code and from a quick read it seemed to have a greedy
regex that could lead to error in a certain scenario. You mentioned it's
well-proven, but do you know if it was tested (e.g. with bats, or another unit
test, covering several cases), or has it just been around for a long time and
never raised any issues?
It's late here, so my logic might be wrong, sorry. Here's what I tried.
```bash
kinow@ranma:/tmp$ mkdir /tmp/opennlp-1568
kinow@ranma:/tmp$ cd /tmp/opennlp-1568
kinow@ranma:/tmp/opennlp-1568$ cat test.sh
#!/bin/bash
PRG="$0"
while [ -h "$PRG" ] ; do
ls=`ls -ld "$PRG"`
link=`expr "$ls" : '.*-> \(.*\)$'`
if expr "$link" : '/.*' > /dev/null; then
PRG="$link"
else
PRG="`dirname "$PRG"`/$link"
fi
done
echo "${PRG}"
kinow@ranma:/tmp/opennlp-1568$ # not a symlink, all good...
kinow@ranma:/tmp/opennlp-1568$ bash test.sh
test.sh
```
Then if I create a symlink `/tmp/test/opennlp.sh`, it works, and the correct
path is resolved, same as `realpath`.
```bash
kinow@ranma:/tmp/opennlp-1568$ cd ../
kinow@ranma:/tmp$ mkdir test
kinow@ranma:/tmp$ cd test
kinow@ranma:/tmp/test$ ln -s /tmp/opennlp-1568/test.sh opennlp.sh
kinow@ranma:/tmp/test$ bash opennlp.sh
/tmp/opennlp-1568/test.sh
```
But if the file is not in `/tmp/opennlp-1568`, but rather in some folder
like `/tmp/opennlp -> tests/`:
```bash
kinow@ranma:/tmp$ mkdir '/tmp/opennlp -> tests/'
kinow@ranma:/tmp$ cd '/tmp/opennlp -> tests/'
kinow@ranma:/tmp/opennlp -> tests$ cp ../opennlp-1568/test.sh .
kinow@ranma:/tmp/opennlp -> tests$ cd /tmp/test/
kinow@ranma:/tmp/test$ ln -s '/tmp/opennlp -> tests/test.sh' opennlp2.sh
kinow@ranma:/tmp/test$ bash opennlp2.sh
./tests/test.sh
```
I believe it's due to the regex in the `expr` (but again, late here, came
just to check the score of a soccer match and saw the GH notification. Here's
the `-eux` below.
```bash
kinow@ranma:/tmp/test$ bash -eux opennlp2.sh
+ PRG=opennlp2.sh
+ '[' -h opennlp2.sh ']'
++ ls -ld opennlp2.sh
+ ls='lrwxrwxrwx 1 kinow kinow 29 Jun 16 23:36 opennlp2.sh -> /tmp/opennlp
-> tests/test.sh'
++ expr 'lrwxrwxrwx 1 kinow kinow 29 Jun 16 23:36 opennlp2.sh ->
/tmp/opennlp -> tests/test.sh' : '.*-> \(.*\)$'
+ link=tests/test.sh
+ expr tests/test.sh : '/.*'
++ dirname opennlp2.sh
+ PRG=./tests/test.sh
+ '[' -h ./tests/test.sh ']'
+ echo ./tests/test.sh
./tests/test.sh
```
And `realpath`:
```bash
kinow@ranma:/tmp/test$ realpath opennlp.sh
/tmp/opennlp-1568/test.sh
kinow@ranma:/tmp/test$ realpath opennlp2.sh
/tmp/opennlp -> tests/test.sh
```
We do not need to use `realpath`. I was honestly just curious about this. We
don't even need to fix the regex if you/others agree users are not likely to
create paths with ` -> ` :+1: Other than that, changes looks OK to me (:soccer:
:wave: )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
