From f516c27517456a00ae352cf2d191533fd7a75c7d Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Fri, 9 Apr 2021 11:43:05 +1000 Subject: [PATCH] incremental compilation without a worker This is considerably less invasive than #667, #517 and #421 - there is no extra code to bootstrap, and no toolchains have to be plumbed into the rules. We had been using a worker to get around sandboxing restrictions, but as @hlopko pointed out on #667, that turns out not to be necessary - even normal rules have access to /tmp. Unfortunately it seems that rustc does not expect the source files to change location, and it will consistently crash when used in a sandboxed context. So the approach this PR takes is to disable sandboxing on targets that are being compiled incrementally. This fixes the compiler crashes, and as a bonus, means we're not limited to saving the cache in /tmp. This PR adds a --@rules_rust//:experimental_incremental_base flag to specify the path where incremental build products are stored - if not provided, the rules will function as they normally do. The default behaviour will incrementally compile crates in the local workspace, like cargo does. The behaviour can be adjusted with another flag, which is covered in docs/index.md. --- BUILD.bazel | 20 ++++++- docs/BUILD.bazel | 1 + docs/index.md | 25 +++++++++ rust/defs.bzl | 8 +++ rust/private/rust.bzl | 26 +++++++++- rust/private/rustc.bzl | 115 +++++++++++++++++++++++++++++++++++++++-- 6 files changed, 190 insertions(+), 5 deletions(-) diff --git a/BUILD.bazel b/BUILD.bazel index feb93a2b9f..ba18e7c7f8 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -1,5 +1,10 @@ load("@bazel_skylib//:bzl_library.bzl", "bzl_library") -load("//rust:rust.bzl", "error_format") +load( + "//rust:defs.bzl", + "error_format", + "experimental_incremental_base", + "experimental_incremental_prefixes", +) exports_files(["LICENSE"]) @@ -17,3 +22,16 @@ error_format( build_setting_default = "human", visibility = ["//visibility:public"], ) + +# Optional incremental compilation - see docs/index.md +experimental_incremental_base( + name = "experimental_incremental_base", + build_setting_default = "", + visibility = ["//visibility:public"], +) + +experimental_incremental_prefixes( + name = "experimental_incremental_prefixes", + build_setting_default = "//,-//vendored//", + visibility = ["//visibility:public"], +) diff --git a/docs/BUILD.bazel b/docs/BUILD.bazel index 712587c1fb..c3e4492f6d 100644 --- a/docs/BUILD.bazel +++ b/docs/BUILD.bazel @@ -6,6 +6,7 @@ package(default_visibility = ["//visibility:private"]) bzl_library( name = "docs_deps", srcs = [ + "@bazel_skylib//rules:common_settings", "@bazel_tools//tools:bzl_srcs", "@build_bazel_rules_nodejs//internal/providers:bzl", ], diff --git a/docs/index.md b/docs/index.md index 8227e4ec44..4cf214afa2 100644 --- a/docs/index.md +++ b/docs/index.md @@ -90,3 +90,28 @@ 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. + +## Incremental Compilation + +There is an experimental flag that enables incremental compilation, which can +considerably speed up rebuilds during development. + +Targets built with incremental compilation enabled will have sandboxing +disabled, so enabling this option is trading off some of Bazel's hermeticity in +the name of performance. This option is not recommended for CI or release +builds. + +To enable incremental compilation, add the following argument to your bazel +build command, adjusting the directory path to one that suits: + + --@rules_rust//:experimental_incremental_base=/home/user/cache/bazel_incremental + +A separate flag allows you to control which crates are incrementally compiled. +The default is: + + --@rules_rust//:experimental_incremental_prefixes=//,-//vendored + +This will incrementally compile any crates in the local workspace, but exclude +other repositories like `@raze__*`, and anything in the //vendored package. This +behaviour is similar to cargo, which only incrementally compiles the local +workspace. diff --git a/rust/defs.bzl b/rust/defs.bzl index da664323a3..53e9c4df94 100644 --- a/rust/defs.bzl +++ b/rust/defs.bzl @@ -39,6 +39,8 @@ load( load( "//rust/private:rustc.bzl", _error_format = "error_format", + _incremental_base = "incremental_base", + _incremental_prefixes = "incremental_prefixes", ) load( "//rust/private:rustdoc.bzl", @@ -88,6 +90,12 @@ rust_clippy = _rust_clippy error_format = _error_format # See @rules_rust//rust/private:rustc.bzl for a complete description. +experimental_incremental_base = _incremental_base +# See @rules_rust//rust/private:rustc.bzl for a complete description. + +experimental_incremental_prefixes = _incremental_prefixes +# See @rules_rust//rust/private:rustc.bzl for a complete description. + rust_common = _rust_common # See @rules_rust//rust/private:common.bzl for a complete description. diff --git a/rust/private/rust.bzl b/rust/private/rust.bzl index 0101c707bf..f863bded35 100644 --- a/rust/private/rust.bzl +++ b/rust/private/rust.bzl @@ -13,8 +13,9 @@ # limitations under the License. # buildifier: disable=module-docstring +load("@bazel_skylib//rules:common_settings.bzl", "BuildSettingInfo") load("//rust/private:common.bzl", "rust_common") -load("//rust/private:rustc.bzl", "rustc_compile_action") +load("//rust/private:rustc.bzl", "IncrementalInfo", "IncrementalPrefixInfo", "rustc_compile_action") load("//rust/private:utils.bzl", "crate_name_from_attr", "determine_output_hash", "expand_locations", "find_toolchain") # TODO(marco): Separate each rule into its own file. @@ -98,6 +99,24 @@ def _determine_lib_name(name, crate_type, toolchain, lib_hash = ""): extension = extension, ) +def incremental_info(attr): + """Extract incremental compilation info from common rule attributes. + + Expects to find the following attributes: + + _incremental_base + _incremental_prefixes + + Returns: + IncrementalInfo: incremental compilation configuration + """ + prefixes = attr._incremental_prefixes[IncrementalPrefixInfo] + base = attr._incremental_base[BuildSettingInfo].value + return IncrementalInfo( + prefixes = prefixes, + base = base, + ) + def get_edition(attr, toolchain): """Returns the Rust edition from either the current rule's attirbutes or the current `rust_toolchain` @@ -263,6 +282,7 @@ def _rust_library_common(ctx, crate_type): is_test = False, ), output_hash = output_hash, + incremental_info = incremental_info(ctx.attr), ) def _rust_binary_impl(ctx): @@ -297,6 +317,7 @@ def _rust_binary_impl(ctx): rustc_env = ctx.attr.rustc_env, is_test = False, ), + incremental_info = incremental_info(ctx.attr), ) def _create_test_launcher(ctx, toolchain, output, providers): @@ -434,6 +455,7 @@ def _rust_test_common(ctx, toolchain, output): crate_type = crate_type, crate_info = crate_info, rust_flags = ["--test"], + incremental_info = incremental_info(ctx.attr), ) return _create_test_launcher(ctx, toolchain, output, providers) @@ -673,6 +695,8 @@ _common_attrs = { default = "@bazel_tools//tools/cpp:current_cc_toolchain", ), "_error_format": attr.label(default = "//:error_format"), + "_incremental_base": attr.label(default = "//:experimental_incremental_base"), + "_incremental_prefixes": attr.label(default = "//:experimental_incremental_prefixes"), "_process_wrapper": attr.label( default = Label("//util/process_wrapper"), executable = True, diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl index b73ba37209..99eaf227f1 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", @@ -47,6 +48,22 @@ AliasableDepInfo = provider( }, ) +IncrementalInfo = provider( + doc = "Data relating to incremental compilation.", + fields = { + "base": "string: base folder to store incremental build products", + "prefixes": "IncrementalPrefixInfo: prefixes to include and exclude", + }, +) + +IncrementalPrefixInfo = provider( + doc = "Prefixes to include and exclude in incremental compilation.", + fields = { + "exclude": "List[string]: prefixes that will exclude a label if matched", + "include": "List[string]: prefixes that will include a label if matched", + }, +) + _error_format_values = ["human", "json", "short"] ErrorFormatInfo = provider( @@ -492,7 +509,8 @@ def rustc_compile_action( crate_info, output_hash = None, rust_flags = [], - environ = {}): + environ = {}, + incremental_info = None): """Create and run a rustc compile action based on the current rule's attributes Args: @@ -503,6 +521,7 @@ def rustc_compile_action( output_hash (str, optional): The hashed path of the crate root. Defaults to None. rust_flags (list, optional): Additional flags to pass to rustc. Defaults to []. environ (dict, optional): A set of makefile expandable environment variables for the action + incremental_info (str, optional): path to store incremental build products in. Returns: list: A list of the following providers: @@ -554,14 +573,34 @@ def rustc_compile_action( else: formatted_version = "" + if (incremental_info and + incremental_info.base and + _want_incremental_compile(ctx.label, incremental_info.prefixes)): + incremental_dir = "{}/{}_{}".format( + incremental_base, + ctx.var["COMPILATION_MODE"], + toolchain.target_triple, + ) + args.add("--codegen", "incremental=" + incremental_dir) + + # with sandboxing enabled, subsequent rustc invocations will crash, + # as it doesn't expect the source files to have moved + execution_requirements = {"no-sandbox": "1"} + mnemonic = "RustIncr" + else: + execution_requirements = {} + mnemonic = "Rustc" + ctx.actions.run( executable = ctx.executable._process_wrapper, inputs = compile_inputs, outputs = [crate_info.output], env = env, arguments = [args], - mnemonic = "Rustc", - progress_message = "Compiling Rust {} {}{} ({} files)".format( + mnemonic = mnemonic, + execution_requirements = execution_requirements, + progress_message = "Compiling {} {} {}{} ({} files)".format( + mnemonic, crate_info.type, ctx.label.name, formatted_version, @@ -934,3 +973,73 @@ error_format = rule( implementation = _error_format_impl, build_setting = config.string(flag = True), ) + +def _incremental_base_impl(ctx): + """Implementation for the incremental_base() rule + + Args: + ctx (ctx): The rule's context object + + Returns: + BuildSettingInfo: an object with a `value` attribute containing the string. + """ + value = ctx.build_setting_value + return BuildSettingInfo(value = value) + +incremental_base = rule( + build_setting = config.string(flag = True), + implementation = _incremental_base_impl, + doc = "Declares a command line argument that accepts an arbitrary string.", +) + +def _incremental_prefixes_impl(ctx): + """Implementation for the incremental_prefixes_flag() rule + + Splits the provided string on commas, and then partitions prefixes starting + with a hypen into the exclude list, returning a provider with the include + and exclude list. The hypens are stripped from the entries in the exclude list. + + Args: + ctx (ctx): The rule's context object + + Returns: + (IncrementalPrefixInfo): a list of prefixes to include and exclude + """ + values = ctx.build_setting_value.split(",") + include = [] + exclude = [] + for value in ctx.build_setting_value.split(","): + if value.startswith("-"): + exclude.append(value[1:]) + else: + include.append(value) + return IncrementalPrefixInfo(include = include, exclude = exclude) + +incremental_prefixes = rule( + build_setting = config.string(flag = True), + implementation = _incremental_prefixes_impl, + doc = """Declares a command line argument for incremental prefixes. + + See _incremental_prefixes_impl() for the details. + """, +) + +def _want_incremental_compile(label, prefixes): + """True if the provided prefixes indicate the target should be incrementally compiled. + + Args: + label (Label): the label for a given target + prefixes (IncrementalPrefixInfo): prefixes to include and exclude + + Returns: + bool + """ + label = str(label) + for prefix in prefixes.exclude: + if label.startswith(prefix): + return False + for prefix in prefixes.include: + if label.startswith(prefix): + return True + + return False