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

Respect changes to BINDGEN_EXTRA_CLANG_ARGS #1813

Merged
merged 1 commit into from
Jun 22, 2020
Merged

Conversation

kulp
Copy link
Member

@kulp kulp commented Jun 22, 2020

This is a fix for a tiny edge case, in the spirit of #1766, to support the flag added in #1537.

I looked for other env! and env::var instances in bindgen itself and did not find anything else to change.

@emilio emilio merged commit 939959a into rust-lang:master Jun 22, 2020
@kulp kulp deleted the extra-args branch June 22, 2020 11:36
@tmfink
Copy link
Contributor

tmfink commented Jun 25, 2020

@kulp I'm confused why we need this change.
I thought that BINDGEN_EXTRA_CLANG_ARGS was only read a run-time (not compile-time)?
The value of BINDGEN_EXTRA_CLANG_ARGS should not affect how bindgen is compiled.

@kulp
Copy link
Member Author

kulp commented Jun 25, 2020

@tmfink, bindgen is sometimes (often ?) used to generate bindings at the time of build of a downstream crate. I believe that the current value of BINDGEN_EXTRA_CLANG_ARGS can change the generated bindings, and if so, changing the environment variable should definitely cause bindings to be regenerated.

I would be happy to be proven wrong, but on the other hand, being sensitive to this rarely-changing variable is the conservative option.

Am I answering the right question ?

@emilio
Copy link
Contributor

emilio commented Jun 25, 2020

That's right, it really doesn't need to rebuild bindgen itself, but all crates that depend on it. It'd be great if there was a way to tell this to cargo without rebuilding bindgen itself.

@tmfink
Copy link
Contributor

tmfink commented Jun 26, 2020

@kulp I understand now. The check was not added to cause bindgen to be recompiled but to get downstream dependencies to be recompiled. After some tests, it does seem that changes to BINDGEN_EXTRA_CLANG_ARGS causes bindgen to be recompiled which causes crates that depend on bindgen to also be recompiled.

As @emilio says, it's too bad we don't have a way to just get the dependencies to recompile. We could add a function like bindgen::build_script_checks() that would add this, but it would complicate the interface and not all existing users of bindgen would use it.

I think the current behavior is fine because BINDGEN_EXTRA_CLANG_ARGS probably won't be changing often for the same crate.

Thanks for the clarification!

@kulp
Copy link
Member Author

kulp commented Jun 26, 2020

@tmfink, it was a good question, and my original and secondary explanations were not very clear, so thanks for getting helping make this clear for future readers.

Maybe at some point cargo will have a feature that more precisely supports this need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants