Revert "[lldb] refactor PlatformAndroid and make threadsafe" #153626
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit bcb48aa.
|
@llvm/pr-subscribers-lldb Author: Leandro Lupori (luporl) ChangesReverts llvm/llvm-project#145382 This broke a couple of buildbots. Patch is 58.58 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/153626.diff 7 Files Affected:
diff --git a/lldb/source/Plugins/Platform/Android/AdbClient.cpp b/lldb/source/Plugins/Platform/Android/AdbClient.cpp
index 0fbb48a2e16a0..a179260ca15f6 100644
--- a/lldb/source/Plugins/Platform/Android/AdbClient.cpp
+++ b/lldb/source/Plugins/Platform/Android/AdbClient.cpp
@@ -8,48 +8,61 @@
#include "AdbClient.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FileUtilities.h"
+
#include "lldb/Host/ConnectionFileDescriptor.h"
#include "lldb/Host/FileSystem.h"
-#include "lldb/Utility/Connection.h"
+#include "lldb/Host/PosixApi.h"
+#include "lldb/Utility/DataBuffer.h"
+#include "lldb/Utility/DataBufferHeap.h"
#include "lldb/Utility/DataEncoder.h"
#include "lldb/Utility/DataExtractor.h"
#include "lldb/Utility/FileSpec.h"
-#include "lldb/Utility/LLDBLog.h"
-#include "lldb/Utility/Log.h"
-#include "lldb/Utility/Status.h"
#include "lldb/Utility/StreamString.h"
#include "lldb/Utility/Timeout.h"
-#include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/Support/FileUtilities.h"
-#include <chrono>
#include <climits>
+
+#include <algorithm>
#include <cstdlib>
#include <fstream>
#include <sstream>
+// On Windows, transitive dependencies pull in <Windows.h>, which defines a
+// macro that clashes with a method name.
+#ifdef SendMessage
+#undef SendMessage
+#endif
+
using namespace lldb;
using namespace lldb_private;
using namespace lldb_private::platform_android;
using namespace std::chrono;
-using namespace llvm;
-static const char *kSocketNamespaceAbstract = "localabstract";
-static const char *kSocketNamespaceFileSystem = "localfilesystem";
-const seconds kReadTimeout(20);
+static const seconds kReadTimeout(20);
static const char *kOKAY = "OKAY";
static const char *kFAIL = "FAIL";
static const char *kDATA = "DATA";
static const char *kDONE = "DONE";
+
static const char *kSEND = "SEND";
static const char *kRECV = "RECV";
static const char *kSTAT = "STAT";
+
static const size_t kSyncPacketLen = 8;
+// Maximum size of a filesync DATA packet.
static const size_t kMaxPushData = 2 * 1024;
-static const uint32_t kDefaultMode = 0100770;
+// Default mode for pushed files.
+static const uint32_t kDefaultMode = 0100770; // S_IFREG | S_IRWXU | S_IRWXG
+
+static const char *kSocketNamespaceAbstract = "localabstract";
+static const char *kSocketNamespaceFileSystem = "localfilesystem";
static Status ReadAllBytes(Connection &conn, void *buffer, size_t size) {
+
Status error;
ConnectionStatus status;
char *read_buffer = static_cast<char *>(buffer);
@@ -72,215 +85,86 @@ static Status ReadAllBytes(Connection &conn, void *buffer, size_t size) {
error = Status::FromErrorStringWithFormat(
"Unable to read requested number of bytes. Connection status: %d.",
status);
-
return error;
}
-static Status ReadAdbMessage(Connection &conn, std::vector<char> &message) {
- message.clear();
-
- char buffer[5];
- buffer[4] = 0;
-
- auto error = ReadAllBytes(conn, buffer, 4);
- if (error.Fail())
- return error;
-
- unsigned int packet_len = 0;
- sscanf(buffer, "%x", &packet_len);
+Status AdbClient::CreateByDeviceID(const std::string &device_id,
+ AdbClient &adb) {
+ Status error;
+ std::string android_serial;
+ if (!device_id.empty())
+ android_serial = device_id;
+ else if (const char *env_serial = std::getenv("ANDROID_SERIAL"))
+ android_serial = env_serial;
- message.resize(packet_len, 0);
- error = ReadAllBytes(conn, &message[0], packet_len);
- if (error.Fail())
- message.clear();
+ if (android_serial.empty()) {
+ DeviceIDList connected_devices;
+ error = adb.GetDevices(connected_devices);
+ if (error.Fail())
+ return error;
+ if (connected_devices.size() != 1)
+ return Status::FromErrorStringWithFormat(
+ "Expected a single connected device, got instead %zu - try "
+ "setting 'ANDROID_SERIAL'",
+ connected_devices.size());
+ adb.SetDeviceID(connected_devices.front());
+ } else {
+ adb.SetDeviceID(android_serial);
+ }
return error;
}
-static Status GetResponseError(Connection &conn, const char *response_id) {
- if (strcmp(response_id, kFAIL) != 0)
- return Status::FromErrorStringWithFormat(
- "Got unexpected response id from adb: \"%s\"", response_id);
-
- std::vector<char> error_message;
- auto error = ReadAdbMessage(conn, error_message);
- if (!error.Success())
- return error;
-
- std::string error_str(&error_message[0], error_message.size());
- Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "ADB error: %s", error_str.c_str());
- return Status(error_str);
-}
-
-static Status ReadResponseStatus(Connection &conn) {
- char response_id[5];
-
- const size_t packet_len = 4;
- response_id[packet_len] = 0;
+AdbClient::AdbClient() = default;
- auto error = ReadAllBytes(conn, response_id, packet_len);
- if (error.Fail())
- return error;
+AdbClient::AdbClient(const std::string &device_id) : m_device_id(device_id) {}
- if (strncmp(response_id, kOKAY, packet_len) != 0)
- return GetResponseError(conn, response_id);
+AdbClient::~AdbClient() = default;
- return error;
+void AdbClient::SetDeviceID(const std::string &device_id) {
+ m_device_id = device_id;
}
-static Status SendAdbMessage(Connection &conn, llvm::StringRef packet) {
- Status error;
-
- char length_buffer[5];
- snprintf(length_buffer, sizeof(length_buffer), "%04x",
- static_cast<int>(packet.size()));
-
- ConnectionStatus status;
-
- conn.Write(length_buffer, 4, status, &error);
- if (error.Fail())
- return error;
-
- conn.Write(packet.str().c_str(), packet.size(), status, &error);
- return error;
-}
+const std::string &AdbClient::GetDeviceID() const { return m_device_id; }
-static Status ConnectToAdb(Connection &conn) {
+Status AdbClient::Connect() {
+ Status error;
+ m_conn = std::make_unique<ConnectionFileDescriptor>();
std::string port = "5037";
- if (const char *env_port = std::getenv("ANDROID_ADB_SERVER_PORT"))
+ if (const char *env_port = std::getenv("ANDROID_ADB_SERVER_PORT")) {
port = env_port;
+ }
std::string uri = "connect://127.0.0.1:" + port;
+ m_conn->Connect(uri.c_str(), &error);
- Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "Connecting to ADB server at %s", uri.c_str());
-
- Status error;
- conn.Connect(uri.c_str(), &error);
return error;
}
-static Status EnterSyncMode(Connection &conn) {
- auto error = SendAdbMessage(conn, "sync:");
+Status AdbClient::GetDevices(DeviceIDList &device_list) {
+ device_list.clear();
+
+ auto error = SendMessage("host:devices");
if (error.Fail())
return error;
- return ReadResponseStatus(conn);
-}
-
-static Status SelectTargetDevice(Connection &conn, llvm::StringRef device_id) {
- Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOG(log, "Selecting device: {0}", device_id);
-
- std::ostringstream msg;
- msg << "host:transport:" << device_id.str();
-
- auto error = SendAdbMessage(conn, msg.str());
+ error = ReadResponseStatus();
if (error.Fail())
return error;
- return ReadResponseStatus(conn);
-}
-
-Expected<std::string> AdbClient::ResolveDeviceID(StringRef device_id) {
- StringRef preferred_serial;
- if (!device_id.empty())
- preferred_serial = device_id;
- else if (const char *env_serial = std::getenv("ANDROID_SERIAL"))
- preferred_serial = env_serial;
-
- if (preferred_serial.empty()) {
- DeviceIDList connected_devices;
-
- auto GetDevices = [](DeviceIDList &device_list) -> Status {
- device_list.clear();
-
- // Create temporary ADB client for this operation only
- auto temp_conn = std::make_unique<ConnectionFileDescriptor>();
- auto error = ConnectToAdb(*temp_conn);
- if (error.Fail())
- return error;
-
- // NOTE: ADB closes the connection after host:devices response.
- // The connection is no longer valid
- error = SendAdbMessage(*temp_conn, "host:devices");
- if (error.Fail())
- return error;
+ std::vector<char> in_buffer;
+ error = ReadMessage(in_buffer);
- error = ReadResponseStatus(*temp_conn);
- if (error.Fail())
- return error;
+ llvm::StringRef response(&in_buffer[0], in_buffer.size());
+ llvm::SmallVector<llvm::StringRef, 4> devices;
+ response.split(devices, "\n", -1, false);
- std::vector<char> in_buffer;
- error = ReadAdbMessage(*temp_conn, in_buffer);
-
- StringRef response(&in_buffer[0], in_buffer.size());
- SmallVector<StringRef, 4> devices;
- response.split(devices, "\n", -1, false);
-
- for (const auto &device : devices)
- device_list.push_back(std::string(device.split('\t').first));
- return error;
- };
+ for (const auto &device : devices)
+ device_list.push_back(std::string(device.split('\t').first));
- Status error = GetDevices(connected_devices);
- if (error.Fail())
- return error.ToError();
-
- if (connected_devices.size() != 1)
- return createStringError(
- inconvertibleErrorCode(),
- "Expected a single connected device, got instead %zu - try "
- "setting 'ANDROID_SERIAL'",
- connected_devices.size());
-
- std::string resolved_device_id = std::move(connected_devices.front());
- Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "AdbClient::ResolveDeviceID Resolved device ID: %s",
- resolved_device_id.c_str());
- return resolved_device_id;
- }
-
- std::string resolved_device_id = preferred_serial.str();
- Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log, "AdbClient::ResolveDeviceID Resolved device ID: %s",
- resolved_device_id.c_str());
- return resolved_device_id;
-}
-
-AdbClient::AdbClient(llvm::StringRef device_id) : m_device_id(device_id) {
- Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log,
- "AdbClient::AdbClient(device_id='%s') - Creating AdbClient with "
- "device ID",
- device_id.str().c_str());
- m_conn = std::make_unique<ConnectionFileDescriptor>();
- Connect();
-}
-
-AdbClient::AdbClient() {
- Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(
- log,
- "AdbClient::AdbClient() - Creating AdbClient with default constructor");
- m_conn = std::make_unique<ConnectionFileDescriptor>();
- Connect();
-}
-
-AdbClient::~AdbClient() {
- Log *log = GetLog(LLDBLog::Platform);
- LLDB_LOGF(log,
- "AdbClient::~AdbClient() - Destroying AdbClient for device: %s",
- m_device_id.c_str());
-}
-
-llvm::StringRef AdbClient::GetDeviceID() const { return m_device_id; }
-
-Status AdbClient::Connect() {
- if (m_conn->IsConnected())
- return Status();
-
- return ConnectToAdb(*m_conn);
+ // Force disconnect since ADB closes connection after host:devices response
+ // is sent.
+ m_conn.reset();
+ return error;
}
Status AdbClient::SetPortForwarding(const uint16_t local_port,
@@ -293,7 +177,7 @@ Status AdbClient::SetPortForwarding(const uint16_t local_port,
if (error.Fail())
return error;
- return ReadResponseStatus(*m_conn);
+ return ReadResponseStatus();
}
Status
@@ -312,7 +196,7 @@ AdbClient::SetPortForwarding(const uint16_t local_port,
if (error.Fail())
return error;
- return ReadResponseStatus(*m_conn);
+ return ReadResponseStatus();
}
Status AdbClient::DeletePortForwarding(const uint16_t local_port) {
@@ -323,13 +207,56 @@ Status AdbClient::DeletePortForwarding(const uint16_t local_port) {
if (error.Fail())
return error;
- return ReadResponseStatus(*m_conn);
+ return ReadResponseStatus();
+}
+
+Status AdbClient::SendMessage(const std::string &packet, const bool reconnect) {
+ Status error;
+ if (!m_conn || reconnect) {
+ error = Connect();
+ if (error.Fail())
+ return error;
+ }
+
+ char length_buffer[5];
+ snprintf(length_buffer, sizeof(length_buffer), "%04x",
+ static_cast<int>(packet.size()));
+
+ ConnectionStatus status;
+
+ m_conn->Write(length_buffer, 4, status, &error);
+ if (error.Fail())
+ return error;
+
+ m_conn->Write(packet.c_str(), packet.size(), status, &error);
+ return error;
}
-Status AdbClient::SendDeviceMessage(llvm::StringRef packet) {
+Status AdbClient::SendDeviceMessage(const std::string &packet) {
std::ostringstream msg;
- msg << "host-serial:" << m_device_id << ":" << packet.str();
- return SendAdbMessage(*m_conn, msg.str());
+ msg << "host-serial:" << m_device_id << ":" << packet;
+ return SendMessage(msg.str());
+}
+
+Status AdbClient::ReadMessage(std::vector<char> &message) {
+ message.clear();
+
+ char buffer[5];
+ buffer[4] = 0;
+
+ auto error = ReadAllBytes(buffer, 4);
+ if (error.Fail())
+ return error;
+
+ unsigned int packet_len = 0;
+ sscanf(buffer, "%x", &packet_len);
+
+ message.resize(packet_len, 0);
+ error = ReadAllBytes(&message[0], packet_len);
+ if (error.Fail())
+ message.clear();
+
+ return error;
}
Status AdbClient::ReadMessageStream(std::vector<char> &message,
@@ -337,9 +264,6 @@ Status AdbClient::ReadMessageStream(std::vector<char> &message,
auto start = steady_clock::now();
message.clear();
- if (!m_conn)
- return Status::FromErrorString("No connection available");
-
Status error;
lldb::ConnectionStatus status = lldb::eConnectionStatusSuccess;
char buffer[1024];
@@ -358,22 +282,87 @@ Status AdbClient::ReadMessageStream(std::vector<char> &message,
return error;
}
+Status AdbClient::ReadResponseStatus() {
+ char response_id[5];
+
+ static const size_t packet_len = 4;
+ response_id[packet_len] = 0;
+
+ auto error = ReadAllBytes(response_id, packet_len);
+ if (error.Fail())
+ return error;
+
+ if (strncmp(response_id, kOKAY, packet_len) != 0)
+ return GetResponseError(response_id);
+
+ return error;
+}
+
+Status AdbClient::GetResponseError(const char *response_id) {
+ if (strcmp(response_id, kFAIL) != 0)
+ return Status::FromErrorStringWithFormat(
+ "Got unexpected response id from adb: \"%s\"", response_id);
+
+ std::vector<char> error_message;
+ auto error = ReadMessage(error_message);
+ if (!error.Success())
+ return error;
+ return Status(std::string(&error_message[0], error_message.size()));
+}
+
+Status AdbClient::SwitchDeviceTransport() {
+ std::ostringstream msg;
+ msg << "host:transport:" << m_device_id;
+
+ auto error = SendMessage(msg.str());
+ if (error.Fail())
+ return error;
+
+ return ReadResponseStatus();
+}
+
+Status AdbClient::StartSync() {
+ auto error = SwitchDeviceTransport();
+ if (error.Fail())
+ return Status::FromErrorStringWithFormat(
+ "Failed to switch to device transport: %s", error.AsCString());
+
+ error = Sync();
+ if (error.Fail())
+ return Status::FromErrorStringWithFormat("Sync failed: %s",
+ error.AsCString());
+
+ return error;
+}
+
+Status AdbClient::Sync() {
+ auto error = SendMessage("sync:", false);
+ if (error.Fail())
+ return error;
+
+ return ReadResponseStatus();
+}
+
+Status AdbClient::ReadAllBytes(void *buffer, size_t size) {
+ return ::ReadAllBytes(*m_conn, buffer, size);
+}
+
Status AdbClient::internalShell(const char *command, milliseconds timeout,
std::vector<char> &output_buf) {
output_buf.clear();
- auto error = SelectTargetDevice(*m_conn, m_device_id);
+ auto error = SwitchDeviceTransport();
if (error.Fail())
return Status::FromErrorStringWithFormat(
- "Failed to select target device: %s", error.AsCString());
+ "Failed to switch to device transport: %s", error.AsCString());
StreamString adb_command;
adb_command.Printf("shell:%s", command);
- error = SendAdbMessage(*m_conn, std::string(adb_command.GetString()));
+ error = SendMessage(std::string(adb_command.GetString()), false);
if (error.Fail())
return error;
- error = ReadResponseStatus(*m_conn);
+ error = ReadResponseStatus();
if (error.Fail())
return error;
@@ -428,8 +417,18 @@ Status AdbClient::ShellToFile(const char *command, milliseconds timeout,
return Status();
}
-Status AdbSyncService::PullFileImpl(const FileSpec &remote_file,
- const FileSpec &local_file) {
+std::unique_ptr<AdbClient::SyncService>
+AdbClient::GetSyncService(Status &error) {
+ std::unique_ptr<SyncService> sync_service;
+ error = StartSync();
+ if (error.Success())
+ sync_service.reset(new SyncService(std::move(m_conn)));
+
+ return sync_service;
+}
+
+Status AdbClient::SyncService::internalPullFile(const FileSpec &remote_file,
+ const FileSpec &local_file) {
const auto local_file_path = local_file.GetPath();
llvm::FileRemover local_file_remover(local_file_path);
@@ -463,8 +462,8 @@ Status AdbSyncService::PullFileImpl(const FileSpec &remote_file,
return error;
}
-Status AdbSyncService::PushFileImpl(const FileSpec &local_file,
- const FileSpec &remote_file) {
+Status AdbClient::SyncService::internalPushFile(const FileSpec &local_file,
+ const FileSpec &remote_file) {
const auto local_file_path(local_file.GetPath());
std::ifstream src(local_file_path.c_str(), std::ios::in | std::ios::binary);
if (!src.is_open())
@@ -488,9 +487,7 @@ Status AdbSyncService::PushFileImpl(const FileSpec &local_file,
error.AsCString());
}
error = SendSyncRequest(
- kDONE,
- llvm::sys::toTimeT(
- FileSystem::Instance().GetModificationTime(local_file)),
+ kDONE, llvm::sys::toTimeT(FileSystem::Instance().GetModificationTime(local_file)),
nullptr);
if (error.Fail())
return error;
@@ -503,7 +500,7 @@ Status AdbSyncService::PushFileImpl(const FileSpec &local_file,
error.AsCString());
if (response_id == kFAIL) {
std::string error_message(data_len, 0);
- error = ReadAllBytes(*m_conn, &error_message[0], data_len);
+ error = ReadAllBytes(&error_message[0], data_len);
if (error.Fail())
return Status::FromErrorStringWithFormat(
"Failed to read DONE error message: %s", error.AsCString());
@@ -521,8 +518,9 @@ Status AdbSyncService::PushFileImpl(const FileSpec &local_file,
return error;
}
-Status AdbSyncService::StatImpl(const FileSpec &remote_file, uint32_t &mode,
- uint32_t &size, uint32_t &mtime) {
+Status AdbClient::SyncService::internalStat(const FileSpec &remote_file,
+ uint32_t &mode, uint32_t &size,
+ uint32_t &mtime) {
const std::string remote_file_path(remote_file.GetPath(false));
auto error = SendSyncRequest(kSTAT, remote_file_path.length(),
remote_file_path.c_str());
@@ -534,7 +532,7 @@ Status AdbSyncService::StatImpl(const FileSpec &remote_file, uint32_t &mode,
static const size_t response_len = stat_len + (sizeof(uint32_t) * 3);
std::vector<char> buffer(response_len);
- error = ReadAllBytes(*m_conn, &buffer[0], buffer.size());
+ error = ReadAllBytes(&buffer[0], buffer.size());
if (error.Fail())
return Status::FromErrorStringWithFormat("Failed to read response: %s",
error.AsCString());
@@ -557,57 +555,51 @@ Status AdbSyncService::StatImpl(const FileSpec &remote_file, uint32_t &mode,
return Status();
}
-Status AdbSyncService::PullFile(const FileSpec &remote_file,
- const FileSpec &local_file) {
- return ExecuteCommand([this, &remote_file, &local_file]() {
- return PullFileImpl(remote_file, local_file);
+Status AdbClient::SyncService::PullFile(const FileSpec &remote_file,
+ const FileSpec &local_file) {
+ return executeCommand([this, &remote_file, &local_file]() {
+ return internalPullFile(remote_file, local_file);
});
}
-Status AdbSyncService::PushFile(const FileSpec &local_file,
- const FileSpec &remote_file) {
- return ExecuteCommand([this, &local_file, &remote_file]() {
- return PushFileImpl(local_file, remote_file);
+Status AdbClient::SyncService::PushFile(const FileSpec &local_file,
+ ...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp,h -- lldb/source/Plugins/Platform/Android/AdbClient.cpp lldb/source/Plugins/Platform/Android/AdbClient.h lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp lldb/source/Plugins/Platform/Android/PlatformAndroid.h lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp lldb/unittests/Platform/Android/AdbClientTest.cpp lldb/unittests/Platform/Android/PlatformAndroidTest.cppView the diff from clang-format here.diff --git a/lldb/source/Plugins/Platform/Android/AdbClient.cpp b/lldb/source/Plugins/Platform/Android/AdbClient.cpp
index a179260ca..895fda95e 100644
--- a/lldb/source/Plugins/Platform/Android/AdbClient.cpp
+++ b/lldb/source/Plugins/Platform/Android/AdbClient.cpp
@@ -487,7 +487,9 @@ Status AdbClient::SyncService::internalPushFile(const FileSpec &local_file,
error.AsCString());
}
error = SendSyncRequest(
- kDONE, llvm::sys::toTimeT(FileSystem::Instance().GetModificationTime(local_file)),
+ kDONE,
+ llvm::sys::toTimeT(
+ FileSystem::Instance().GetModificationTime(local_file)),
nullptr);
if (error.Fail())
return error;
diff --git a/lldb/unittests/Platform/Android/AdbClientTest.cpp b/lldb/unittests/Platform/Android/AdbClientTest.cpp
index 0808b96f6..be87867b8 100644
--- a/lldb/unittests/Platform/Android/AdbClientTest.cpp
+++ b/lldb/unittests/Platform/Android/AdbClientTest.cpp
@@ -6,8 +6,8 @@
//
//===----------------------------------------------------------------------===//
-#include "gtest/gtest.h"
#include "Plugins/Platform/Android/AdbClient.h"
+#include "gtest/gtest.h"
#include <cstdlib>
static void set_env(const char *var, const char *value) {
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #145382
This broke a couple of buildbots.