-
Notifications
You must be signed in to change notification settings - Fork 406
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
Adjust env for bindgen compilation. #916
Adjust env for bindgen compilation. #916
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.
Thanks!
@@ -126,6 +129,9 @@ def _rust_bindgen_impl(ctx): | |||
"LIBCLANG_PATH": libclang_dir, | |||
"RUST_BACKTRACE": "1", | |||
} | |||
cc_toolchain, feature_configuration = find_cc_toolchain(ctx) | |||
_, _, linker_env = get_linker_and_args(ctx, ctx.attr, cc_toolchain, feature_configuration, None) | |||
env.update(**linker_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.
Does this need any special handling in the case of clashes with things we explicitly set? e.g. should we be prepending/appending to the LD_LIBRARY_PATH
rather than overwriting it?
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.
There's that issue, and whether the the system include directories from the cc_toolchain should be added. I'm inclined to leave it as I've written it, and adjust if people have problems with it, since this patch is clearly a step forward.
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.
Yeah, sounds reasonable - let's see how we go!
Can you push a rebase/merge-main, and I'll merge to main? :) |
We should have the user in #899 contribute their test repo as an example, to get some coverage on this. |
This fixes #899 by putting
XCODE_VERSION_OVERRIDE
,APPLE_SDK_VERSION_OVERRIDE
,APPLE_SDK_PLATFORM
in the env.