Skip to content
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

Discover ADB executable in AdbBinaryInteractor #82

Merged
merged 1 commit into from
May 14, 2022

Conversation

snpefk
Copy link
Contributor

@snpefk snpefk commented May 12, 2022

Sometimes adb executable may be installed as a standalone binary, not as part of ANDROID SDK. For example, in debian/ubuntu based containers with android-tools-adb package. We can discover it with where command (read on SO that which should work on Windows, but I'm unable to test it).

I'm not sure if this a suitable addition since AdbBinaryInteractor already has parameter for specifying path to adb. As a compromise function discoverAdbBinary can be refactor into DiscoverAdbBinary interactor.

@snpefk snpefk requested a review from Malinskiy as a code owner May 12, 2022 14:17
@snpefk
Copy link
Contributor Author

snpefk commented May 12, 2022

And if you are okay with this I can change

val adbBinaryName = when {
    os.contains("win") -> {
        "adb.exe"
    }
    else -> "adb"
}
... 
return when (process.exitValue()) {
    0 -> true
    else -> false
}

to more brief alternatives

val adbBinaryName = if ("win" in os) "adb.exe" else "adb"
... 
return process.exitValue() == 0

But it's not very important and it's your project :)

@snpefk snpefk changed the title Add method for discover ADB executable Discover ADB executable in AdbBinaryInteractor May 12, 2022
@Malinskiy
Copy link
Owner

that's a nice new heuristic @snpefk. doesn't guarantee that in those single adb binary cases it's going to be found, but it's better than just failing for sure.
will merge in the next couple of days :)

@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #82 (d23dfe1) into master (0172564) will decrease coverage by 0.29%.
The diff coverage is 6.66%.

@@             Coverage Diff              @@
##             master      #82      +/-   ##
============================================
- Coverage     81.41%   81.12%   -0.30%     
  Complexity      715      715              
============================================
  Files           138      138              
  Lines          3046     3057      +11     
  Branches        501      507       +6     
============================================
  Hits           2480     2480              
- Misses          305      314       +9     
- Partials        261      263       +2     
Flag Coverage Δ
integration 63.52% <0.00%> (-0.27%) ⬇️
unit 72.16% <6.66%> (-0.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...m/malinskiy/adam/interactor/AdbBinaryInteractor.kt 36.36% <6.66%> (-18.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0172564...d23dfe1. Read the comment docs.

@Malinskiy Malinskiy merged commit 86bea1f into Malinskiy:master May 14, 2022
@snpefk
Copy link
Contributor Author

snpefk commented May 14, 2022

doesn't guarantee that in those single adb binary cases it's going to be found, but it's better than just failing for sure.

Hmm, which searching for an executable in the directories listed in the environment variable PATH. This should cover all cases except manually added binary to container. Am I missing something?

@snpefk snpefk deleted the patch-1 branch May 14, 2022 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants