-
Notifications
You must be signed in to change notification settings - Fork 402
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
pass compile environment into build script #426
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have a recommendation to add a TODO.
Thanks a lot for all your changes, hopefully I can get to them all now.
cargo/cargo_build_script.bzl
Outdated
@@ -52,6 +52,8 @@ def _cargo_build_script_run(ctx, script): | |||
# Pull in env vars which may be required for the cc_toolchain to work (e.g. on OSX, the SDK version). | |||
# We hope that the linker env is sufficient for the whole cc_toolchain. | |||
cc_toolchain, feature_configuration = get_cc_toolchain(ctx) | |||
cc_env = get_cc_compile_env(cc_toolchain, feature_configuration) | |||
env.update(**cc_env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure either this is the best solution but I think we are better with it than without.
env.update(**cc_env) | |
# TODO: Trim down the list of environment shipped. | |
env.update(**cc_env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'd prefer to keep env changes to a minimum, it seems as if INCLUDE is sufficient for the CC-using crates in my project to compile:
diff --git a/cargo/cargo_build_script.bzl b/cargo/cargo_build_script.bzl
index 0725c4c..a13826b 100644
--- a/cargo/cargo_build_script.bzl
+++ b/cargo/cargo_build_script.bzl
@@ -53,7 +53,9 @@ def _cargo_build_script_run(ctx, script):
# We hope that the linker env is sufficient for the whole cc_toolchain.
cc_toolchain, feature_configuration = get_cc_toolchain(ctx)
cc_env = get_cc_compile_env(cc_toolchain, feature_configuration)
- env.update(**cc_env)
+ includes = cc_env.get("INCLUDE")
+ if includes:
+ env["INCLUDE"] = includes
_, _, linker_env = get_linker_and_args(ctx, cc_toolchain, feature_configuration, None)
env.update(**linker_env)
Would you like me to push this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tentatively made this change and rebased on master.
On Windows, MSVC requires INCLUDE to be set to the path of the standard include files. Not sure if this is the best solution, but it seems to allow a build to proceed in my case.
Thanks that's perfect like this. |
On Windows, MSVC requires INCLUDE to be set to the path of the standard
include files. Not sure if this is the best solution, but it solved issues with
missing includes mentioned in #423 for me.