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

Print Out SDL2 Include Directory in sdl2-sys Build Script #968

Merged
merged 4 commits into from Feb 5, 2020
Merged

Print Out SDL2 Include Directory in sdl2-sys Build Script #968

merged 4 commits into from Feb 5, 2020

Conversation

cwfitzgerald
Copy link
Contributor

In order to facilitate using the includes provided/found by sdl2-sys in other *-sys crates, I have added print statements which set the "include" key to the path to the include folder that has either been provided or found.

This can be accessed through the environment variable DEP_SDL2_INCLUDE in all crate build scripts which directly depend on sdl2-sys, transitives are not affected.

If there are multiple include directories, they are separated by a :.

A use case can be found here where I must pass the header directory to cc to build c++ code which depends on SDL2.

I have tried to make as few changes as possible to the build script, however I did make two changes that shouldn't affect the usability of it at all:

  • I factored the retrieval of the bundled header path into it's own function, so it can be called in two places.
  • I changed generate_bindings to no longer be generic, as every single instance was being called with S = String. This started because I wanted to use .join(":") on the slice, but that requires something that is Borrow<str> not AsRef<str>. As borrow is a newer feature, I didn't know your minimum rustc version, and all invocations were with String, I just went ahead and removed the generic.

All of the particulars about the PR can be changed if needed to fit style, usability, etc. I have verified that my own fork works as expected for downstream crates.

It might be worthwhile to add this to documentation as well, though I don't know if that is deserving of its own PR.

Closes #967

@Cobrand
Copy link
Member

Cobrand commented Feb 5, 2020

Great job!

Question though, what do you mean by

This can be accessed through the environment variable DEP_SDL2_INCLUDE in all crate build scripts which directly depend on sdl2-sys, transitives are not affected.

Where does DEP_SDL2_INCLUDE come from?

@cwfitzgerald
Copy link
Contributor Author

cwfitzgerald commented Feb 5, 2020

That is how cargo passes the information forward. All crates that have a links parameter can output arbitrary key-value pairs from their build script in the form of cargo:<key>=<value>. These get passed to their immediate dependents in the environment variable DEP_<links>_<key>, in SCREAMING_SNAKE_CASE. For example, with sdl2-sys, it has links = "sdl2", and the value I pass back has the key of "include" so that gets combined into DEP_SDL2_INCLUDE.

This is all documented in the cargo book, though the documentation is kind of poor and you have to piece the story together though the examples and a variety of different sections. Here, the second to last paragraph here, and a bit more clearly in the example here. Maybe the next thing I should do it touch up the docs.... 😄

@Cobrand
Copy link
Member

Cobrand commented Feb 5, 2020

Really interesting, I didn't know about that. You are right, perhaps one or two sentences in the readme explaining what you've just said (or linking to this issue) could help someone with your same use case!

@cwfitzgerald
Copy link
Contributor Author

Is this issue worthy of the changelog also? If so where should I put it?

@Cobrand
Copy link
Member

Cobrand commented Feb 5, 2020

In the changelog create a "v0.34" or "unreleased" entry above all the rest, and I'll tidy it up when I push an actual release then. Thanks!

@Cobrand
Copy link
Member

Cobrand commented Feb 5, 2020

Perfect, thanks!

@Cobrand Cobrand merged commit 4ff7e9c into Rust-SDL2:master Feb 5, 2020
@cwfitzgerald cwfitzgerald deleted the include-from-sys-pr branch February 5, 2020 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get Include Directory in Dependencies
3 participants