Closed chirag-jn closed 9 months ago
chirag-jn
commented
9 months ago Commit message is pretty hard to follow. Also
[ -n ]is not a syntax error, otherwise sh would have complained.
Updated the commit message to make it more readable. I added the logs in the PR to elaborate the issue faced.
Further, In the case of [ -n ], as visible in the output of sh -x, the output isn't coming as expected. Also, using if-else makes the code more readable and understandable.
lumag
commented
9 months ago It still talks about the syntax error. [ condition ] && do_something is a perfectly fine from the syntax point of view.
I think just adding true or exit 0 to the end of the setup script will fix your issue.
chirag-jn
commented
9 months ago It still talks about the syntax error.
[ condition ] && do_somethingis a perfectly fine from the syntax point of view. I think just addingtrueorexit 0to the end of the setup script will fix your issue.
I’ll try this and get back!
lumag
commented
9 months ago Also, I'm not sure how does that fix the issue for the serial number being not available in the /proc/cmdline
chirag-jn
commented
9 months ago Also, I'm not sure how does that fix the issue for the serial number being not available in the
/proc/cmdline
if serial number is not available in /proc/cmdline, then default value will be picked up. Default value is defined in /usr/bin/android-gadget-setup
lumag
commented
9 months ago Ok, so we should be back to #547 to get the proper serial number even if we fix this script.
chirag-jn
commented
9 months ago Ok, so we should be back to #547 to get the proper serial number even if we fix this script.
Yeah we are working on #547 to fix reading serial number. Meanwhile, we can proceed with this PR.
chirag-jn
commented
9 months ago It still talks about the syntax error.
[ condition ] && do_somethingis a perfectly fine from the syntax point of view. I think just addingtrueorexit 0to the end of the setup script will fix your issue.
Followed this. Can you check?
lumag
commented
9 months ago It still talks about the syntax error.
[ condition ] && do_somethingis a perfectly fine from the syntax point of view. I think just addingtrueorexit 0to the end of the setup script will fix your issue.Followed this. Can you check?
There is still a talk about the syntax error in the commit message. Also could you please check whether a single exit 0 helps? Because I think the issue is that the script returns 1 to systemd, nothing else.
chirag-jn
commented
9 months ago exit 0 did not help. It can be tested locally too.
$ cat file1
set -e
val="someval"
if [ -r ./file2 ] ; then
. ./file2
fi
echo $val$ cat file2
a=""
[ -n "$a" ] && val="$a"
exit 0$ sh -x ./file1
+ set -e
+ val=someval
+ '[' -r ./file2 ']'
+ . ./file2
++ a=
++ '[' -n '' ']'
++ exit 0$ cat file2
a=""
[ -n "$a" ] && val="$a" || true$ sh -x ./file1
+ set -e
+ val=someval
+ '[' -r ./file2 ']'
+ . ./file2
++ a=
++ '[' -n '' ']'
++ true
+ echo someval
somevalIt still talks about the syntax error.
[ condition ] && do_somethingis a perfectly fine from the syntax point of view. I think just addingtrueorexit 0to the end of the setup script will fix your issue.Followed this. Can you check?
There is still a talk about the syntax error in the commit message. Also could you please check whether a single
exit 0helps? Because I think the issue is that the script returns 1 to systemd, nothing else.
My mistake. Updated the commit message.
chirag-jn
commented
9 months ago $ cat file2
a="newval"
[ -n "$a" ] && val="$a" || true$ sh -x ./file1
+ set -e
+ val=someval
+ '[' -r ./file2 ']'
+ . ./file2
++ a=newval
++ '[' -n newval ']'
++ val=newval
+ echo newval
newval
[ -n ]used is an incomplete conditional test. In bash and similar shells,[ -n ]without a string following-nis considered incomplete.
This is still not applicable to the source code in question. There is a string argument after -n, double quotes ensure that enven if the environment variable is empty, there will be an empty string argument:
[ -n "$androidserial" ] Ack, I checked, this works:
lumag@eriador:/tmp$ cat test.sh
#!/bin/sh
set -e
if [ -r test2.sh ] ; then
. ./test2.sh
fi
echo continued
lumag@eriador:/tmp$ cat test2.sh
[ -n "$EMPTY" ] && echo not-empty
true
lumag@eriador:/tmp$ sh test.sh
continued
lumag@eriador:/tmp$ EMPTY=not sh test.sh
not-empty
continuedAck, I checked, this works:
lumag@eriador:/tmp$ cat test.sh #!/bin/sh set -e if [ -r test2.sh ] ; then . ./test2.sh fi echo continued lumag@eriador:/tmp$ cat test2.sh [ -n "$EMPTY" ] && echo not-empty true lumag@eriador:/tmp$ sh test.sh continued lumag@eriador:/tmp$ EMPTY=not sh test.sh not-empty continued
yeah exit 0 as the last command doesn't work but true as the last command works. Do I still have to make a change or the current commit works fine?
lumag
commented
9 months ago Yes, please.
lumag
commented
9 months ago And fix the commit message too.
chirag-jn
commented
9 months ago And fix the commit message too.
fixed the code as well as the commit message. Thanks @lumag !
chirag-jn
commented
9 months ago @lumag so is it complete or anything else needed?
Change the conditional argument to check if androidserial is non-empty.
Initially we observed the following logs on boot-up as well as on running
systemctl restart android-tools-adbdBoot-up logs:
Logs of
systemctl restart android-tools-adbdTo analyze any issue in the script, we ran
$ sh -x /usr/bin/android-gadget-setup, and got the followingScript abruptly stopped at
'[' -n ], which is a syntax error. Script is running underset -eso any non-zero exit code in a command will abort the script. To fix the issue, we made the patch as provided in the commit.After the patch, we saw the following output of
$ sh -x /usr/bin/android-gadget-setupWe can see that the script progressed past the stage of
[ -n ], confirming that the patch works. Further,android-tools-adbdstarted working and we could see the device visible inadb deviceson host machine.