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

The compile_data attribute can now be gathered from dependencies #814

Merged
merged 8 commits into from
Jul 20, 2021

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Jun 30, 2021

The compile_data attribute seems like it should propagate all the way through to the final build outputs since it'd be required by wrapper rules (rust_test) to be able to successfully compile.

closes #567

@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Jun 30, 2021

Not sure if this is an expensive solution but I think it's fine? Hopefully reviewers will have some insight 😅

@UebelAndre UebelAndre marked this pull request as ready for review June 30, 2021 01:32
@hlopko
Copy link
Member

hlopko commented Jun 30, 2021

Do we really need to propagate compile_data all the way to the binary? Do you have a real world example? AFAIU (and it seems docs https://bazelbuild.github.io/rules_rust/defs.html#rust_library-compile_data suggest the same) compile data are only meant to be used to build the library, in other words they are used by rustc when producing the rlib.

Adding wrapped_crate_compile_data into CrateInfo that allows rust_test and other wrapper rules to recreate the original rustc invocation makes total sense and it is a good improvement.

@UebelAndre
Copy link
Collaborator Author

Do we really need to propagate compile_data all the way to the binary? Do you have a real world example? AFAIU (and it seems docs https://bazelbuild.github.io/rules_rust/defs.html#rust_library-compile_data suggest the same) compile data are only meant to be used to build the library, in other words they are used by rustc when producing the rlib.

It's also used in binaries but maybe I misread this as a suggestion that it's only used by libraries.

Adding wrapped_crate_compile_data into CrateInfo that allows rust_test and other wrapper rules to recreate the original rustc invocation makes total sense and it is a good improvement.

The issue is that at the time it's the responsibility of the crate that's given compile_data to forward it. Once we're in a wrapper rule's implementation, we no longer have access to that attribute if it wasn't put in a provider somewhere. It could definitely be the case that we don't want compile data to propagate always and to actually restrict it to wrapper rules, but I wasn't sure.

How would I populate wrapped_crate_compile_data only for wrapped targets?

@UebelAndre
Copy link
Collaborator Author

Additionally, I do think it might be valuable to be able to transitively provide data without having it end up in the runfiles for runtime.

@hlopko
Copy link
Member

hlopko commented Jun 30, 2021

It's also used in binaries but maybe I misread this as a suggestion that it's only used by libraries.

Yup it is used in binaries too for the same reason it is in libraries - to make a dependency available during the rustc invocation, not to be propagated.

The issue is that at the time it's the responsibility of the crate that's given compile_data to forward it. Once we're in a wrapper rule's implementation, we no longer have access to that attribute if it wasn't put in a provider somewhere. It could definitely be the case that we don't want compile data to propagate always and to actually restrict it to wrapper rules, but I wasn't sure.

Oh right that is true, we have to put compile data into the provider in case there is a wrapper rule. But I would still only put the compile data of the current target, not transitive ones.

Additionally, I do think it might be valuable to be able to transitively provide data without having it end up in the runfiles for runtime.

I'd be interested to learn about an actual, real world use case for this. And even then, I think it would make sense to create a different attribute for transitive propagation. So target author can decide which behavior matches their use case. Propagating by default can end up being too memory costly in large workspaces.

@UebelAndre
Copy link
Collaborator Author

I'd be interested to learn about an actual, real world use case for this. And even then, I think it would make sense to create a different attribute for transitive propagation. So target author can decide which behavior matches their use case. Propagating by default can end up being too memory costly in large workspaces.

I don't think I have a reasonable real-world use case. I've removed the use of transitive compile-data. I think this covers the practical use case now.

Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you! Only a kind unit test nerd snipe attempt remaining :)

test/compile_data/compile_data.rs Outdated Show resolved Hide resolved
rust/private/rust.bzl Show resolved Hide resolved
@UebelAndre UebelAndre requested a review from hlopko July 15, 2021 13:50
Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much! Looks great.

@UebelAndre UebelAndre merged commit e10ab21 into bazelbuild:main Jul 20, 2021
@UebelAndre UebelAndre deleted the data branch July 30, 2021 13:53
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.

compile_data does not propagate to rust_test targets
2 participants