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

made arch validation more robust #417

Merged
merged 3 commits into from Jul 24, 2017

Conversation

Projects
None yet
2 participants
@stefaang
Contributor

stefaang commented Jul 24, 2017

The installer fails to parse i686 as a platform on the Google Pixel x686 emulator.
This is caused by some weird behavior of the ValidArch filter on this specific platform.
This fix is less 'spacy' and allows proper platform parsing.

An example of how echo piped into sed works on this platform:

adb shell
# var=`echo " aa bb cc " | sed "s/ cc //"`
# echo $var
aa bb aa bb cc
# var=`echo " aa bb cc " | sed "s/ aa //"`
# echo $var
aa bb cc
@hugbug

This comment has been minimized.

Show comment
Hide comment
@hugbug

hugbug Jul 24, 2017

Member

This unfortunately adds a dependency from "grep" which is a bad thing. There are so many busybox variants with differently limited functionality.

Maybe a loop like this instead?
https://github.com/nzbget/nzbget/blob/develop/linux/installer.sh#L243

Member

hugbug commented Jul 24, 2017

This unfortunately adds a dependency from "grep" which is a bad thing. There are so many busybox variants with differently limited functionality.

Maybe a loop like this instead?
https://github.com/nzbget/nzbget/blob/develop/linux/installer.sh#L243

@hugbug

This comment has been minimized.

Show comment
Hide comment
@hugbug

hugbug Jul 24, 2017

Member

Have you tested this? The return statements look wrong.

Member

hugbug commented Jul 24, 2017

Have you tested this? The return statements look wrong.

@stefaang

This comment has been minimized.

Show comment
Hide comment
@stefaang

stefaang Jul 24, 2017

Contributor

Yeah. The existing implementation filters for the arch, replaces it by an empty string and checks if ALLARCHS matches the result. If it matches, the arch was actually NOT in ALLARCHS.

The new implementation is a bit more logical, the test returning 0 means it is in the ALLARCHS variable.

Contributor

stefaang commented Jul 24, 2017

Yeah. The existing implementation filters for the arch, replaces it by an empty string and checks if ALLARCHS matches the result. If it matches, the arch was actually NOT in ALLARCHS.

The new implementation is a bit more logical, the test returning 0 means it is in the ALLARCHS variable.

@hugbug

This comment has been minimized.

Show comment
Hide comment
@hugbug

hugbug Jul 24, 2017

Member

Ah, thanks.

The last thing. To make the new code more similar to other one please use upper case for variables and use quotes in test-comparisons.

for TARG in $ALLARCHS; do
   if test "$TARG" = "$1"; then

I use TARG here instead of ARCH because ARCH is used in other places and I'm not sure about variables lifetime (if it would be local to the function or would overwrite ARCH used elsewhere).

Thanks.

Member

hugbug commented Jul 24, 2017

Ah, thanks.

The last thing. To make the new code more similar to other one please use upper case for variables and use quotes in test-comparisons.

for TARG in $ALLARCHS; do
   if test "$TARG" = "$1"; then

I use TARG here instead of ARCH because ARCH is used in other places and I'm not sure about variables lifetime (if it would be local to the function or would overwrite ARCH used elsewhere).

Thanks.

@hugbug hugbug merged commit 01d4ebb into nzbget:develop Jul 24, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@hugbug

This comment has been minimized.

Show comment
Hide comment
@hugbug

hugbug Jul 24, 2017

Member

Great. Thank you!

Member

hugbug commented Jul 24, 2017

Great. Thank you!

@hugbug hugbug added the improvement label Jul 24, 2017

@hugbug hugbug added this to the v20 milestone Jul 24, 2017

@stefaang

This comment has been minimized.

Show comment
Hide comment
@stefaang

stefaang Sep 14, 2017

Contributor

I found the root cause: android M ships with busybox and toybox.

root@box:/ # ls -l /system/bin/sed
lrwxr-xr-x root     shell             2017-09-07 20:44 sed -> toybox
root@box:/ # echo " aa bb cc " | sed "s/ cc //"
 aa bb aa bb cc
root@box:/ # echo " aa bb cc " | busybox sed "s/ cc //"
 aa bb

This is a bug in toybox sed ...

Contributor

stefaang commented Sep 14, 2017

I found the root cause: android M ships with busybox and toybox.

root@box:/ # ls -l /system/bin/sed
lrwxr-xr-x root     shell             2017-09-07 20:44 sed -> toybox
root@box:/ # echo " aa bb cc " | sed "s/ cc //"
 aa bb aa bb cc
root@box:/ # echo " aa bb cc " | busybox sed "s/ cc //"
 aa bb

This is a bug in toybox sed ...

@stefaang

This comment has been minimized.

Show comment
Hide comment
@stefaang

stefaang Sep 14, 2017

Contributor

And the same for the weird tar behaviour I reported in the android installer: a bug in toybox tar

Contributor

stefaang commented Sep 14, 2017

And the same for the weird tar behaviour I reported in the android installer: a bug in toybox tar

@hugbug

This comment has been minimized.

Show comment
Hide comment
@hugbug

hugbug Sep 14, 2017

Member

I guess the installation from Android installer app would work as the included busybox would be used.

Member

hugbug commented Sep 14, 2017

I guess the installation from Android installer app would work as the included busybox would be used.

@stefaang

This comment has been minimized.

Show comment
Hide comment
@stefaang

stefaang Sep 14, 2017

Contributor

Sure but the x86 build is broken :) that's why I tried to use the builtin tools.
Using the builtin tools makes the android installer compatible with x86, x64 and the ARM flavours.

Contributor

stefaang commented Sep 14, 2017

Sure but the x86 build is broken :) that's why I tried to use the builtin tools.
Using the builtin tools makes the android installer compatible with x86, x64 and the ARM flavours.

@hugbug

This comment has been minimized.

Show comment
Hide comment
@hugbug

hugbug Sep 14, 2017

Member

Sure but the x86 build is broken :) that's why I tried to use the builtin tools.

Ah, right. Now I remember.

Member

hugbug commented Sep 14, 2017

Sure but the x86 build is broken :) that's why I tried to use the builtin tools.

Ah, right. Now I remember.

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