Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use depset instead of mutable lists #633

Merged
merged 1 commit into from
Mar 12, 2021
Merged

Conversation

hlopko
Copy link
Member

@hlopko hlopko commented Mar 12, 2021

Because of the implementation of rust_test we have to provide srcs, deps, and proc_macro_deps in CrateInfo so the test can fetch those and pass them together with its own sources and dependencies to rustc.

Currently a Rust library provides 2 providers: (1) CrateInfo describing the target, and (2) DepInfo describing its dependencies. The first thing that downstream libraries do is they get transitive depset from DepInfo, and put CrateInfo as direct.

This could be improved by putting CrateInfo into DepInfo already in the target that provides it - there would be only one depset created, not one per downstream dependency. The problem is though that for rust_test we cannot put the CrateInfo into the depset because Starlark sees it as mutable, and the cause is the use of a concatenated list. How come it worked before? Well, before the list came from a provider from a different target, so it was frozen. Now we are creating the provider, and our lists are not frozen.

I tried to lament this a bit and IMHO using depsets (which are frozen upon construction) is the best immediate step. I have a followup PR that modifies DepInfo so linking with complicated C++ dependency graphs works correctly, and that's where I encountered this problem (and that's where I extracted this PR from).

@hlopko hlopko requested a review from damienmg March 12, 2021 13:19
@google-cla google-cla bot added the cla: yes label Mar 12, 2021
@hlopko hlopko merged commit 40a8c9e into bazelbuild:main Mar 12, 2021
@hlopko hlopko deleted the add_sad_depsets branch April 1, 2021 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants