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 the ability to pass environment variables to rustc #315

Closed
wants to merge 1 commit into from

Conversation

smklein
Copy link
Contributor

@smklein smklein commented May 8, 2020

This patch enables callers to supply an additional
parameter to rust targets (such as "rust_library", "rust_binary",
etc):

rustc_env = [
"KEY=VALUE",
]

This parameter parses the key/value pairs, and appends them
to the list of environment variables supplied to rustc.

This parameter emulated the impact of "cargo:rustc-env=VAR=VALUE"
lines, which are common in build scripts.

This patch enables callers to supply an additional
parameter to rust targets (such as "rust_library", "rust_binary",
etc):

  rustc_env = [
    "KEY=VALUE",
  ]

This parameter parses the key/value pairs, and appends them
to the list of environment variables supplied to rustc.

This parameter emulated the impact of "cargo:rustc-env=VAR=VALUE"
lines, which are common in build scripts.
@smklein
Copy link
Contributor Author

smklein commented May 8, 2020

For context, I've been trying to add an integration test of https://docs.rs/prost-build/0.6.1/prost_build/ into cargo-raze.

In this process, I've uncovered that the build script for this crate really wants to pass PROTOC as an environment variable: https://docs.rs/prost-build/0.6.1/prost_build/#sourcing-protoc

It can be supplied as an argument to the build script (which itself may or may not be enabled, depending on gen_buildrs)... but that build script turns around and emits a "cargo:rustc-env=VAR=VALUE" line, which raze + rules_rust ignores!

In the crate itself, the "env!" command is used (just like in the test added within this patch), and it refuses to compile without a compile-time value.

error: environment variable `PROTOC` not defined
   --> vendored/codegen_cargo_library/cargo/vendor/prost-build-0.6.1/src/lib.rs:680:15
    |
680 |     Path::new(env!("PROTOC"))
    |               ^^^^^^^^^^^^^^

error: environment variable `PROTOC_INCLUDE` not defined
   --> vendored/codegen_cargo_library/cargo/vendor/prost-build-0.6.1/src/lib.rs:685:15
    |
685 |     Path::new(env!("PROTOC_INCLUDE"))
    |               ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

Personally, I think this is a value which should be parsed at runtime, not compile-time (and my colleagues have patches to prost-build pushing in this direction).

Regardless, it's inevitable that some crates will have "env!" macros, and this patch provides a mechanism for compiling them with an arbitrary env value.

@smklein
Copy link
Contributor Author

smklein commented May 8, 2020

Welp, I feel quite silly, I just realized #314 exists, and somehow forgot about it. Happy to abandon this patch in favor of that one, unless ya'll like string lists more than dicts in this context.

Copy link
Contributor

@tommilligan tommilligan left a comment

Choose a reason for hiding this comment

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

Hah, no worries. It's nice to see it's not just me that's run into this issue! And hilarious that we chose the exact same attribute name.

I do prefer the dictionary style as it avoids any issues with parsing, and it just ends up being a smaller diff.

# Add any additional explicitly passed environment variables.
extra_envs = []
for kv in getattr(ctx.attr, "rustc_env", []):
k, v = kv.split("=")
Copy link
Contributor

@tommilligan tommilligan May 10, 2020

Choose a reason for hiding this comment

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

This will fail for values that contain an = character, such as A=B=C, which should be parsed to ["A", "B=C"]. This could be fixed with

k, v = kv.split("=", 1)

@damienmg
Copy link
Collaborator

I would also prefer dict so PR #314 over this one.

@smklein smklein closed this May 11, 2020
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

4 participants