Conversation
| @@ -0,0 +1,209 @@ | |||
| /* | |||
| Copyright 2023 Google Inc. | |||
There was a problem hiding this comment.
lol, this is supposed to be the same file as command_utils.cc, I guess most of the content are changed
There was a problem hiding this comment.
a more general question: why do you put the platform dependent func in this file instead of the command_utils_.cpp? (for example, why not put MakeWindowsCommand() in src/dive/os/command_utils_win32.cpp)
There was a problem hiding this comment.
MakeWindowsCommand is not platform dependent, it's just a string manipulation function which does string escape according to Windows rules.
MakeUnixCommand is additionally required to escape adb shell command correctly even on Windows.
There was a problem hiding this comment.
e.g. Python expose both Windows and Linux path function, and you can run the following on Linux environment
import pathlib
print(str(pathlib.PureWindowsPath("C:/Program Files")))There was a problem hiding this comment.
it seems that "MakeWindowsCommand" is only used by command_utils_win32.cpp? do you see possible usage for other platforms as well? I understand that MakeWindowsCommand is platform-independent logic (just string manipulation), If we moved MakeWindowsCommand to command_utils_win32.cpp, It would clearly signal that this logic is intended for the Windows platform (or targeting Windows).
There was a problem hiding this comment.
The name MakeWindowsCommand is clear enough that it is for Windows.
| context.mgr.GetDevice() | ||
| ->Adb() | ||
| .Run(absl::StrFormat("shell rm -rf %s", on_device_capture_directory)) | ||
| .Shell(absl::StrFormat("rm -rf %s", on_device_capture_directory)) |
There was a problem hiding this comment.
would this fail if there is a space in on_device_capture_directory?
i see you have 2 overload Shell(), and this will trigger the
absl::Status Shell(std::string_view command) const
{
return RunCommand("adb", {"-s", m_serial, "shell", std::string(command)}).status();
}
which doesn't call "MakeUnixCommand?" on the command string?
There was a problem hiding this comment.
Yes, it will fail.
It should be .Shell("rm", {"-rf", on_device_capture_directory}), however most of these are find-replace regex edit.
There's still a problem if on_device_capture_directory starts with - and rm think it is a parameter rather than a flag. However, I don't aim to solve every problem.
I'd like to have adb.Run(Adb::Remove{.recursive = true, .force = true, .path= on_device_capture_directory}), but that's a lot of code to write.
2 participants
No description provided.