Skip to content
This repository has been archived by the owner on Jan 5, 2022. It is now read-only.

Compatibility with MobaXterm/Windows #24

Merged
merged 3 commits into from
Jan 24, 2017
Merged

Conversation

JimboJoe
Copy link
Contributor

PR corresponding to issue #23

@JimboJoe JimboJoe mentioned this pull request Jan 21, 2017
@@ -153,14 +153,15 @@ fi
# Check whether a device is connected at all and, if configured, the serial matches
# No device connected:
ADBOPTS=""
if [ -z "$(adb devices|egrep "^[0-9A-Za-z.:]+\s+device$"|awk '{print $1}')" ]; then

if [ -z "$(adb devices|grep -E "^[0-9A-Za-z]+[^ ]+device"|awk '{print $1}')" ]; then
Copy link
Owner

@IzzySoft IzzySoft Jan 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a completely different logic: 1) you removed the "end of string" marker, and 2) most likely broke the spacing. An expected string would look like:

0123456789ABCDEF	device
012.34:56789ABCD	device

which matches the regex ^[0-9A-Za-z.:]+\s+device$: {string containing only alpha-numerics, dots and colons} {followed by at least one white space} {followed by the literal term "device"} {end-of-string}. Your [^ ], instead of "white spaces" would match "everything but a blank". It probably works for you right now as there's a "tab" in that place, but it might break for others. You also removed .: from the device name, which is definitely needed there (some devices have it; your replacement currently still works, but only if the two are separated by a tab).

What broke there with the original regex? We need to fix that in some other way. The output of adb devices you get would enable me to help out :)

echo "No device found. Make sure you have connected your device with"
echo "USB debugging enabled, and try again."
echo
exit 2
fi

serials=($(adb devices|egrep "^[0-9A-Za-z.:]+\s+device$"|awk '{print $1}'))
serials=($(adb devices|grep -E "^[0-9A-Za-z]+[^ ]+device"|awk '{print $1}'))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for line 157 here.

@@ -221,7 +222,7 @@ fi
# What Android version shall we assume (for specific features)? Do not evaluate if user has configured an override.
[[ -z "${DEVICE_SDKVER}" ]] && DEVICE_SDKVER=$(adb shell "getprop ro.build.version.sdk")
DEVICE_SDKVER=${DEVICE_SDKVER//[$'\t\r\n']}

#DEVICE_SDKVER=23
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was for testing I guess, and can be removed?

@JimboJoe
Copy link
Contributor Author

You're damn right, I broke the logic on the spacing part... weird it was working!
I put back the ".:" part, I took it out while debugging and forgot to put it back.
I've found an elegant proposition that should cover the space and tab spacing between the device name and the "device" part.
I'm still puzzled by the "$" that prevents from matching... That should have something to do with EOL differences...
I attached my adb devices output if you want to have a look : adb_devices.txt

@IzzySoft
Copy link
Owner

Thanks Jim. I've checked my regex against your adb devices output, and it works fine as expected.

Thanks for the update, too. As it still lacks the $ the issue I described would remain (and I wonder why you should have issues in that place, while not in the other at line 104). You could try adding it back as \s*$ though to see if that makes a difference. Note: If you just try that grep in the command line (with the \s*$) it will look like it matched an empty line – as the ^M (CR) blanks out the result. Redirecting it to a file however shows the full line. I've just tested the full condition here, it seems to work. If \s doesn't work, there's also [[:space:]].

And while [[:blank:]] should almost be the same as \s, I'm not 100% sure about that. But I don't see any issue with that in this place either, as I expect nothing but tab or spaces there.

TL;DR: Could you please check if the $ can be placed back – if necessary, with a leading \s* or [[:space:]]*?

@JimboJoe
Copy link
Contributor Author

You got it right!
It wouldn't do with "\s*", but worked with "[[:space:]]*" before the $.

@IzzySoft
Copy link
Owner

Thanks a lot! Looks very good now, so I'm merging this. May I ask you to add your configuration to #7 then?

@IzzySoft IzzySoft merged commit 7e18ceb into IzzySoft:master Jan 24, 2017
@JimboJoe
Copy link
Contributor Author

Done, thanks again!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants