Page MenuHomePhabricator

Fix execution of platform shell commands on android
ClosedPublic

Authored by tberghammer on Mar 2 2015, 9:50 AM.

Details

Summary

Fix execution of platform shell commands on android

  • Add missing functionality to the process launcher
  • Fixup PATH environment variable to workaround an OS bug
  • Add default shell path to the host info structure

Diff Detail

Repository
rL LLVM

Event Timeline

tberghammer updated this revision to Diff 21013.Mar 2 2015, 9:50 AM
tberghammer retitled this revision from to Fix execution of platform shell commands on android.
tberghammer updated this object.
tberghammer edited the test plan for this revision. (Show Details)
tberghammer added reviewers: labath, vharron.
tberghammer set the repository for this revision to rL LLVM.
tberghammer added a subscriber: Unknown Object (MLST).
labath edited edge metadata.Mar 2 2015, 10:57 AM

Looks good overall, but I have a couple of comments...

include/lldb/Host/android/HostInfoAndroid.h
26 ↗(On Diff #21013)

How is the constructor removal related to the rest of the patch?

source/Host/android/ProcessLauncherAndroid.cpp
42 ↗(On Diff #21013)

Storage specifiers (static) are usually placed before the type (including the "const" keyword).

tberghammer added inline comments.Mar 2 2015, 11:17 AM
include/lldb/Host/android/HostInfoAndroid.h
26 ↗(On Diff #21013)

It is absolutely unrelated just I noticed that construction is already disabled in the bottom most base class (HostInfoBase) and non of the derived have explicit code for doing it, so made it consistent.
Can revert this part if you want.

labath requested changes to this revision.Mar 2 2015, 12:59 PM
labath edited edge metadata.
labath added inline comments.
include/lldb/Host/android/HostInfoAndroid.h
26 ↗(On Diff #21013)

Sounds like I good thing to do. Keep them removed.

source/Host/android/ProcessLauncherAndroid.cpp
42 ↗(On Diff #21013)

This should be read "please put static before const". :)

This revision now requires changes to proceed.Mar 2 2015, 12:59 PM
tberghammer updated this revision to Diff 21093.Mar 3 2015, 3:19 AM
tberghammer edited edge metadata.
tberghammer removed rL LLVM as the repository for this revision.
labath accepted this revision.Mar 3 2015, 3:42 AM
labath edited edge metadata.

lgtm

This revision is now accepted and ready to land.Mar 3 2015, 3:42 AM
This revision was automatically updated to reflect the committed changes.