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

Do not include static SFML libraries in windows builds because CSFML … #181

Merged
merged 2 commits into from
Jan 1, 2018

Conversation

TheJare
Copy link
Contributor

@TheJare TheJare commented Dec 31, 2017

…are DLLs with SFML linked statically inside them.

See discussion in #138

…are DLLs with SFML linked statically inside them.
println!("cargo:rustc-link-search=native={}/lib", sfml_home);
}
// The windows build of CSFML links SFML libraries statically, so no
// need to link them to the rust executable as well.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My primary concern is that I'm not sure if this is always the case. Is it the official download that does this? Does the GCC version do this too, or only the MSVC version? What about people who have built their SFML from source? Does it statically link by default when you build from source?

Perhaps we need to do more detailed analysis to determine these factors, and/or provide options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My belief is that how CSFML links to SFML is irrelevant, as long as CSFML itself is a DLL, which I understood was how this crate expects it to be. I'll try to compile CSFML in different ways and report back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, my theory is correct: CSFML must be built as a dynamic library (I couldn't build it as static library), and how it links to SFML is irrelevant to this crate.

To build this crate you need the CSFML .lib files, and to run it you need the CSFML DLLs. If the CSFML DLLs were linked statically to SFML then all you need to run the programs are the CSFML DLLs; if CSFML links dynamically to SFML then you also need the SFML DLLs to run the program.

In either case, you never need the SFML .lib files to build or run this crate.

(This is how I would expect dynamic libs to work on any OS, but I don't really know specifics about the topic outside of Windows)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Actually, I tested it, and not linking against SFML works even on Linux.

Now I'm not sure anymore why we even linked against SFML in the first place.

Anyway, for now I'll just merge this, and maybe completely remove linking against SFML later, if it turns out there is no need for it.

}
// The windows build of CSFML links SFML libraries statically, so no
// need to link them to the rust executable as well.
if !cfg!(target_family = "windows") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I forget: cfg! is actually not the correct way to do target-specific configuration, because cfg! is derived from the host, not the target. Cargo provides CARGO_CFG_ variants for build scripts.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify: This is probably fine if you're only ever building rust-sfml for the same target as the host, but it messes up cross-compilation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So something like var("CARGO_CFG_TARGET_OS").unwrap_or_default().as_str() != "windows" ?

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.

2 participants