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

add compile_data argument for data required at compile time #516

Merged
merged 3 commits into from
Dec 12, 2020

Conversation

dae
Copy link
Contributor

@dae dae commented Dec 1, 2020

Using the existing data argument for build-time data dependencies causes the files to also show up in the runfiles. This can be seen when using rules_docker's rust_image() for example - the files are included in the image twice; once in the binary and once in their original form.

If this is accepted I'll send through a PR to cargo-raze to expose this for third-party crates similar to data_attr.

Closes #222

@google-cla google-cla bot added the cla: yes label Dec 1, 2020
@UebelAndre
Copy link
Collaborator

UebelAndre commented Dec 1, 2020

Nit: maybe build_data might be a more accurate description of the functionality here? Data that needs to be available for a build (compile and/or linker commands)

@dae
Copy link
Contributor Author

dae commented Dec 1, 2020

Build was my first choice actually, but I thought it might be confused with the cargo build script functionality, and the tool we're calling is 'rustc' rather than 'rustb' :-)

@UebelAndre
Copy link
Collaborator

I think because in these rules https://github.com/bazelbuild/rules_rust/blob/master/cargo/cargo_build_script.bzl handles the build.rs stuff that the new variable would still make sense as build_data with a little bit of clarifying points in the doc field. Your thoughts there?

@dae
Copy link
Contributor Author

dae commented Dec 1, 2020

I don't feel strongly about it, but think compile might be the slightly better choice - there are various direct and indirect (eg rustc_flags) references to compiling already, and build feels more like it's talking about the building process as a whole more than a single step.

@UebelAndre
Copy link
Collaborator

Maybe rustc_data? I still like build_data but maybe I'm incorrectly thinking about the linker step as something larger than it is (more like C++) or something. But I still don't feel compile_data is as clear as it could be.

@dae
Copy link
Contributor Author

dae commented Dec 1, 2020

Maybe someone else can chime in with their thoughts?

I went looking for precedent, but there doesn't appear to be much. The Java/Scala rules use 'resources' which I presume is a Javarism, and Go has a separate rule for embedding data rather than an argument to an existing rule.

@UebelAndre
Copy link
Collaborator

Yeah, more thoughts sound good. And yeah, I think Java (gradle?) has it's own terminology.

dae added a commit to ankitects/cargo-raze that referenced this pull request Dec 1, 2020
Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, and I don't think I have a better name than compile_data. I guess the other direction to go could be to describe what it's not, i.e. something like intransitive_data or non_propagated_data, but I don't think I have any particularly pithy ideas.

@mfarrugi @dfreese WDYT?

@dfreese
Copy link
Collaborator

dfreese commented Dec 1, 2020

Generally agree with the feature being necessary, so that part looks good to me.

In terms of the name, the best reference I would have for this is textual_hdrs, so I would lean towards textual_srcs.

That would indicate to me:

  1. The files must be textually included by other source files to build valid code
  2. Are, like srcs for cc_library targets, are not visible to other targets.

@UebelAndre
Copy link
Collaborator

Generally agree with the feature being necessary, so that part looks good to me.

In terms of the name, the best reference I would have for this is textual_hdrs, so I would lean towards textual_srcs.

That would indicate to me:

1. The files must be textually included by other source files to build valid code

2. Are, like srcs for cc_library targets, are not visible to other targets.

This seems less discoverable than compile_data to me and inconsistent with the existing data field. If we went with a name like that then I think we should consider renaming it as well.

@dae
Copy link
Contributor Author

dae commented Dec 1, 2020

I find textual_data a bit confusing - it makes me think of text-based data , rather than data being referenced by source code. The user may want to include binary data with include_bytes!(), and textual_data may not be obvious in that case.

textual_srcs also seems to be inconsistent with srcs, as only .rs files are accepted for the latter, and this feature would be useful for more than just include!()

@illicitonion
Copy link
Collaborator

I agree that textual_ is a non-obvious prefix, though I can see where it's coming from... Some other thoughts...

rustc_srcs (distinct from srcs so hopefully not confused with it, and follows the pattern of rustc_env being the env vars to give rustc)
other_compile_inputs or non_src_compile_inputs or compile_inputs?

Though honestly I think I'm happy enough with compile_data - I'm not convinced we're going to find the perfect name here :)

@dfreese
Copy link
Collaborator

dfreese commented Dec 2, 2020

compile_data seems clear enough.

@dae
Copy link
Contributor Author

dae commented Dec 10, 2020

I've rebased this on master, and the subsequent change to cargo raze is waiting over on google/cargo-raze#308

@illicitonion illicitonion merged commit eef7f56 into bazelbuild:master Dec 12, 2020
dae added a commit to ankitects/cargo-raze that referenced this pull request Dec 12, 2020
dae added a commit to ankitects/cargo-raze that referenced this pull request Dec 12, 2020
dfreese pushed a commit to google/cargo-raze that referenced this pull request Dec 12, 2020
@rdelfin
Copy link

rdelfin commented Mar 25, 2021

Are there any examples on how this would get practically used? I'm currently trying to include the output of bindgen, passing it through rustfmt_generator and including that in the compile_data of a library that does an include!() on the result. However, by the looks of it the OUT_DIR env var doesn't exist and using just the plain filename name doesn't do much. By the looks of it this doesn't add any env vars to specify where these compile data fields are but I might be missing something. What's the right approach here? I'd be glad to send a PR with documentation or an example

@illicitonion
Copy link
Collaborator

There are some examples in https://github.com/bazelbuild/rules_rust/blob/dd7e458de8d6921f274ee91664b326460d5d9d4c/examples/env_locations/BUILD - in short, you need to set an environment variable using the rustc_env attribute, and use an $(execpath ...) macro when setting its value, and then you can include!(env!(...)) via the env var. All rules in this repo should take a rustc_env attribute , and the mechanism works the same across them all :)

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.

Add compile_data to rust rules
5 participants