search

alexcrichton / openssl-src-rs

Source code and logic to build OpenSSL from source
Apache License 2.0
69 stars 114 forks source link

Moved "no-shared" so that also windows statically link to the libraries #83

Closed molleafauss closed 3 years ago

molleafauss commented 3 years ago

Moved the argument and added it close to the "no-dso", as it's seems it's a "related" configuration switch.

alexcrichton commented 3 years ago

Thanks! Since I'm not entirely sure whether we have CI for it or not, have you confirmed this works for you both with and without crt-static?

molleafauss commented 3 years ago

I'll double check.

sfackler commented 3 years ago

It looks like we do have CI coverage for that I think: https://github.com/alexcrichton/openssl-src-rs/blob/master/.github/workflows/main.yml#L99-L107

molleafauss commented 3 years ago

Also, @alexcrichton should the crt-static workflow here be phased out given it was used only to check the two different linking version (wondering how it was working though...)? https://github.com/molleafauss/openssl-src-rs/blob/master/.github/workflows/main.yml#L90

sfackler commented 3 years ago

I am a bit confused as to what changed from when this was written to now though: https://github.com/alexcrichton/openssl-src-rs/commit/3bce2e86cea1ce9e13d3a44ecc9e3fba17e3f3fd. Presumably that was made for a good reason!

alexcrichton commented 3 years ago

Hm so C/C++ code needs to be compiled with a different flag depending on the crt-static value, and typically that happens through the cc crate but reviewing the code here it looks like cc may not be used and the no-shared/etc flags are what's being used. In that case @molleafauss when testing this I'd expect it to fail?

Looks like CI is at least busted in one way because there's failures which aren't failing CI. I'll look into that.

molleafauss commented 3 years ago

Haven't looked into what tests are run in this crate, and how the built openssl lib is used. I would indeed expect that the build missing the no-shared would fail, as it would not be able to load the openssl dlls, unless those libraries were available system-wide (i.e. via %PATH%) in the docker image used for testing? But if the tests are only checking that the openssl sources are built properly, both shared and dll build for windows are working, the issue is with the upstream dependencies (openssl->openssl-src->this crate) and what they end up linking ultimately. When the dll were built, then rustc was linking only the stubs to your binary, and then the binary would fail running, as the .dll were nowhere to be found.

molleafauss commented 3 years ago

ouch - think I see the issue: this is in the windows build; both x86_64-msvc-static and x86_64-msvc. But the build passes... 🤦‍♂️

immagine

molleafauss commented 3 years ago

And as I was suspecting this is the build environment: the OpenSSL 1.1.1 is included...

alexcrichton commented 3 years ago

Hm ok so testing this locally, I'm surprised that this ever worked actually. The problem the current static builds (pre-this-PR) is that a new dependency (user32.dll) was introduced and we're not linking it. That's easily fixed, however. The pre-this-PR non-static builds are actually doing entirely the wrong thing. They're attempting to statically link the import libraries, which is causing the duplicate symbol error. This crate was never intended to build shared libraries, so the fact that it's building shared libraries when +crt-static is missing is entirely a mistake.

This PR definitely fixes that issue and can be fixed in CI by adding an extra-linked library (user32.dll), but the problem still remains that the C code is compiled with /MT instead of /MD with -crt-static. I don't know how to fix that with OpenSSL's build system at this time, though. Somehow that needs to be fixed as well to fully fix this

sfackler commented 3 years ago

There is this thing that README.Windows mentions briefly you're supposed to compile into your application: https://github.com/openssl/openssl/blob/master/ms/applink.c. Maybe it does that to avoid dealing with static vs dynamic linkage to the crt?

alexcrichton commented 3 years ago

Ok so there's actually a whole mess of problems wrong with this crate and its CI right now:

I think those are all fixed with #84, however. Want to try rebasing over that and see what happens?

It may still be the case that +crt-static appears to work, but I don't think it will in general because there still isn't any supporting code to figure out how to compile libssl.lib with /MD for linking to libcmt.lib

molleafauss commented 3 years ago

Did rebase the work, all tests are passing. I've removed the crt-static flag, given it seems pointless, basically making the configurations msvc and msvc-static identical. I see that the tests are run. Let me know your thoughts.

alexcrichton commented 3 years ago

Ok I did some testing locally and happened to stumble upon https://stackoverflow.com/questions/3469841/mixing-code-compiled-with-mt-and-md. Indeed OpenSSL is compiled with /Zl, and testing locally I can mix a /MT OpenSSL with a /MD object file and it appears to work. Given that OpenSSL seems like it may be passing that option intentionall then this looks good to me!

Sorry for the runaround with weird CI and such...

In any case, want to drop the shared flag now since it's always false? Other than that seems good to merge to me!

molleafauss commented 3 years ago

no probs, will give a look later tonight (you might have guessed I'm on GMT here)

molleafauss commented 3 years ago

Also should we remove the MSVC CI configs for the crt-static, leaving only 2 and not 4 for windows?

alexcrichton commented 3 years ago

Nah since that's different enough at the rustc level I'd prefer to not drop them just yet.