search

mcgoo / vcpkg-rs

Build library for finding native libraries in vcpkg for Rust - Windows (msvc), Linux and macOS
https://docs.rs/vcpkg
Apache License 2.0
108 stars 22 forks source link

Add support for mingw (-pc-windows-gnu) targets #52

Open micolous opened 1 year ago

micolous commented 1 year ago

Fixes #48

Open questions:

Currently blocked on:

pkgw commented 1 year ago

Thank you for taking this on! To be honest I personally have a very poor grasp of Windows target triples, so I doubt that I am going to be able to review any work here with a lot of expertise.

One thing I did want to mention is that I do know that vcpkg-rs has some issues relating to cross-compilation. Specifically, it has the kinds of problems that are solved in the tectonic_cfg_support crate: if you have build scripts that need to deal with cross-compiles, they need to check the CARGO_CFG_TARGET_* variables rather than using the built-in cfg!(target=) constructs. This is because the build script is (of course) compiled to run on the build host, so its target is that architecture, not the target of the overall project. There are a few places where vcpkg-rs is jumping through some hoops because the pieces that would run in build scripts aren't doing this properly.

micolous commented 1 year ago

Yeah, my main reason for doing this is to enable cross-compilation from non-Windows hosts to Windows targets, and the PRs I have out for libz-sys, curl-rust and openssl-sys fix similar issues.

pkgw commented 1 year ago

@micolous Is this PR still a draft? I see that the systest/Cargo.toml patches in some personal repo forks; have your PRs to those repos all gone in?

micolous commented 1 year ago

@pkgw Per the initial comment, it's still blocked on https://github.com/alexcrichton/curl-rust/pull/509.

It's pointing at my forks because I was waiting for pull requests to land, and for a release of those packages. It looks like my PRs libz-sys and openssl-sys have since been released – so I've updated the original post with that progress.

As for "why": curl-rust's maintainer is very cautious about changing the build scripts, so that hasn't landed yet. One adjacent issue which popped up is that curl-rust depends on the vendoring behaviour of libz-sys on Windows, so a change in libz-sys broke curl-rust.

IMO, Rust crates shouldn't silently auto-vendor (it makes dependency management extremely difficult). However, it'll be a SemVer-incompatible change to remove that, and both of those crates need to work out some of their build script limitations in the process (eg: introducing build-time configuration like openssl-sys has) and then handle any fall-out from the change with their downstream users.

Fixing all of that is way beyond my originally-intended scope – I only really care about linking against OpenSSL, and openssl-sys is not affected by any of this. At least until I discover one of those crates in my project's dependency graph. 😅

I believe that vcpkg-rs' CI does not verify which libraries are linked against, so auto-vendoring made that test ineffective anyway (it would continue to pass, even if not using a vcpkg-provided library).

An alternative is that if libz-sys and openssl-sys continue to work with this PR, then vcpkg-rs could (temporarily) drop curl-rust from its CI on -pc-windows-gnu. I don't think that would break anything on either side, it just means it's not testing things quite so well, and curl-rust would continue to not support vcpkg on -pc-windows-gnu; so it doesn't make things any worse than the status quo.

🤷

micolous commented 1 year ago

This PR now drops curl-rust from the -pc-windows-gnu CI tests, and no longer points at my forks of those packages.

I've started a CI run, but this will take a while.

pkgw commented 1 year ago

Thanks for the status update!

micolous commented 1 year ago

It looks like CI is passing, but per above, I only really trust openssl-sys to give accurate results. I've got another minor change running in CI at the moment to aid debugging.

@pkgw I'd like to have a link_lib_name_is_correct test for mingw. How do I generate new targets in test-data/normalized?

pkgw commented 1 year ago

I'm afraid I don't actually know. @waych? I think it looks like that tree just contains the results of successful vcpkg installs, with the binaries all replaced with zero-size files? In digging through the repo history I don't see any explanation of how the files were generated, though.

waych commented 1 year ago

Hi sorry I won't be able to look at this until next week at best.

waych commented 1 year ago

Spent some time looking at this and your other PRs related to making this work.

I don't see much problem with getting the wiring for the targets in, but it is very unfortunate that the CI is blocked requiring other changes you are pushing to other projects. I dug through these PRs too it mostly makes sense to me what you are doing.

I understand the quest for disabling autovendoring, but also sort of agree with the counter argument that having to opt-in to autovendor for crates is a potential cause of friction for oncoming new cargo/rust users. Have you considered Byron's suggestion of having the autovendoring be disabled by some flag? Introducing a community environment variable like CARGO_AUTOVENDOR_OPTOUT or something that can be set in config.toml.env, and that various build.rs can pivot off of to disable the autovendor support altogether? That does avoid the semver bump doesn't it?

I don't understand the parts of the discussion I've seen trying to associate static linking with the use of autovendored libs. Could you please elaborate on that? Seems unrelated to me / I need help seeing the connection there, as it still makes sense to link statically with other options.

As for the files in test-data, this is just a bare vcpkg repository mock install. To generate, I don't think there is a script, it is simply the output from something like:

I have a project that I'll be trying these mingw changes out with in the coming days, so I'll let you know how it goes.