Add experimental import proc macro
#1127
Conversation
There was a problem hiding this comment.
So this is a large PR, and I have some ideas that I didn't have before (I'm sorry for all the back and forth). Would you mind doing the following?
- I think the size of this PR warrants splitting code and the tests into separate PRs:
- The first PR would only contain macro code dump, no integration, no examples
- The second PR would only contain rules integration, no examples
- The third PR would only contain examples
- That said, I think modifying all examples to use the macro may be too confusing for users, they may think this is how they should write their code, and in reality the renaming is experimental. So I'm afraid I'll suggest to implement a way of opting-in to renaming for individual targets. So between 1.2. and 1.3. I'd add:
- A PR that adds an attribute to all Rust rules named 'crate_name_format' with 3 possible values: 'target_name', 'target_label', 'default'. Default is the default, and it means whatever the build_setting sets. This is distantly similar to other 3-state flags in Bazel like dynamic_mode. This PR would also change the code in rustc.bzl to override what build_setting does when target sets the attribute to something else than default.
If the user uses this attribute on a target, they will have to make sure all targets that depend on it also set the attribute, otherwise the build will obviously fail. Alternatively we could detect this in rules implementation, but that would be a separate PR.
- A PR that adds an attribute to all Rust rules named 'crate_name_format' with 3 possible values: 'target_name', 'target_label', 'default'. Default is the default, and it means whatever the build_setting sets. This is distantly similar to other 3-state flags in Bazel like dynamic_mode. This PR would also change the code in rustc.bzl to override what build_setting does when target sets the attribute to something else than default.
- We may want to later address the need to add explicit dependency on the macro in
proc_macro_deps, but I think this is fine for now.
Does this make any sense?
| load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") | ||
|
|
||
| rust_library( | ||
| name = "mod1", |
There was a problem hiding this comment.
Nit: I was a little confused by naming the target "mod", I associate "mod" with a module, which in Rust context is a part of a single crate, not multiple crates. WDYT about using the name "lib"?
|
(I think I'm holding GitHub wrong, and can't figure out how to reply to your comment in a threaded way. But I'll reply here.)
Sounds reasonable - this PR did get bigger than I planned. I'll see if I can figure out how to wrangle my branch into several new ones, to submit PRs for the proc_macro itself, tweaking of the utils.bzl implementation, implementation for proto_library rules, and examples, all separately. (I may wait on some of those until their predecessor PRs have landed.)
Agreed, the examples probably don't need to be updated unless we want them to build with renaming turned on - but since most users probably won't use that flag, that's probably unnecessary.
I'm not sure I follow the motivation for this attribute. Can you explain? Why would a user need to rename only certain first-party targets? (This isn't necessary during rollout, since the macro is a no-op if the build setting is disabled. So an easy rollout would be to update all targets to use the macro, incrementally, and then flip the build setting.)
Agreed. Maybe as an implicit dependency of every target, similar to how cargo_build_script_runner works? Though that is an unnecessary dependency for targets that don't use the macro. Maybe we need to add another flag, to control whether every target implicitly depends on the proc_macro? Or maybe there are better solutions to this. |
Consider the situation where we have two variants of the same crate:
```
rust_library(
name = "foo",
srcs = ["foo.rs"],
)
rust_library(
name = "foo2",
crate_name = "foo",
srcs = ["foo.rs"],
)
```
A use case for this is for example when we want to define different variants
of a crate using different feature sets, e.g., define a rustc_private non-std
variant of a vendored crate of the standard library together with a more
standard version of the vendored crate that requires the standard library.
Currently, this is a fragile definition since the output rlib filename of both
of these is the same, e.g., `libfoo-$HASH.rlib`, where `$HASH` is
derived from the crate root source filename, in this case `foo.rs`. So whenever
we have a Bazel query that includes both of these, the build may fail with
errors like:
```
ERROR: file 'test/unit/crate_variants/libfoo-717083168.rlib' is generated by these conflicting actions:
Label: //test/unit/crate_variants:foo2, //test/unit/crate_variants:foo
```
This patch reduces the risk of such output filename collisions by mixing in
the rule label in the output hash calculation.
…1073) When building on an Apple Silicon machine, a different target triple is required to target the simulator: https://doc.rust-lang.org/rustc/platform-support/aarch64-apple-ios-sim.html As rules_rust currently ignores abi in triples, listing device and simulator triples in a rust_repository_set() ends up always using the first one. Adding this constraint allows a build to be switched between targeting a device and targeting the simulator.
…azelbuild#1089) * Fix proc_macro_dylib_path in cases where they are built in both opt and debug * Resolve rustfmt issues * Update test comment
* optimization: switch fastbuild opt default to 1 from 0 This will very slightly increase build times but applies basic optimizations that will do things like make iterators _much_ faster. Since fastbuild is how people iterate most of the time, this seems like a sensible default value. * Regenerate documentation Co-authored-by: Augie Fackler <augie@google.com>
This variable is set for the default profiles as described in https://doc.rust-lang.org/cargo/reference/profiles.html -- true for dev and false for release. This is an environment variable cargo sets for build scripts documented here https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts
* Added Rust 1.58.1 * Regenerate documentation
* Updated rust_test docs * Regenerate documentation
…bazelbuild#1101) * Allow users to configure the timeout of `cargo_bootstrap_repository`. * Regenerate documentation
…azelbuild#1104) * Made components of `rustc` bundles mandatory for `rust_toolchain`. * Regenerate documentation
bazelbuild#1100) * Allow cargo_bootstrap_repository to specify rustc and cargo separately * Regenerate documentation
* Fixed typo * Regenerate documentation
…azelbuild#1109) * Replace lists of files and Targets in `rust_toolchain` with depsets * Use `find_sysroot` helper. * Simply use `rust_std` files directly * Revert "Use `find_sysroot` helper." This reverts commit 914d159. * files
…1115) * Calculate more things in the toolchain construction * Update sysroot logic
We use the equivalent `toolchain._cc_toolchain` attribute instead
cfredric
force-pushed
the
import_macro
branch
from
66118f1
to
ef189fb
Compare
|
Closing this now, as these changes were split apart and merged as #1142, #1143, #1145, and #1151. Not included in those PRs was:
|
This PR implements the experimental
importmacro discussed in #1008 and #1013. This macro is the compiler-side of the changes required to do this renaming, so that the compiler looks for the crate using the correct name.The macro reads two environment variables, in order to be aware of whether renaming is turned on for this repository, and if so, which directory is considered "third-party" (and therefore should not be renamed).
This PR also contains a bugfix or two for the Bazel side of this flag - I made the renaming itself slightly more efficient, fixed a backward boolean, and made
rust_proto_libraryparticipate in renaming as well.All tests in @rules_rust pass with and without renaming enabled.
I also updated the
examplesworkspace to contain some test cases for renaming, and modified its other packages so that they build/pass with and without renaming enabled. The exception to this is the two rust_doc[_test] targets. rust_doc and rust_doc_test do not currently support aproc_macro_depsattribute (nor adepsattribute), so it is currently not possible to use the macro in a doc comment. So, these targets break if renaming is enabled, at the moment.