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

[molleafauss]

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

[alexcrichton]

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]

I'll double check.

[sfackler]

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]

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]

I am a bit confused as to what changed from when this was written to now though: 3bce2e8. Presumably that was made for a good reason!

[alexcrichton]

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]

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]

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... 🤦‍♂️

[molleafauss]

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

[alexcrichton]

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]

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]

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

• CI doesn't actually ever enable crt-static testing, it's incorrect GitHub Actions configuration.
• When crt-static is disabled, OpenSSL dynamic libraries are built but they're linked statically according to the info we provide rustc, this generates linking errors because you're not supposed to do that.
• When compiled as a static library we're missing the -luser32 link argument, meaning that linking always fails if you actually enable crt-static.
• CI was accidentally ignoring some failures on Windows due to using the wrong shell.
• The directory with the dlls for OpenSSL wasn't actually added to Cargo's search path on MSVC.

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]

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]

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]

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

[molleafauss]

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

[alexcrichton]

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