-
Notifications
You must be signed in to change notification settings - Fork 491
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 feature 'bundled' #693
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 haven't tested this, just read it quickly, but if I'm not mistaken every time you build this will download the archive, uncompress it, build it and link it statically. If that's really what happens, it would be much better to have a way to "cache" the output of all of that so you don't have to re-downloaded and re-compile everything every time you want another build.
Also, I expected the "bundled" feature to link dynamically the libraries... For instance take a look at sqlite3: https://github.com/jgallagher/rusqlite/blob/master/libsqlite3-sys/build.rs#L81
Can you show me the crates you have contributed to that have this "bundled" feature where it's linked statically?
I think your PR is fine as well, but maybe we could call it "bundled_static" or "static_bundle" or something, to avoid confusing users between crates.
Cargo.toml
Outdated
@@ -4,7 +4,7 @@ name = "sdl2" | |||
description = "SDL2 bindings for Rust" | |||
repository = "https://github.com/AngryLawyer/rust-sdl2" | |||
documentation = "http://angrylawyer.github.io/rust-sdl2/sdl2/" | |||
version = "0.30.0" | |||
version = "0.30.1" |
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.
Please update to 0.31, or don't update sdl2 at all. There has been some breaking changes since 0.30, and if we want to update it has to be 0.31 and not 0.30.1
However I still have some changes to do before 0.31, so I'd appreciate if you could just bump sdl2-sys to 0.30.1 and leave sdl2 as-is.
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.
Well, I modified Cargo.toml so it seemed reasonable to make it a micro version bump since no code was changed. Also, I just tended to prefer to not develop with a version that matches a published version.
sdl2-sys/build.rs
Outdated
#[cfg(feature="bundled")] | ||
fn main() { | ||
let target_info = TargetInfo::init(); | ||
let sdl2_archive_url = "http://libsdl.org/release/SDL2-2.0.5.tar.gz"; |
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.
Can you put "2.0.5" in a const
variable somwhere and use the const variable everytime you have "2.0.5"? It will be nicer to edit a single const from "2.0.5" to "2.0.6" instead of searching for all the lines.
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.
Good idea.
I was the originator of the example you're referring to. :) And if you look more closely at that build script you'll see that it is in fact building a static library, we just didn't use I believe I can change those lines from this build script and it'll still work out but I haven't tried yet. I'd rather add another feature called Yes, you're right about this always re-downloading the archive. That isn't really smart. A better approach would be to cache that so I'll make the change today sometime. I tried this out on Linux and there are some missing symbols in several examples so I need to figure out what the missing requirement is. |
Allows statically linking to a self-contained build of SDL2.
On my linux test machine I only needed to add a link directive for libsndio and examples are running now. |
Ok, this should get linux builds working on more or less any system. There should probably be some approach here to enable more or less of the build.
Hrm... I guess libsndio isn't typically installed on machines. Or at least not on the travis system. We can disable it in the configuration step for the moment and look into other things. |
I only have a Linux myself available, but just for info, how do you find which libraries you need to include for static linking? Other than that, LGTM |
In this case I just read over the CMakeLists.txt to figure out the options available after I tried building and running an example and it didn't link. The linker errors told me what functions were missing so it was a bit obvious thankfully. On Windows there are so many and a big list is printed out if you run the cmake build manually. |
cc @michaelfairley for OSX insight, would you be able to help with that? |
} else if target_info.os == "darwin" {
println!("cargo:rustc-link-lib=framework=Cocoa");
println!("cargo:rustc-link-lib=framework=IOKit");
println!("cargo:rustc-link-lib=framework=Carbon");
println!("cargo:rustc-link-lib=framework=ForceFeedback");
println!("cargo:rustc-link-lib=framework=CoreVideo");
println!("cargo:rustc-link-lib=framework=CoreAudio");
println!("cargo:rustc-link-lib=framework=AudioToolbox");
println!("cargo:rustc-link-lib=iconv"); will get you building on mac os. I've been working on something like this myself, though it's in a rather messy (lottts of duplication), half-implemented, and not-entirely-always-actually-static-linking state: https://github.com/michaelfairley/sdl2-lib. It uses the autotools build setup, since the cmake one has limited platform support and is officially "experimental". "Doesn't require an internet connection to build" and "supports emscripten, ios, and android" are very important features for me. |
I've thought about this and I'm really wondering: why does the bundle has to imply static linking and vice versa? What prevents us from having two features there, one being "download the library from source and compile it", and the other being simply "static link instead of dynamic". If you don't have an argument against that proposition, I would like these two to be 2 separate features, since sometimes people will want to build statically and sometimes they will want to download the library and link dynamically. |
Is this going to work for SDL2-Mixer? |
It is not going to work for SDL2-Mixer or any of the other features for now, but if this goes well we'll definitely include those features in a future PR |
@photex it's been a few weeks since you've last updated this issue, if you don't answer within the next week (or unless you tell me so), I will take care of finishing the implementation in another PR and close this one, while of course keeping your commits intact. |
@Cobrand sorry! Thanks for the reminder. I just started a new job and have been distracted. I think your suggestion to specify the link/library style via another feature is absolutely the right idea. The more people this can serve the better. There is also the opportunity to download pre-built libraries if we wanted to support that. Perhaps along these lines: @michaelfairley CMake has been the most reliable for me, and is the only option for uniformly building on the big 3 platforms. But all that aside, I think if you've started work on a version using autotools then maybe on Linux and MacOS we can use autotools instead of cmake. That could answer some questions or open some doors for the other libraries too so I'm open to help with that if I can. |
This feature has been updated and overhauled to match master in PR #720. I will close this in favor of the other PR. Please give your input on the other PR, if some of you on Windows/macOS could give some input on the other PR, that would be a lifesaver! |
Allows statically linking to a self-contained build of SDL2 as mentioned in #681.
Currently tested is Windows-MSVC.