Refactor CrateInfo construction #2161
Merged
+279
−182
Conversation
This file contains 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
daivinhtran
force-pushed
the
refactor-crate_info-construction
branch
from
7465ff7 to
6d5f62a
Compare
dzbarsky
reviewed
Sep 18, 2023
rust/private/rustc.bzl
Outdated
| if getattr(attr, "edition"): | ||
| return attr.edition | ||
| elif not toolchain.default_edition: | ||
| fail("Attribute `edition` is required for {}.".format(label)) |
There was a problem hiding this comment.
Preexisting, but the first time I hit this error it wasn't obvious that you could set a default edition in the toolchain, might be worth a mention in this error?
There was a problem hiding this comment.
I'm leaving your comments unresolved in this PR and will address in a separate PR because they're not within scope.
rust/private/rustc.bzl
Outdated
| """Creates symlinks of the source files if needed. | ||
|
|
||
| Rustc assumes that the source files are located next to the crate root. | ||
| In case of a mix between generated and non-generated source files, this |
There was a problem hiding this comment.
I think we want to snip "this" at the end of this line?
There was a problem hiding this comment.
I'm leaving your comments unresolved in this PR and will address in a separate PR because they're not within scope.
rust/private/rustc.bzl
Outdated
| Returns: | ||
| Tuple(List[File], File): The transformed srcs and crate_root | ||
| """ | ||
| has_generated_sources = len([src for src in srcs if not src.is_source]) > 0 |
There was a problem hiding this comment.
nit: any([src for src in srcs if not src.is_source]) is shorter and you can bail out a little quicker?
There was a problem hiding this comment.
I'm leaving your comments unresolved in this PR and will address in a separate PR because they're not within scope.
|
Hi @dzbarsky . Thanks for the review! It's still WIP and I just marked the PR as draft now while I'm working to fix the issue with |
daivinhtran
force-pushed
the
refactor-crate_info-construction
branch
2 times, most recently
from
9db35ca to
c07f822
Compare
added 8 commits
September 22, 2023 15:23
daivinhtran
force-pushed
the
refactor-crate_info-construction
branch
from
c07f822 to
b07102f
Compare
daivinhtran
force-pushed
the
refactor-crate_info-construction
branch
11 times, most recently
from
4fd87dd to
0792154
Compare
daivinhtran
force-pushed
the
refactor-crate_info-construction
branch
from
0792154 to
1d6dbe4
Compare
daivinhtran
force-pushed
the
refactor-crate_info-construction
branch
3 times, most recently
from
790efa4 to
ab05446
Compare
daivinhtran
force-pushed
the
refactor-crate_info-construction
branch
from
ab05446 to
e138ec9
Compare
scentini
added a commit
that referenced
this pull request
Oct 17, 2023
In #2161, I anticipated to make [create_crate_info_dict](https://github.com/bazelbuild/rules_rust/pull/2161/files#diff-b75adb75969318e09670c45b8eea5aaf147fdc1d2cf229a72a17bd39edf49163R853) reusable across all rules and aspects. The logic to construct `crate_info` is significantly different among the rules and aspects. As I tried to to expand the arguments to make `create_crate_info_dict` universally reusable, the arguments grow which make it not worth anymore. This PR changes the refactoring approach. Before (result from #2161) ``` def _rust_library_common() return rustc_compile_action( # the callback allows us to slowly refactor one rule or aspect at a time. create_crate_info_callback = create_crate_info_dict, ) def rustc_compile_action(create_crate_info_callback) if create_crate_info_callback: crate_info_dict = create_crate_info_callback() ``` After (result from this PR) ``` def _rust_library_common() # create crate_info_dict return rustc_compile_action( crate_info_dict = crate_info_dict, ) def rustc_compile_action(crate_info_dict) # crate_info_dict always exists ``` Instead of creating `crate_info_dict` at the beginning of `rustc_compile_action`, this PR lifted the creation logic to the rules and aspects completely so that they can add any custom logic without caring about the `rustc_compile_action`'s implementation. --------- Co-authored-by: scentini <[email protected]>
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.
Description
This PR addresses #2013 to ensure rust-analyzer can access all the env vars the rust rules pass to
rustcat compile time. This is a large refactoring in almost all the rules and aspects so I aim this PR to just fixrust_libraryrule. The follow-up PR will address the remaining rules and aspects.Summary
create_crate_info_dictfunction inrust/private/utils.bzlto create a mutable dict repsenting CrateInfo_determine_lib_name,get_edition,_transform_sourcesfunctions torust/private/utils.bzlto avoid cyclic dependency whencreate_crate_info_dictfunction use thesecreate_crate_info_callbackattribute torustc_compile_actionto allow creatingCrateInfoinsiderustc_compile_action. This optional attribute allows scoping the refactoring in this PR to justrust_libraryrule.skip_expanding_rustc_envattribute torustc_compile_actionto skip expandingrustc_envattr. This is useful forclippyaspect andrust_testrule because theCrateInfoprovided from the dependedrust_libraryalready expands all the env vars before returning the provider downstream.Notes
rustc_env_attris a temporary field inCrateInfoand needed by the rules and aspects not migrated to createCrateInfoinrustc_compile_actionyet. It will be remove in the next PR afterCrateInfo.rustc_envis always expanded.create_crate_info_callbackwill be removed fromrustc_compile_actionafter allCrateInfos are created insiderustc_compile_action.