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

Add missing windres.exe dependencies to Windows distribution #25030

Closed
wants to merge 1 commit into from

Conversation

gentoo90
Copy link
Contributor

@gentoo90 gentoo90 commented May 1, 2015

As mentioned in #11207 and rust-lang/rfcs#721, windres.exe may be used for embedding Manifest files into Windows binaries.
But windres.exe cannot compile *.rc files into *.res or *.o files without cc1.exe, which it uses as a preprocessor.

`windres.exe` cannot compile `*.rc` files into `*.res` or `*.o`
files without `cc1.exe`, which it uses as a preprocessor.
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@alexcrichton
Copy link
Member

Hm, could you elaborate a little more on why we should include this? We don't currently do anything with manifests, so it may be a little premature to include this tool. I also know that in the past we've avoided libgmp for bignum purposes for license reasons, and I'm not quite sure what the implications are of including it in our distribution, we'd just want to take that step carefully.

@gentoo90
Copy link
Contributor Author

gentoo90 commented May 1, 2015

This will allow users to embed a manifest (or other resources like icons) with a minimum workaround and without the need to install full mingw toolchain.
Here is the minimal example of manifest embedding with the cargo build-script, which works if cc1.exe and it's dependencies are copied to the folder with windres.exe.

Well, almost works, because cargo currently doesn't allow passing -C link-args= to rustc, so I used cmd file to run final linking command.

@gentoo90
Copy link
Contributor Author

gentoo90 commented May 1, 2015

As for libgmp, the windres output will not be linked to it. It's just a dependency of cc1.exe, which is linked to it dynamically (and is an open source product), so shoud be fine for LGPL.

PS: I'm NOT an expert in licencing.

@alexcrichton
Copy link
Member

Right now we don't currently try to produce a full suite of working tools with the compiler on Windows distrubutions, only the bare bones necessary to get the compiler itself working. I'm not sure that we want to dive into providing a whole bunch of utilities as they aren't necessarily standard for Windows (e.g. most installs don't have mingw/msys) and may have other possibilities (e.g. official Microsoft-provided tools).

Put another way, to embed a manifest we may not want to provide this tool but rather do it through some other means and we may not want to support this for forever.

@vadimcn
Copy link
Contributor

vadimcn commented Jun 3, 2015

FWIW, I bundled windres specifically to give devs tools for embeding Windows resources without having to install mingw. I didn't notice that it needs cc1.
I think we should either start bundling cc1, or remove windres... Probably the latter, since mingw's cc1 weighs in at 15MB. Not worth adding more than 10% to Rust's download size.

@alexcrichton
Copy link
Member

Ok, it sounds like we probably don't want to expand our distribution here, so I'm going to close this. I've opened #26803 about removing this from the distribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-needs-decision Issue: In need of a decision.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants