From 1ab68b225a6fc78e36dcf47b83ec0b607c7e6b44 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Mon, 22 Mar 2021 22:18:10 -0700 Subject: [PATCH 01/18] new attempt at rust persistent worker: just the build setting for now going to try this in C++ and have it all work nicely. This commit just sets up a build setting, so that users can customize whether to use the worker by just setting the flag on the command line or in a local bazelrc file. This allows easily switching between worker and non-worker. Use as `bazel build --@rules_rust//rust:use-worker`. Obviously this will fail right now. --- rust/BUILD.bazel | 6 + rust/private/rust.bzl | 7 ++ rust/private/rustc.bzl | 19 ++- util/process_wrapper/BUILD.bazel | 8 ++ util/worker/BUILD | 26 ++++ util/worker/worker.cc | 197 +++++++++++++++++++++++++++++++ 6 files changed, 262 insertions(+), 1 deletion(-) create mode 100644 util/worker/BUILD create mode 100644 util/worker/worker.cc diff --git a/rust/BUILD.bazel b/rust/BUILD.bazel index d5c8d38e30..ffd27c1819 100644 --- a/rust/BUILD.bazel +++ b/rust/BUILD.bazel @@ -1,4 +1,5 @@ load("@bazel_skylib//:bzl_library.bzl", "bzl_library") +load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") package(default_visibility = ["//visibility:public"]) @@ -22,3 +23,8 @@ bzl_library( "//rust/private:rules", ], ) + +bool_flag( + name = "use-worker", + build_setting_default = False, +) diff --git a/rust/private/rust.bzl b/rust/private/rust.bzl index fe92e54de3..b581917c66 100644 --- a/rust/private/rust.bzl +++ b/rust/private/rust.bzl @@ -653,6 +653,13 @@ _common_attrs = { allow_single_file = True, cfg = "exec", ), + "_persistent_worker": attr.label( + default = Label("//util/worker"), + executable = True, + allow_single_file = True, + cfg = "exec", + ), + "_use_worker": attr.label(default = Label("//rust:use-worker")), } _rust_test_attrs = { diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index f8a8f8acc7..8adcdb27db 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -28,6 +28,7 @@ load( "relativize", "rule_attrs", ) +load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo") BuildInfo = provider( doc = "A provider containing `rustc` build settings for a given Crate.", @@ -55,6 +56,9 @@ ErrorFormatInfo = provider( fields = {"error_format": "(string) [" + ", ".join(_error_format_values) + "]"}, ) +def _use_worker(ctx): + return ctx.attr._use_worker[BuildSettingInfo].value + def _get_rustc_env(ctx, toolchain): """Gathers rustc environment variables @@ -355,6 +359,9 @@ def construct_arguments( # Wrapper args first args = ctx.actions.args() + if _use_worker(ctx): + args.set_param_file_format("multiline") + args.use_param_file("@%s", use_always = True) for build_env_file in build_env_files: args.add("--env-file", build_env_file) @@ -554,8 +561,17 @@ def rustc_compile_action( else: formatted_version = "" + executable = ctx.executable._process_wrapper + execution_requirements = {} + if _use_worker(ctx): + executable = ctx.executable._persistent_worker + execution_requirements = { + "supports-workers": "1", + "requires-worker-protocol": "proto" + } + ctx.actions.run( - executable = ctx.executable._process_wrapper, + executable = executable, inputs = compile_inputs, outputs = [crate_info.output], env = env, @@ -567,6 +583,7 @@ def rustc_compile_action( formatted_version, len(crate_info.srcs.to_list()), ), + execution_requirements = execution_requirements, ) dylibs = [get_preferred_artifact(lib) for linker_input in dep_info.transitive_noncrates.to_list() for lib in linker_input.libraries if _is_dylib(lib)] diff --git a/util/process_wrapper/BUILD.bazel b/util/process_wrapper/BUILD.bazel index 4b600369ee..a27603f26e 100644 --- a/util/process_wrapper/BUILD.bazel +++ b/util/process_wrapper/BUILD.bazel @@ -24,3 +24,11 @@ cc_binary( }), visibility = ["//visibility:public"], ) + +exports_files([ + "utils.h", + "utils.cc", + "system.h", + "system_windows.cc", + "system_posix.cc", +]) diff --git a/util/worker/BUILD b/util/worker/BUILD new file mode 100644 index 0000000000..26ff458b3a --- /dev/null +++ b/util/worker/BUILD @@ -0,0 +1,26 @@ +load("@rules_cc//cc:defs.bzl", "cc_binary") + +cc_binary( + name = "worker", + srcs = [ + "worker.cc", + "//util/process_wrapper:system.h", + "//util/process_wrapper:utils.h", + "//util/process_wrapper:utils.cc", + ] + select({ + "@bazel_tools//src/conditions:host_windows": [ + "//util/process_wrapper:system_windows.cc", + ], + "//conditions:default": [ + "//util/process_wrapper:system_posix.cc", + ], + }), + defines = [] + select({ + "@bazel_tools//src/conditions:host_windows": [ + "UNICODE", + "_UNICODE", + ], + "//conditions:default": [], + }), + visibility = ["//visibility:public"], +) diff --git a/util/worker/worker.cc b/util/worker/worker.cc new file mode 100644 index 0000000000..0bde401582 --- /dev/null +++ b/util/worker/worker.cc @@ -0,0 +1,197 @@ +// Copyright 2020 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include +#include +#include + +#include "util/process_wrapper/system.h" +#include "util/process_wrapper/utils.h" + +using CharType = process_wrapper::System::StrType::value_type; + +// Simple process wrapper allowing us to not depend on the shell to run a +// process to perform basic operations like capturing the output or having +// the $pwd used in command line arguments or environment variables +int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { + using namespace process_wrapper; + + System::EnvironmentBlock environment_block; + // Taking all environment variables from the current process + // and sending them down to the child process + for (int i = 0; envp[i] != nullptr; ++i) { + environment_block.push_back(envp[i]); + } + + using Subst = std::pair; + + System::StrType exec_path; + std::vector subst_mappings; + System::StrType stdout_file; + System::StrType stderr_file; + System::StrType touch_file; + System::StrType copy_source; + System::StrType copy_dest; + System::Arguments arguments; + System::Arguments file_arguments; + + // Processing current process argument list until -- is encountered + // everthing after gets sent down to the child process + for (int i = 1; i < argc; ++i) { + System::StrType arg = argv[i]; + if (++i == argc) { + std::cerr << "process wrapper error: argument \"" << ToUtf8(arg) + << "\" missing parameter.\n"; + return -1; + } + if (arg == PW_SYS_STR("--subst")) { + System::StrType subst = argv[i]; + size_t equal_pos = subst.find('='); + if (equal_pos == std::string::npos) { + std::cerr << "process wrapper error: wrong substituion format for \"" + << ToUtf8(subst) << "\".\n"; + return -1; + } + System::StrType key = subst.substr(0, equal_pos); + if (key.empty()) { + std::cerr << "process wrapper error: empty key for substituion \"" + << ToUtf8(subst) << "\".\n"; + return -1; + } + System::StrType value = subst.substr(equal_pos + 1, subst.size()); + if (value == PW_SYS_STR("${pwd}")) { + value = System::GetWorkingDirectory(); + } + subst_mappings.push_back({std::move(key), std::move(value)}); + } else if (arg == PW_SYS_STR("--env-file")) { + if (!ReadFileToArray(argv[i], environment_block)) { + return -1; + } + } else if (arg == PW_SYS_STR("--arg-file")) { + if (!ReadFileToArray(argv[i], file_arguments)) { + return -1; + } + } else if (arg == PW_SYS_STR("--touch-file")) { + if (!touch_file.empty()) { + std::cerr << "process wrapper error: \"--touch-file\" can only appear " + "once.\n"; + return -1; + } + touch_file = argv[i]; + } else if (arg == PW_SYS_STR("--copy-output")) { + // i is already at the first arg position, accountint we need another arg + // and then -- executable_name. + if (i + 1 > argc) { + std::cerr + << "process wrapper error: \"--copy-output\" needs 2 parameters.\n"; + return -1; + } + if (!copy_source.empty() || !copy_dest.empty()) { + std::cerr << "process wrapper error: \"--copy-output\" can only appear " + "once.\n"; + return -1; + } + copy_source = argv[i]; + copy_dest = argv[++i]; + if (copy_source == copy_dest) { + std::cerr << "process wrapper error: \"--copy-output\" source and dest " + "need to be different.\n"; + return -1; + } + } else if (arg == PW_SYS_STR("--stdout-file")) { + if (!stdout_file.empty()) { + std::cerr << "process wrapper error: \"--stdout-file\" can only appear " + "once.\n"; + return -1; + } + stdout_file = argv[i]; + } else if (arg == PW_SYS_STR("--stderr-file")) { + if (!stderr_file.empty()) { + std::cerr << "process wrapper error: \"--stderr-file\" can only appear " + "once.\n"; + return -1; + } + stderr_file = argv[i]; + } else if (arg == PW_SYS_STR("--")) { + exec_path = argv[i]; + for (++i; i < argc; ++i) { + arguments.push_back(argv[i]); + } + // after we consume all arguments we append the files arguments + for (const System::StrType& file_arg : file_arguments) { + arguments.push_back(file_arg); + } + } else { + std::cerr << "process wrapper error: unknow argument \"" << ToUtf8(arg) + << "\"." << '\n'; + return -1; + } + } + + if (subst_mappings.size()) { + for (const Subst& subst : subst_mappings) { + System::StrType token = PW_SYS_STR("${"); + token += subst.first; + token.push_back('}'); + for (System::StrType& arg : arguments) { + ReplaceToken(arg, token, subst.second); + } + + for (System::StrType& env : environment_block) { + ReplaceToken(env, token, subst.second); + } + } + } + + std::cerr << "NIKHILM HELLO\n"; + return 1; + + // Have the last values added take precedence over the first. + // This is simpler than needing to track duplicates and explicitly override them. + std::reverse(environment_block.begin(), environment_block.end()); + + int exit_code = System::Exec(exec_path, arguments, environment_block, + stdout_file, stderr_file); + if (exit_code == 0) { + if (!touch_file.empty()) { + std::ofstream file(touch_file); + if (file.fail()) { + std::cerr << "process wrapper error: failed to create touch file: \"" + << ToUtf8(touch_file) << "\"\n"; + return -1; + } + file.close(); + } + + // we perform a copy of the output if necessary + if (!copy_source.empty() && !copy_dest.empty()) { + std::ifstream source(copy_source, std::ios::binary); + if (source.fail()) { + std::cerr << "process wrapper error: failed to open copy source: \"" + << ToUtf8(copy_source) << "\"\n"; + return -1; + } + std::ofstream dest(copy_dest, std::ios::binary); + if (dest.fail()) { + std::cerr << "process wrapper error: failed to open copy dest: \"" + << ToUtf8(copy_dest) << "\"\n"; + return -1; + } + dest << source.rdbuf(); + } + } + return exit_code; +} From d0cd02636e4420f020d623d1eb2de618bcdb6008 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 23 Mar 2021 22:13:45 -0700 Subject: [PATCH 02/18] Now capable of parsing the WorkRequest --- util/worker/BUILD | 12 +++ util/worker/worker.cc | 151 +++++++++++------------------- util/worker/worker_protocol.proto | 62 ++++++++++++ 3 files changed, 128 insertions(+), 97 deletions(-) create mode 100644 util/worker/worker_protocol.proto diff --git a/util/worker/BUILD b/util/worker/BUILD index 26ff458b3a..72c66430b9 100644 --- a/util/worker/BUILD +++ b/util/worker/BUILD @@ -1,4 +1,5 @@ load("@rules_cc//cc:defs.bzl", "cc_binary") +load("@rules_proto//proto:defs.bzl", "proto_library"); cc_binary( name = "worker", @@ -15,6 +16,7 @@ cc_binary( "//util/process_wrapper:system_posix.cc", ], }), + deps = [":worker_cc_proto"], defines = [] + select({ "@bazel_tools//src/conditions:host_windows": [ "UNICODE", @@ -24,3 +26,13 @@ cc_binary( }), visibility = ["//visibility:public"], ) + +cc_proto_library( + name = "worker_cc_proto", + deps = [":worker_protocol"], +) + +proto_library( + name = "worker_protocol", + srcs = ["worker_protocol.proto"], +) diff --git a/util/worker/worker.cc b/util/worker/worker.cc index 0bde401582..c4461cbd25 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -18,11 +18,43 @@ #include #include +#include +#include + #include "util/process_wrapper/system.h" #include "util/process_wrapper/utils.h" +#include "util/worker/worker_protocol.pb.h" + +// TODO: Library and app split. +using blaze::worker::WorkRequest; +using google::protobuf::io::CodedInputStream; +using google::protobuf::io::FileInputStream; using CharType = process_wrapper::System::StrType::value_type; +bool ReadRequest(CodedInputStream *stream, WorkRequest *request) +{ + uint32_t req_len; + if (!stream->ReadVarint32(&req_len)) { + std::cerr << "Unable to read message length\n"; + return false; + } + + CodedInputStream::Limit limit = stream->PushLimit(req_len); + if (!request->MergeFromCodedStream(stream)) { + std::cerr << "Unable to merge from stream\n"; + return false; + } + + if (!stream->ConsumedEntireMessage()) { + std::cerr << "Did not consume entire message\n"; + return false; + } + + stream->PopLimit(limit); + return true; +} + // Simple process wrapper allowing us to not depend on the shell to run a // process to perform basic operations like capturing the output or having // the $pwd used in command line arguments or environment variables @@ -38,6 +70,9 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { using Subst = std::pair; + // This will need support for understanding param file argument. + // As well as parsing other flags generally. + System::StrType exec_path; std::vector subst_mappings; System::StrType stdout_file; @@ -48,117 +83,39 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { System::Arguments arguments; System::Arguments file_arguments; + bool as_worker = false; + // Processing current process argument list until -- is encountered // everthing after gets sent down to the child process for (int i = 1; i < argc; ++i) { System::StrType arg = argv[i]; - if (++i == argc) { - std::cerr << "process wrapper error: argument \"" << ToUtf8(arg) - << "\" missing parameter.\n"; - return -1; - } - if (arg == PW_SYS_STR("--subst")) { - System::StrType subst = argv[i]; - size_t equal_pos = subst.find('='); - if (equal_pos == std::string::npos) { - std::cerr << "process wrapper error: wrong substituion format for \"" - << ToUtf8(subst) << "\".\n"; - return -1; - } - System::StrType key = subst.substr(0, equal_pos); - if (key.empty()) { - std::cerr << "process wrapper error: empty key for substituion \"" - << ToUtf8(subst) << "\".\n"; - return -1; - } - System::StrType value = subst.substr(equal_pos + 1, subst.size()); - if (value == PW_SYS_STR("${pwd}")) { - value = System::GetWorkingDirectory(); - } - subst_mappings.push_back({std::move(key), std::move(value)}); - } else if (arg == PW_SYS_STR("--env-file")) { - if (!ReadFileToArray(argv[i], environment_block)) { - return -1; - } - } else if (arg == PW_SYS_STR("--arg-file")) { - if (!ReadFileToArray(argv[i], file_arguments)) { - return -1; - } - } else if (arg == PW_SYS_STR("--touch-file")) { - if (!touch_file.empty()) { - std::cerr << "process wrapper error: \"--touch-file\" can only appear " - "once.\n"; - return -1; - } - touch_file = argv[i]; - } else if (arg == PW_SYS_STR("--copy-output")) { - // i is already at the first arg position, accountint we need another arg - // and then -- executable_name. - if (i + 1 > argc) { - std::cerr - << "process wrapper error: \"--copy-output\" needs 2 parameters.\n"; - return -1; - } - if (!copy_source.empty() || !copy_dest.empty()) { - std::cerr << "process wrapper error: \"--copy-output\" can only appear " - "once.\n"; - return -1; - } - copy_source = argv[i]; - copy_dest = argv[++i]; - if (copy_source == copy_dest) { - std::cerr << "process wrapper error: \"--copy-output\" source and dest " - "need to be different.\n"; - return -1; - } - } else if (arg == PW_SYS_STR("--stdout-file")) { - if (!stdout_file.empty()) { - std::cerr << "process wrapper error: \"--stdout-file\" can only appear " - "once.\n"; - return -1; - } - stdout_file = argv[i]; - } else if (arg == PW_SYS_STR("--stderr-file")) { - if (!stderr_file.empty()) { - std::cerr << "process wrapper error: \"--stderr-file\" can only appear " - "once.\n"; - return -1; - } - stderr_file = argv[i]; - } else if (arg == PW_SYS_STR("--")) { - exec_path = argv[i]; - for (++i; i < argc; ++i) { - arguments.push_back(argv[i]); - } - // after we consume all arguments we append the files arguments - for (const System::StrType& file_arg : file_arguments) { - arguments.push_back(file_arg); - } + if (arg == PW_SYS_STR("--persistent_worker")) { + as_worker = true; } else { - std::cerr << "process wrapper error: unknow argument \"" << ToUtf8(arg) + std::cerr << "worker wrapper error: unknown argument \"" << ToUtf8(arg) << "\"." << '\n'; return -1; } } - if (subst_mappings.size()) { - for (const Subst& subst : subst_mappings) { - System::StrType token = PW_SYS_STR("${"); - token += subst.first; - token.push_back('}'); - for (System::StrType& arg : arguments) { - ReplaceToken(arg, token, subst.second); - } + if (!as_worker) { + std::cerr << "Don't know how to run as non-worker yet\n"; + return -1; + } - for (System::StrType& env : environment_block) { - ReplaceToken(env, token, subst.second); - } + CodedInputStream *stream = new CodedInputStream(new FileInputStream(0)); + while (true) { + WorkRequest request; + if (!ReadRequest(stream, &request)) { + return 1; } + for (int i = 0; i < request.arguments_size(); i++) { + std::cerr << request.arguments(i) << " "; + } + std::cerr << "\n"; + return 1; } - std::cerr << "NIKHILM HELLO\n"; - return 1; - // Have the last values added take precedence over the first. // This is simpler than needing to track duplicates and explicitly override them. std::reverse(environment_block.begin(), environment_block.end()); diff --git a/util/worker/worker_protocol.proto b/util/worker/worker_protocol.proto new file mode 100644 index 0000000000..c628b7eb7a --- /dev/null +++ b/util/worker/worker_protocol.proto @@ -0,0 +1,62 @@ +// Copyright 2015 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +syntax = "proto3"; + +package blaze.worker; + +option java_package = "com.google.devtools.build.lib.worker"; + +// An input file. +message Input { + // The path in the file system where to read this input artifact from. This is + // either a path relative to the execution root (the worker process is + // launched with the working directory set to the execution root), or an + // absolute path. + string path = 1; + + // A hash-value of the contents. The format of the contents is unspecified and + // the digest should be treated as an opaque token. + bytes digest = 2; +} + +// This represents a single work unit that Blaze sends to the worker. +message WorkRequest { + repeated string arguments = 1; + + // The inputs that the worker is allowed to read during execution of this + // request. + repeated Input inputs = 2; + + // To support multiplex worker, each WorkRequest must have an unique ID. This + // ID should be attached unchanged to the WorkResponse. + int32 request_id = 3; +} + +// The worker sends this message to Blaze when it finished its work on the +// WorkRequest message. +message WorkResponse { + int32 exit_code = 1; + + // This is printed to the user after the WorkResponse has been received and is + // supposed to contain compiler warnings / errors etc. - thus we'll use a + // string type here, which gives us UTF-8 encoding. + string output = 2; + + // To support multiplex worker, each WorkResponse must have an unique ID. + // Since worker processes which support multiplex worker will handle multiple + // WorkRequests in parallel, this ID will be used to determined which + // WorkerProxy does this WorkResponse belong to. + int32 request_id = 3; +} From d8e95eccafc482c563205d12f79a888782f3572c Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 23 Mar 2021 22:46:00 -0700 Subject: [PATCH 03/18] write fake worker responses. next up is to actually execute the command. --- util/worker/worker.cc | 49 ++++++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/util/worker/worker.cc b/util/worker/worker.cc index c4461cbd25..92b89e7898 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -27,34 +28,46 @@ // TODO: Library and app split. using blaze::worker::WorkRequest; +using blaze::worker::WorkResponse; using google::protobuf::io::CodedInputStream; +using google::protobuf::io::CodedOutputStream; using google::protobuf::io::FileInputStream; +using google::protobuf::io::FileOutputStream; using CharType = process_wrapper::System::StrType::value_type; -bool ReadRequest(CodedInputStream *stream, WorkRequest *request) +bool ReadRequest(CodedInputStream &stream, WorkRequest *request) { uint32_t req_len; - if (!stream->ReadVarint32(&req_len)) { + if (!stream.ReadVarint32(&req_len)) { std::cerr << "Unable to read message length\n"; return false; } - CodedInputStream::Limit limit = stream->PushLimit(req_len); - if (!request->MergeFromCodedStream(stream)) { + CodedInputStream::Limit limit = stream.PushLimit(req_len); + if (!request->MergeFromCodedStream(&stream)) { std::cerr << "Unable to merge from stream\n"; return false; } - if (!stream->ConsumedEntireMessage()) { + if (!stream.ConsumedEntireMessage()) { std::cerr << "Did not consume entire message\n"; return false; } - stream->PopLimit(limit); + stream.PopLimit(limit); return true; } +std::unique_ptr HandleRequest(const WorkRequest &request) { + std::unique_ptr response(new WorkResponse()); + // TODO: Fix to correct values. + response->set_exit_code(1); + response->set_request_id(request.request_id()); + response->set_output("FAKE ERROR\n"); + return response; +} + // Simple process wrapper allowing us to not depend on the shell to run a // process to perform basic operations like capturing the output or having // the $pwd used in command line arguments or environment variables @@ -103,17 +116,29 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { return -1; } - CodedInputStream *stream = new CodedInputStream(new FileInputStream(0)); + std::unique_ptr input(new CodedInputStream(new FileInputStream(0))); + // TODO: Figure out ownership. + FileOutputStream *out = new FileOutputStream(1); + std::unique_ptr output(new CodedOutputStream(out)); + while (true) { WorkRequest request; - if (!ReadRequest(stream, &request)) { + if (!ReadRequest(*input, &request)) { return 1; } - for (int i = 0; i < request.arguments_size(); i++) { - std::cerr << request.arguments(i) << " "; + + std::unique_ptr response; + if ((response = HandleRequest(request)) == nullptr) { + return 1; + } + uint32_t size = response->ByteSize(); + output->WriteVarint32(size); + response->SerializeWithCachedSizes(output.get()); + if (output->HadError()) { + std::cerr << "Error serializing response\n"; + return 1; } - std::cerr << "\n"; - return 1; + out->Flush(); } // Have the last values added take precedence over the first. From 122be7f9edde01fd04dcc5f7a761f6db687806b8 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Thu, 25 Mar 2021 08:44:41 -0700 Subject: [PATCH 04/18] Rename flag to indicate experimental nature --- rust/BUILD.bazel | 2 +- rust/private/rust.bzl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/BUILD.bazel b/rust/BUILD.bazel index ffd27c1819..3fc0bde42b 100644 --- a/rust/BUILD.bazel +++ b/rust/BUILD.bazel @@ -25,6 +25,6 @@ bzl_library( ) bool_flag( - name = "use-worker", + name = "experimental-use-worker", build_setting_default = False, ) diff --git a/rust/private/rust.bzl b/rust/private/rust.bzl index b581917c66..fa4539828a 100644 --- a/rust/private/rust.bzl +++ b/rust/private/rust.bzl @@ -659,7 +659,7 @@ _common_attrs = { allow_single_file = True, cfg = "exec", ), - "_use_worker": attr.label(default = Label("//rust:use-worker")), + "_use_worker": attr.label(default = Label("//rust:experimental-use-worker")), } _rust_test_attrs = { From 45da97c4cba4031f4bb1aae9840eb13771a4c174 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Thu, 25 Mar 2021 09:09:41 -0700 Subject: [PATCH 05/18] Worker runs command, but without incremental mode and without redirecting stderr correctly. --- rust/private/rustc.bzl | 7 ++- util/worker/worker.cc | 114 ++++++++++++++++++++--------------------- 2 files changed, 62 insertions(+), 59 deletions(-) diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index 8adcdb27db..77072574eb 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -562,12 +562,15 @@ def rustc_compile_action( formatted_version = "" executable = ctx.executable._process_wrapper + arguments = [args] execution_requirements = {} if _use_worker(ctx): + # Pass the "compiler" to the worker. + arguments = ["--compiler", executable.path, args] executable = ctx.executable._persistent_worker execution_requirements = { "supports-workers": "1", - "requires-worker-protocol": "proto" + "requires-worker-protocol": "proto", } ctx.actions.run( @@ -575,7 +578,7 @@ def rustc_compile_action( inputs = compile_inputs, outputs = [crate_info.output], env = env, - arguments = [args], + arguments = arguments, mnemonic = "Rustc", progress_message = "Compiling Rust {} {}{} ({} files)".format( crate_info.type, diff --git a/util/worker/worker.cc b/util/worker/worker.cc index 92b89e7898..a172d822a6 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -34,6 +34,8 @@ using google::protobuf::io::CodedOutputStream; using google::protobuf::io::FileInputStream; using google::protobuf::io::FileOutputStream; +using namespace process_wrapper; + using CharType = process_wrapper::System::StrType::value_type; bool ReadRequest(CodedInputStream &stream, WorkRequest *request) @@ -59,21 +61,56 @@ bool ReadRequest(CodedInputStream &stream, WorkRequest *request) return true; } -std::unique_ptr HandleRequest(const WorkRequest &request) { - std::unique_ptr response(new WorkResponse()); - // TODO: Fix to correct values. - response->set_exit_code(1); - response->set_request_id(request.request_id()); - response->set_output("FAKE ERROR\n"); - return response; +std::unique_ptr HandleRequest(const WorkRequest &request, const System::StrType& exec_path, const System::EnvironmentBlock& environment_block) { + System::StrType stdout_file; + System::StrType stderr_file; + System::StrType copy_source; + System::StrType copy_dest; + System::Arguments arguments(request.arguments_size()); + + for (int i = 0; i < request.arguments_size(); i++) { + // TODO: Probably some way to copy the entire memory into the vector in one go. + arguments[i] = request.arguments(i); + } + // TODO: Add incremental arg. + + int exit_code = System::Exec(exec_path, arguments, environment_block, + stdout_file, stderr_file); + if (exit_code == 0) { + // we perform a copy of the output if necessary + if (!copy_source.empty() && !copy_dest.empty()) { + std::ifstream source(copy_source, std::ios::binary); + if (source.fail()) { + std::cerr << "process wrapper error: failed to open copy source: \"" + << ToUtf8(copy_source) << "\"\n"; + //return -1; + } + std::ofstream dest(copy_dest, std::ios::binary); + if (dest.fail()) { + std::cerr << "process wrapper error: failed to open copy dest: \"" + << ToUtf8(copy_dest) << "\"\n"; + //return -1; + } + dest << source.rdbuf(); + } + } else { + std::cerr << "NIKHILM exit " << exit_code << '\n'; + } + + std::unique_ptr response(new WorkResponse()); + // TODO: Fix to correct values. + response->set_exit_code(exit_code); + response->set_request_id(request.request_id()); + response->set_output("FAKE OUTPUT\n"); + return response; } + // Simple process wrapper allowing us to not depend on the shell to run a // process to perform basic operations like capturing the output or having // the $pwd used in command line arguments or environment variables int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { - using namespace process_wrapper; - + System::StrType exec_path; System::EnvironmentBlock environment_block; // Taking all environment variables from the current process // and sending them down to the child process @@ -81,21 +118,13 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { environment_block.push_back(envp[i]); } - using Subst = std::pair; + // Have the last values added take precedence over the first. + // This is simpler than needing to track duplicates and explicitly override them. + std::reverse(environment_block.begin(), environment_block.end()); // This will need support for understanding param file argument. // As well as parsing other flags generally. - System::StrType exec_path; - std::vector subst_mappings; - System::StrType stdout_file; - System::StrType stderr_file; - System::StrType touch_file; - System::StrType copy_source; - System::StrType copy_dest; - System::Arguments arguments; - System::Arguments file_arguments; - bool as_worker = false; // Processing current process argument list until -- is encountered @@ -103,7 +132,13 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { for (int i = 1; i < argc; ++i) { System::StrType arg = argv[i]; if (arg == PW_SYS_STR("--persistent_worker")) { - as_worker = true; + as_worker = true; + } else if (arg == PW_SYS_STR("--compiler")) { + if (++i == argc) { + std::cerr << "--compiler flag missing argument\n"; + return -1; + } + exec_path = argv[i]; } else { std::cerr << "worker wrapper error: unknown argument \"" << ToUtf8(arg) << "\"." << '\n'; @@ -128,7 +163,7 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { } std::unique_ptr response; - if ((response = HandleRequest(request)) == nullptr) { + if ((response = HandleRequest(request, exec_path, environment_block)) == nullptr) { return 1; } uint32_t size = response->ByteSize(); @@ -141,39 +176,4 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { out->Flush(); } - // Have the last values added take precedence over the first. - // This is simpler than needing to track duplicates and explicitly override them. - std::reverse(environment_block.begin(), environment_block.end()); - - int exit_code = System::Exec(exec_path, arguments, environment_block, - stdout_file, stderr_file); - if (exit_code == 0) { - if (!touch_file.empty()) { - std::ofstream file(touch_file); - if (file.fail()) { - std::cerr << "process wrapper error: failed to create touch file: \"" - << ToUtf8(touch_file) << "\"\n"; - return -1; - } - file.close(); - } - - // we perform a copy of the output if necessary - if (!copy_source.empty() && !copy_dest.empty()) { - std::ifstream source(copy_source, std::ios::binary); - if (source.fail()) { - std::cerr << "process wrapper error: failed to open copy source: \"" - << ToUtf8(copy_source) << "\"\n"; - return -1; - } - std::ofstream dest(copy_dest, std::ios::binary); - if (dest.fail()) { - std::cerr << "process wrapper error: failed to open copy dest: \"" - << ToUtf8(copy_dest) << "\"\n"; - return -1; - } - dest << source.rdbuf(); - } - } - return exit_code; } From 9540dc6bc8cb538892e985e9812b007d16752593 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Sun, 28 Mar 2021 16:05:57 -0700 Subject: [PATCH 06/18] Sending command stderr to worker response output. works on Linux. --- util/worker/worker.cc | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/util/worker/worker.cc b/util/worker/worker.cc index a172d822a6..f215deb9d6 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -73,35 +74,29 @@ std::unique_ptr HandleRequest(const WorkRequest &request, const Sy arguments[i] = request.arguments(i); } // TODO: Add incremental arg. + // + + // Create a file to write stderr to. + std::stringstream fn_stream; + fn_stream << System::GetWorkingDirectory() << "/stderr_" << request.request_id() << ".log"; + stderr_file = fn_stream.str(); int exit_code = System::Exec(exec_path, arguments, environment_block, stdout_file, stderr_file); - if (exit_code == 0) { - // we perform a copy of the output if necessary - if (!copy_source.empty() && !copy_dest.empty()) { - std::ifstream source(copy_source, std::ios::binary); - if (source.fail()) { - std::cerr << "process wrapper error: failed to open copy source: \"" - << ToUtf8(copy_source) << "\"\n"; - //return -1; - } - std::ofstream dest(copy_dest, std::ios::binary); - if (dest.fail()) { - std::cerr << "process wrapper error: failed to open copy dest: \"" - << ToUtf8(copy_dest) << "\"\n"; - //return -1; - } - dest << source.rdbuf(); - } + std::ifstream source(fn_stream.str(), std::ios::binary); + std::string stderr_output; + if (source.fail()) { + stderr_output = "[worker] Error getting stderr\n"; } else { - std::cerr << "NIKHILM exit " << exit_code << '\n'; + std::stringstream stderr_stream; + stderr_stream << source.rdbuf(); + stderr_output = stderr_stream.str(); } std::unique_ptr response(new WorkResponse()); - // TODO: Fix to correct values. response->set_exit_code(exit_code); response->set_request_id(request.request_id()); - response->set_output("FAKE OUTPUT\n"); + response->set_output(stderr_output); return response; } From 3018ff9a6e672b230e2e168586230f72c9365c03 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Mon, 29 Mar 2021 21:19:41 -0700 Subject: [PATCH 07/18] Use compilation mode as part of incremental dir path --- rust/private/rustc.bzl | 7 +++++-- util/worker/worker.cc | 27 +++++++++++++++++++++++---- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index 77072574eb..fcbe210408 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -565,8 +565,11 @@ def rustc_compile_action( arguments = [args] execution_requirements = {} if _use_worker(ctx): - # Pass the "compiler" to the worker. - arguments = ["--compiler", executable.path, args] + arguments = [ + "--compiler", executable.path, + "--compilation_mode", ctx.var["COMPILATION_MODE"], + args, + ] executable = ctx.executable._persistent_worker execution_requirements = { "supports-workers": "1", diff --git a/util/worker/worker.cc b/util/worker/worker.cc index f215deb9d6..5c8b726cb2 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -62,7 +62,12 @@ bool ReadRequest(CodedInputStream &stream, WorkRequest *request) return true; } -std::unique_ptr HandleRequest(const WorkRequest &request, const System::StrType& exec_path, const System::EnvironmentBlock& environment_block) { +std::unique_ptr HandleRequest( + const WorkRequest &request, + const System::StrType& exec_path, + const System::StrType& compilation_mode, + const System::EnvironmentBlock& environment_block +) { System::StrType stdout_file; System::StrType stderr_file; System::StrType copy_source; @@ -73,8 +78,15 @@ std::unique_ptr HandleRequest(const WorkRequest &request, const Sy // TODO: Probably some way to copy the entire memory into the vector in one go. arguments[i] = request.arguments(i); } - // TODO: Add incremental arg. - // + + // Considering + // https://github.com/rust-lang/rust/blob/673d0db5e393e9c64897005b470bfeb6d5aec61b/compiler/rustc_incremental/src/persist/fs.rs#L145 + // as the canonical description of how incremental compilation is affected by + // the choice of directory, it helps to segment based on compilation mode. + + arguments.push_back("--codegen"); + // TODO: Can be shared across requests to avoid concatenation. + arguments.push_back("incremental=" + System::GetWorkingDirectory() + "/rustc-target-" + compilation_mode + "/incremental"); // Create a file to write stderr to. std::stringstream fn_stream; @@ -106,6 +118,7 @@ std::unique_ptr HandleRequest(const WorkRequest &request, const Sy // the $pwd used in command line arguments or environment variables int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { System::StrType exec_path; + System::StrType compilation_mode; System::EnvironmentBlock environment_block; // Taking all environment variables from the current process // and sending them down to the child process @@ -128,6 +141,12 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { System::StrType arg = argv[i]; if (arg == PW_SYS_STR("--persistent_worker")) { as_worker = true; + } else if (arg == PW_SYS_STR("--compilation_mode")) { + if (++i == argc) { + std::cerr << "--compilation_mode flag missing argument\n"; + return -1; + } + compilation_mode = argv[i]; } else if (arg == PW_SYS_STR("--compiler")) { if (++i == argc) { std::cerr << "--compiler flag missing argument\n"; @@ -158,7 +177,7 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { } std::unique_ptr response; - if ((response = HandleRequest(request, exec_path, environment_block)) == nullptr) { + if ((response = HandleRequest(request, exec_path, compilation_mode, environment_block)) == nullptr) { return 1; } uint32_t size = response->ByteSize(); From ed6a0c39acf8d60d26c86e0bd10e2884f402efa7 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Mon, 29 Mar 2021 23:46:47 -0700 Subject: [PATCH 08/18] When running the worker, the original executable should be marked as an input via tools. --- rust/private/rustc.bzl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index fcbe210408..82da7f697b 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -562,9 +562,11 @@ def rustc_compile_action( formatted_version = "" executable = ctx.executable._process_wrapper + tools = [] arguments = [args] execution_requirements = {} if _use_worker(ctx): + tools = [executable] arguments = [ "--compiler", executable.path, "--compilation_mode", ctx.var["COMPILATION_MODE"], @@ -581,6 +583,7 @@ def rustc_compile_action( inputs = compile_inputs, outputs = [crate_info.output], env = env, + tools = tools, arguments = arguments, mnemonic = "Rustc", progress_message = "Compiling Rust {} {}{} ({} files)".format( From 922efe24c45782d49effe9218e7b81d44c0feefb Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Mon, 29 Mar 2021 23:50:39 -0700 Subject: [PATCH 09/18] Oof! This was traumatic. A CodedOutputStream preserves its contents in some sense, possibly because of the use of EpsOutputStream and FileOutputStream beneath, which seem to keep buffers around. This was causing the original WorkResponse to be repeatedly written to stdout on future invocations of the worker, leading to Bazel not waiting around for the command to actually run. Specifically, I believe what was happening was, the internal buffer had the initial WorkResponse. Every time Bazel sent a request, it tried to read the response and protobuf would just send the already available response right away. --- util/worker/worker.cc | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/util/worker/worker.cc b/util/worker/worker.cc index 5c8b726cb2..2655219241 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -88,6 +88,9 @@ std::unique_ptr HandleRequest( // TODO: Can be shared across requests to avoid concatenation. arguments.push_back("incremental=" + System::GetWorkingDirectory() + "/rustc-target-" + compilation_mode + "/incremental"); + std::stringstream fn_stdout_stream; + fn_stdout_stream << System::GetWorkingDirectory() << "/stdout_" << request.request_id() << ".log"; + stdout_file = fn_stdout_stream.str(); // Create a file to write stderr to. std::stringstream fn_stream; fn_stream << System::GetWorkingDirectory() << "/stderr_" << request.request_id() << ".log"; @@ -113,9 +116,6 @@ std::unique_ptr HandleRequest( } -// Simple process wrapper allowing us to not depend on the shell to run a -// process to perform basic operations like capturing the output or having -// the $pwd used in command line arguments or environment variables int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { System::StrType exec_path; System::StrType compilation_mode; @@ -168,7 +168,6 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { std::unique_ptr input(new CodedInputStream(new FileInputStream(0))); // TODO: Figure out ownership. FileOutputStream *out = new FileOutputStream(1); - std::unique_ptr output(new CodedOutputStream(out)); while (true) { WorkRequest request; @@ -176,16 +175,21 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { return 1; } + // TODO: Stack allocate and pass by reference. std::unique_ptr response; if ((response = HandleRequest(request, exec_path, compilation_mode, environment_block)) == nullptr) { return 1; } - uint32_t size = response->ByteSize(); - output->WriteVarint32(size); - response->SerializeWithCachedSizes(output.get()); - if (output->HadError()) { - std::cerr << "Error serializing response\n"; - return 1; + std::cerr << time(nullptr) << " YOWH\n"; + { + uint32_t size = response->ByteSize(); + CodedOutputStream output(out); + output.WriteVarint32(size); + response->SerializeWithCachedSizes(&output); + if (output.HadError()) { + std::cerr << "Error serializing response\n"; + return 1; + } } out->Flush(); } From a8da854016cbf4c9b580ca28f154a363867e9319 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 30 Mar 2021 09:46:18 -0700 Subject: [PATCH 10/18] Readability fixes and cleanup --- util/worker/worker.cc | 77 ++++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 42 deletions(-) diff --git a/util/worker/worker.cc b/util/worker/worker.cc index 2655219241..fcaad64e78 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -27,7 +27,6 @@ #include "util/process_wrapper/utils.h" #include "util/worker/worker_protocol.pb.h" -// TODO: Library and app split. using blaze::worker::WorkRequest; using blaze::worker::WorkResponse; using google::protobuf::io::CodedInputStream; @@ -37,18 +36,17 @@ using google::protobuf::io::FileOutputStream; using namespace process_wrapper; -using CharType = process_wrapper::System::StrType::value_type; - -bool ReadRequest(CodedInputStream &stream, WorkRequest *request) +bool ReadRequest(FileInputStream *input, WorkRequest &request) { uint32_t req_len; + CodedInputStream stream(input); if (!stream.ReadVarint32(&req_len)) { std::cerr << "Unable to read message length\n"; return false; } CodedInputStream::Limit limit = stream.PushLimit(req_len); - if (!request->MergeFromCodedStream(&stream)) { + if (!request.MergeFromCodedStream(&stream)) { std::cerr << "Unable to merge from stream\n"; return false; } @@ -62,43 +60,38 @@ bool ReadRequest(CodedInputStream &stream, WorkRequest *request) return true; } -std::unique_ptr HandleRequest( +bool HandleRequest( const WorkRequest &request, + WorkResponse &response, const System::StrType& exec_path, const System::StrType& compilation_mode, const System::EnvironmentBlock& environment_block ) { - System::StrType stdout_file; - System::StrType stderr_file; - System::StrType copy_source; - System::StrType copy_dest; - System::Arguments arguments(request.arguments_size()); + System::Arguments arguments; + // Pre-allocate. +2 for the incremental argument. + arguments.reserve(request.arguments_size() + 2); + auto request_args = request.arguments(); for (int i = 0; i < request.arguments_size(); i++) { - // TODO: Probably some way to copy the entire memory into the vector in one go. - arguments[i] = request.arguments(i); + arguments.push_back(request.arguments(i)); } // Considering // https://github.com/rust-lang/rust/blob/673d0db5e393e9c64897005b470bfeb6d5aec61b/compiler/rustc_incremental/src/persist/fs.rs#L145 // as the canonical description of how incremental compilation is affected by // the choice of directory, it helps to segment based on compilation mode. - + // That prevents the GC phase from clearing the cache of a debug build when running an opt build. arguments.push_back("--codegen"); // TODO: Can be shared across requests to avoid concatenation. arguments.push_back("incremental=" + System::GetWorkingDirectory() + "/rustc-target-" + compilation_mode + "/incremental"); - std::stringstream fn_stdout_stream; - fn_stdout_stream << System::GetWorkingDirectory() << "/stdout_" << request.request_id() << ".log"; - stdout_file = fn_stdout_stream.str(); - // Create a file to write stderr to. - std::stringstream fn_stream; - fn_stream << System::GetWorkingDirectory() << "/stderr_" << request.request_id() << ".log"; - stderr_file = fn_stream.str(); + // Since the worker is not multiplexed, we can always log to the same file and overwrite on the next request. + System::StrType stdout_file = System::GetWorkingDirectory() + "/stdout.log"; + System::StrType stderr_file = System::GetWorkingDirectory() + "/stderr.log"; int exit_code = System::Exec(exec_path, arguments, environment_block, stdout_file, stderr_file); - std::ifstream source(fn_stream.str(), std::ios::binary); + std::ifstream source(stderr_file, std::ios::binary); std::string stderr_output; if (source.fail()) { stderr_output = "[worker] Error getting stderr\n"; @@ -108,14 +101,15 @@ std::unique_ptr HandleRequest( stderr_output = stderr_stream.str(); } - std::unique_ptr response(new WorkResponse()); - response->set_exit_code(exit_code); - response->set_request_id(request.request_id()); - response->set_output(stderr_output); - return response; + response.set_exit_code(exit_code); + response.set_request_id(request.request_id()); + response.set_output(std::move(stderr_output)); + return true; } +using CharType = process_wrapper::System::StrType::value_type; + int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { System::StrType exec_path; System::StrType compilation_mode; @@ -165,33 +159,32 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { return -1; } - std::unique_ptr input(new CodedInputStream(new FileInputStream(0))); - // TODO: Figure out ownership. - FileOutputStream *out = new FileOutputStream(1); + std::unique_ptr input(new FileInputStream(0)); + std::unique_ptr output(new FileOutputStream(1)); while (true) { WorkRequest request; - if (!ReadRequest(*input, &request)) { + if (!ReadRequest(input.get(), request)) { return 1; } - // TODO: Stack allocate and pass by reference. - std::unique_ptr response; - if ((response = HandleRequest(request, exec_path, compilation_mode, environment_block)) == nullptr) { + WorkResponse response; + if (!HandleRequest(request, response, exec_path, compilation_mode, environment_block)) { return 1; } - std::cerr << time(nullptr) << " YOWH\n"; + + // A CodedInputStream will try to move around the underlying buffer when destroyed. + // If we Flush stdout, that fails. So ensure it goes out of scope before we flush. { - uint32_t size = response->ByteSize(); - CodedOutputStream output(out); - output.WriteVarint32(size); - response->SerializeWithCachedSizes(&output); - if (output.HadError()) { + CodedOutputStream coded_out(output.get()); + coded_out.WriteVarint32(response.ByteSize()); + response.SerializeWithCachedSizes(&coded_out); + if (coded_out.HadError()) { std::cerr << "Error serializing response\n"; return 1; } } - out->Flush(); + output->Flush(); } - + return 0; } From e02966b6c5a7fdf0428d6eb4489523b428f414fd Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 30 Mar 2021 17:27:39 -0700 Subject: [PATCH 11/18] Encode target triple in incremental dir path Consider the layout documented in https://github.com/rust-lang/cargo/blob/58a961314437258065e23cb6316dfc121d96fb71/src/cargo/core/compiler/layout.rs#L50. It relies on the incremental dir being in `target//{debug,release}/incremental`. With this change, Bazel is creating a very similar structure. --- util/worker/worker.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/util/worker/worker.cc b/util/worker/worker.cc index fcaad64e78..882d6ae4ca 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -72,8 +72,14 @@ bool HandleRequest( arguments.reserve(request.arguments_size() + 2); auto request_args = request.arguments(); + std::string target; for (int i = 0; i < request.arguments_size(); i++) { - arguments.push_back(request.arguments(i)); + auto argument = request.arguments(i); + // Starts with + if (argument.rfind("--target=", 0) != std::string::npos) { + target = argument.substr(9) + '/'; + } + arguments.push_back(argument); } // Considering @@ -83,7 +89,7 @@ bool HandleRequest( // That prevents the GC phase from clearing the cache of a debug build when running an opt build. arguments.push_back("--codegen"); // TODO: Can be shared across requests to avoid concatenation. - arguments.push_back("incremental=" + System::GetWorkingDirectory() + "/rustc-target-" + compilation_mode + "/incremental"); + arguments.push_back("incremental=" + System::GetWorkingDirectory() + "/rustc-target/" + target + compilation_mode + "/incremental"); // Since the worker is not multiplexed, we can always log to the same file and overwrite on the next request. System::StrType stdout_file = System::GetWorkingDirectory() + "/stdout.log"; From 41f8049d476d28c4f8ce1773de45321c2bc0e73a Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 30 Mar 2021 17:38:23 -0700 Subject: [PATCH 12/18] Add documentation on how to use the C++ worker, which is a lot easier --- docs/index.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/docs/index.md b/docs/index.md index 8227e4ec44..1a7e770f2f 100644 --- a/docs/index.md +++ b/docs/index.md @@ -90,3 +90,18 @@ bazel build @examples//hello_world_wasm --platforms=@rules_rust//rust/platform:w `rust_wasm_bindgen` will automatically transition to the `wasm` platform and can be used when building WebAssembly code for the host target. + +## Persistent Worker support + +The rules ship with a persistent worker implementation that uses Rust's [incremental compilation](https://doc.rust-lang.org/edition-guide/rust-2018/the-compiler/incremental-compilation-for-faster-compiles.html) for faster build times when iterating on code. + +To enable this: + +Enable the protobuf rules by adding this to your WORKSPACE + +``` +load("@rules_rust//proto:repositories.bzl", "rust_proto_repositories") +rust_proto_repositories() +``` + +In your build command, use the flag `--@rules_rust//rust:experimental-use-worker`. Optionally you can add this flag to your `.bazelrc` to always use this. From 7981f84233a92c1c16395b48ee12211a03e66d22 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 30 Mar 2021 17:53:52 -0700 Subject: [PATCH 13/18] refactor to move worker mode into function --- util/worker/worker.cc | 71 ++++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/util/worker/worker.cc b/util/worker/worker.cc index 882d6ae4ca..09e56acc2b 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -113,6 +113,41 @@ bool HandleRequest( return true; } +int RunAsWorker( + const System::StrType& exec_path, + const System::StrType& compilation_mode, + const System::EnvironmentBlock& environment_block +) { + std::unique_ptr input(new FileInputStream(0)); + std::unique_ptr output(new FileOutputStream(1)); + + while (true) { + WorkRequest request; + if (!ReadRequest(input.get(), request)) { + return 1; + } + + WorkResponse response; + if (!HandleRequest(request, response, exec_path, compilation_mode, environment_block)) { + return 1; + } + + // A CodedInputStream will try to move around the underlying buffer when destroyed. + // If we Flush stdout, that fails. So ensure it goes out of scope before we flush. + { + CodedOutputStream coded_out(output.get()); + coded_out.WriteVarint32(response.ByteSize()); + response.SerializeWithCachedSizes(&coded_out); + if (coded_out.HadError()) { + std::cerr << "Error serializing response\n"; + return 1; + } + } + output->Flush(); + } + + return 0; +} using CharType = process_wrapper::System::StrType::value_type; @@ -160,37 +195,9 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { } } - if (!as_worker) { - std::cerr << "Don't know how to run as non-worker yet\n"; - return -1; + if (as_worker) { + return RunAsWorker(exec_path, compilation_mode, environment_block); } - - std::unique_ptr input(new FileInputStream(0)); - std::unique_ptr output(new FileOutputStream(1)); - - while (true) { - WorkRequest request; - if (!ReadRequest(input.get(), request)) { - return 1; - } - - WorkResponse response; - if (!HandleRequest(request, response, exec_path, compilation_mode, environment_block)) { - return 1; - } - - // A CodedInputStream will try to move around the underlying buffer when destroyed. - // If we Flush stdout, that fails. So ensure it goes out of scope before we flush. - { - CodedOutputStream coded_out(output.get()); - coded_out.WriteVarint32(response.ByteSize()); - response.SerializeWithCachedSizes(&coded_out); - if (coded_out.HadError()) { - std::cerr << "Error serializing response\n"; - return 1; - } - } - output->Flush(); - } - return 0; + std::cerr << "Don't know how to run as non-worker yet\n"; + return -1; } From 83de9fe8ac4f04a4c9b009d7f303566370cebd98 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 30 Mar 2021 18:04:22 -0700 Subject: [PATCH 14/18] support for running standalone --- util/worker/worker.cc | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/util/worker/worker.cc b/util/worker/worker.cc index 09e56acc2b..97799fdbb5 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -149,11 +149,36 @@ int RunAsWorker( return 0; } +int RunStandalone( + const System::StrType& exec_path, + const System::EnvironmentBlock& environment_block, + const System::StrType& param_file_param +) { + if (param_file_param[0] != '@') { + std::cerr << "Param file must start with '@', got \"" << param_file_param << "\"\n"; + return -1; + } + + std::string param_file = ToUtf8(param_file_param).substr(1); + System::Arguments arguments; + + std::ifstream source(param_file); + std::string line; + while (std::getline(source, line)) { + arguments.push_back(line); + } + + std::string empty; + + return System::Exec(exec_path, arguments, environment_block, empty, empty); +} + using CharType = process_wrapper::System::StrType::value_type; int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { System::StrType exec_path; System::StrType compilation_mode; + System::StrType param_file; System::EnvironmentBlock environment_block; // Taking all environment variables from the current process // and sending them down to the child process @@ -188,6 +213,8 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { return -1; } exec_path = argv[i]; + } else if (arg[0] == '@') { + param_file = arg; } else { std::cerr << "worker wrapper error: unknown argument \"" << ToUtf8(arg) << "\"." << '\n'; @@ -196,8 +223,12 @@ int PW_MAIN(int argc, const CharType* argv[], const CharType* envp[]) { } if (as_worker) { - return RunAsWorker(exec_path, compilation_mode, environment_block); + if (!param_file.empty()) { + std::cerr << "Param file argument \"" << param_file << "\" not supported in worker mode\n"; + return -1; + } + return RunAsWorker(exec_path, compilation_mode, environment_block); + } else { + return RunStandalone(exec_path, environment_block, param_file); } - std::cerr << "Don't know how to run as non-worker yet\n"; - return -1; } From c8165c7795429ac1b2eecd00a20949f2717994b1 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 30 Mar 2021 18:05:54 -0700 Subject: [PATCH 15/18] clean up indentation --- util/worker/worker.cc | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/util/worker/worker.cc b/util/worker/worker.cc index 97799fdbb5..1453f86f3d 100644 --- a/util/worker/worker.cc +++ b/util/worker/worker.cc @@ -74,12 +74,12 @@ bool HandleRequest( auto request_args = request.arguments(); std::string target; for (int i = 0; i < request.arguments_size(); i++) { - auto argument = request.arguments(i); - // Starts with - if (argument.rfind("--target=", 0) != std::string::npos) { - target = argument.substr(9) + '/'; - } - arguments.push_back(argument); + auto argument = request.arguments(i); + // Starts with + if (argument.rfind("--target=", 0) != std::string::npos) { + target = argument.substr(9) + '/'; + } + arguments.push_back(argument); } // Considering @@ -135,13 +135,13 @@ int RunAsWorker( // A CodedInputStream will try to move around the underlying buffer when destroyed. // If we Flush stdout, that fails. So ensure it goes out of scope before we flush. { - CodedOutputStream coded_out(output.get()); - coded_out.WriteVarint32(response.ByteSize()); - response.SerializeWithCachedSizes(&coded_out); - if (coded_out.HadError()) { - std::cerr << "Error serializing response\n"; - return 1; - } + CodedOutputStream coded_out(output.get()); + coded_out.WriteVarint32(response.ByteSize()); + response.SerializeWithCachedSizes(&coded_out); + if (coded_out.HadError()) { + std::cerr << "Error serializing response\n"; + return 1; + } } output->Flush(); } @@ -165,7 +165,7 @@ int RunStandalone( std::ifstream source(param_file); std::string line; while (std::getline(source, line)) { - arguments.push_back(line); + arguments.push_back(line); } std::string empty; From cfddfb11aa75d6402785b7963584008ddb8b9f3e Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 30 Mar 2021 18:09:13 -0700 Subject: [PATCH 16/18] default to false if attr is not present on rule --- rust/private/rustc.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index 82da7f697b..1279a5d1bc 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -57,7 +57,7 @@ ErrorFormatInfo = provider( ) def _use_worker(ctx): - return ctx.attr._use_worker[BuildSettingInfo].value + return hasattr(ctx.attr, "_use_worker") and ctx.attr._use_worker[BuildSettingInfo].value def _get_rustc_env(ctx, toolchain): """Gathers rustc environment variables From 66623d76eccf226ee6702d83742d433559b32b0f Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 30 Mar 2021 18:12:58 -0700 Subject: [PATCH 17/18] buildifier fixes --- rust/private/rust.bzl | 8 ++++---- rust/private/rustc.bzl | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/rust/private/rust.bzl b/rust/private/rust.bzl index fa4539828a..a88be4edd1 100644 --- a/rust/private/rust.bzl +++ b/rust/private/rust.bzl @@ -647,14 +647,14 @@ _common_attrs = { default = "@bazel_tools//tools/cpp:current_cc_toolchain", ), "_error_format": attr.label(default = "//:error_format"), - "_process_wrapper": attr.label( - default = Label("//util/process_wrapper"), + "_persistent_worker": attr.label( + default = Label("//util/worker"), executable = True, allow_single_file = True, cfg = "exec", ), - "_persistent_worker": attr.label( - default = Label("//util/worker"), + "_process_wrapper": attr.label( + default = Label("//util/process_wrapper"), executable = True, allow_single_file = True, cfg = "exec", diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index 1279a5d1bc..f4d24a0f3d 100644 --- a/rust/private/rustc.bzl +++ b/rust/private/rustc.bzl @@ -13,6 +13,7 @@ # limitations under the License. # buildifier: disable=module-docstring +load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo") load( "@bazel_tools//tools/build_defs/cc:action_names.bzl", "CPP_LINK_EXECUTABLE_ACTION_NAME", @@ -28,7 +29,6 @@ load( "relativize", "rule_attrs", ) -load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo") BuildInfo = provider( doc = "A provider containing `rustc` build settings for a given Crate.", @@ -574,8 +574,8 @@ def rustc_compile_action( ] executable = ctx.executable._persistent_worker execution_requirements = { - "supports-workers": "1", "requires-worker-protocol": "proto", + "supports-workers": "1", } ctx.actions.run( From 8b736814c24f7ff21eda809c5f365fba2f18d1b0 Mon Sep 17 00:00:00 2001 From: Nikhil Marathe Date: Tue, 30 Mar 2021 18:40:38 -0700 Subject: [PATCH 18/18] try adding CI job --- .bazelci/presubmit.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index c57d817954..a3ecd590ba 100644 --- a/.bazelci/presubmit.yml +++ b/.bazelci/presubmit.yml @@ -78,6 +78,18 @@ tasks: working_directory: examples test_targets: - //... + examples-worker: + name: Examples Persistent Worker + platform: ubuntu1804 + working_directory: examples + build_flags: + - "--@rules_rust//rust:experimental-use-worker" + - "--worker_verbose" + test_flags: + - "--@rules_rust//rust:experimental-use-worker" + - "--worker_verbose" + test_targets: + - //... docs_linux: name: Docs platform: ubuntu1804