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

fix(dashboard): 🐛 Fix SteamVR launch fail for dangling ADB process #2757

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 20 additions & 34 deletions alvr/adb/src/commands.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
// https://android.googlesource.com/platform/packages/modules/adb/+/refs/heads/main/docs/user/adb.1.md

use crate::parse::{self, Device, ForwardedPorts};
use alvr_filesystem::Layout;
use alvr_filesystem as afs;
use anyhow::{anyhow, Context, Result};
use std::{
collections::HashSet,
io::{Cursor, Read},
path::PathBuf,
process::Command,
time::Duration,
};
@@ -137,7 +136,7 @@ pub fn is_activity_resumed(
// ADB Installation

pub fn require_adb(
layout: &Layout,
layout: &afs::Layout,
progress_callback: impl Fn(usize, Option<usize>),
) -> Result<String> {
match get_adb_path(layout) {
@@ -150,11 +149,12 @@ pub fn require_adb(
}
}

fn install_adb(layout: &Layout, progress_callback: impl Fn(usize, Option<usize>)) -> Result<()> {
let buffer = download_adb(progress_callback)?;
let mut reader = Cursor::new(buffer);
let path = get_installation_path(layout);
ZipArchive::new(&mut reader)?.extract(path)?;
fn install_adb(
layout: &afs::Layout,
progress_callback: impl Fn(usize, Option<usize>),
) -> Result<()> {
let mut reader = Cursor::new(download_adb(progress_callback)?);
ZipArchive::new(&mut reader)?.extract(layout.executables_dir.clone())?;

Ok(())
}
@@ -261,34 +261,20 @@ pub fn list_installed_packages(adb_path: &str, device_serial: &str) -> Result<Ha
// Paths

/// Returns the path of a local (i.e. installed by ALVR) or OS version of `adb` if found, `None` otherwise.
fn get_adb_path(layout: &Layout) -> Option<String> {
get_os_adb_path().or(get_local_adb_path(layout))
}

fn get_os_adb_path() -> Option<String> {
let name = get_executable_name().to_owned();

get_command(&name, &[]).output().is_ok().then_some(name)
}

fn get_local_adb_path(layout: &Layout) -> Option<String> {
let path = get_platform_tools_path(layout).join(get_executable_name());

path.try_exists()
.is_ok_and(|e| e)
.then(|| path.to_string_lossy().to_string())
}

fn get_installation_path(layout: &Layout) -> PathBuf {
layout.executables_dir.to_owned()
}
pub fn get_adb_path(layout: &afs::Layout) -> Option<String> {
let exe_name = afs::exec_fname("adb").to_owned();
let adb_path = get_command(&exe_name, &[])
.output()
.is_ok()
.then_some(exe_name);

fn get_platform_tools_path(layout: &Layout) -> PathBuf {
get_installation_path(layout).join("platform-tools")
}
adb_path.or_else(|| {
let path = layout.local_adb_exe();

fn get_executable_name() -> String {
alvr_filesystem::exec_fname("adb")
path.try_exists()
.unwrap_or(false)
.then(|| path.to_string_lossy().to_string())
})
}

//////////////////
7 changes: 7 additions & 0 deletions alvr/dashboard/src/steamvr_launcher/mod.rs
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@ mod linux_steamvr;
#[cfg(windows)]
mod windows_steamvr;

use alvr_adb::commands as adb;
use alvr_common::{
anyhow::{Context, Result},
debug,
@@ -112,6 +113,12 @@ pub struct Launcher {

impl Launcher {
pub fn launch_steamvr(&self) {
// The ADB server might be left running because of a unclean termination of SteamVR
// Note that this will also kill a system wide ADB server not started by ALVR
if let Some(path) = adb::get_adb_path(&crate::get_filesystem_layout()) {
adb::kill_server(&path).ok();
}
Comment on lines +118 to +120
Copy link
Member Author

Choose a reason for hiding this comment

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

This is alright but it would unnecessarily kill a global adb server instance. Better would be to get only the local adb path and try to kill that. But better yet would be to get the current running adb processes and kill any that is child of SteamVR (vrserver). This last solution would also be resistant to edge cases where the users installs a global adb server while running SteamVR

Copy link
Collaborator

Choose a reason for hiding this comment

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

note though, i think you can't run two adb instances as well, so you kinda have to replace one instance with another anyway


#[cfg(target_os = "linux")]
linux_steamvr::linux_hardware_checks();

6 changes: 6 additions & 0 deletions alvr/filesystem/src/lib.rs
Original file line number Diff line number Diff line change
@@ -201,6 +201,12 @@ impl Layout {
self.executables_dir.join(dashboard_fname())
}

pub fn local_adb_exe(&self) -> PathBuf {
self.executables_dir
.join("platform-tools")
.join(exec_fname("adb"))
}

pub fn resources_dir(&self) -> PathBuf {
self.openvr_driver_root_dir.join("resources")
}