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

crt-static is only translated to -static under gcc, not clang #1074

Open
mqudsi opened this issue May 20, 2024 · 7 comments
Open

crt-static is only translated to -static under gcc, not clang #1074

mqudsi opened this issue May 20, 2024 · 7 comments

Comments

@mqudsi
Copy link
Contributor

mqudsi commented May 20, 2024

In add_default_flags(), the presence of feature crt-static causes -static to be supplied to the compiler iff it's gcc; clang doesn't get the same treatment.

I know that cc itself doesn't compile without -c so the presence of -static makes no difference there, but if a downstream library or crate uses cc to manually get a compiler invocation and runs that without -c to generate an executable itself in build.rs (and some do), it matters.

I can easily open a PR to do the same under clang, but given the subtleties of how much linker-related stuff this crate should be doing to begin with I preferred to first open an issue.

@NobodyXu
Copy link
Collaborator

Hmmm, could we remove that -static flag?

I wonder why is that added in the first place.

@mqudsi
Copy link
Contributor Author

mqudsi commented May 22, 2024

I think removing it makes the most sense. crt-static is not static and shouldn't come with the assumption that just because we're statically linking the runtime it means that all libraries should also be statically linked.

I mean, that's one way of interpreting it but clearly rust doesn't interpret it that way, so we probably shouldn't either.

Not sure about breakage, though.

@NobodyXu
Copy link
Collaborator

Not sure about breakage, though.

I think it should be fine.

Don't think there are many crates using cc to generate executables

@NobodyXu
Copy link
Collaborator

Looking at it, I found that we also have a function cc::Build::static_flag to add -static.

Honestly I have no idea why it's there in the first place, cc @thomcc Do you know why it is there (and if we could just remove

cc-rs/src/lib.rs

Lines 2173 to 2175 in 3ba2356

if features.contains("crt-static") {
cmd.args.push("-static".into());
}
)?

@mqudsi
Copy link
Contributor Author

mqudsi commented May 30, 2024

I don't think we can just remove static_flag() itself without breaking a decent amount of code.

It's used a decent amount though as far as I can see it seems most usage is under the very incorrect assumption that it determines whether the output is a .a or .so/.dylib.

Specifically for projects calling .compile(), we should be able to safely remove the flag itself from default_flags() though (and separately mark the static_flag() routine as deprecated).

But a reminder that there are crates that use cc-rs to get build flags before then manually compiling binaries. My own rsconf is only one example; I'm sure there are others.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jun 1, 2024

and separately mark the static_flag() routine as deprecated

I agree, it should be marked as deprecated, while keep the existing implementation.

I will then open a PR to remove

cc-rs/src/lib.rs

Lines 2173 to 2175 in 3ba2356

if features.contains("crt-static") {
cmd.args.push("-static".into());
}

@riidefi
Copy link

riidefi commented Jun 14, 2024

We should probably specify the clang-equivalent of -MT (on clang-cl) when on Windows. It is odd that if you run SET CXX=clang before cargo build on Windows, the -Ctarget-feature=+crt-static RUSTFLAG is ignored.

E.g.

Tell C++ to use the dynamic version, via one of the following methods (depending on build system):

  • Directly specify '/MD' to cl.exe or clang-cl.exe during the C++ compile/link steps.
  • Directly specify '-Wl,-nodefaultlib:libcmt -D_DLL -lmsvcrt' to clang++.exe

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 a pull request may close this issue.

3 participants