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 support for mingw (-pc-windows-gnu) targets #52

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

micolous
Copy link

@micolous micolous commented Jun 27, 2023

Fixes #48

Open questions:

  • Adding link_lib_name_is_correct tests for mingw. How do I generate new targets in test-data/normalized?

Currently blocked on:

@pkgw
Copy link
Collaborator

pkgw commented Jun 29, 2023

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
Copy link
Author

micolous commented Jul 4, 2023

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
Copy link
Collaborator

pkgw commented Aug 26, 2023

@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
Copy link
Author

micolous commented Aug 28, 2023

@pkgw Per the initial comment, it's still blocked on alexcrichton/curl-rust#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
Copy link
Author

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
Copy link
Collaborator

pkgw commented Aug 28, 2023

Thanks for the status update!

@micolous
Copy link
Author

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
Copy link
Collaborator

pkgw commented Aug 29, 2023

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
Copy link
Collaborator

waych commented Aug 29, 2023

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

@waych
Copy link
Collaborator

waych commented Sep 20, 2023

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:

  • checkout microsoft/vcpkg
  • ./vcpkg-bootstrap.sh
  • ./vcpkg install --triplet x64-mingw-static curl
  • touch the .vcpkg-root file, required to be recognized as a vcpkg install.
  • from the ./installed directory, copy all the *.list files
  • from the ./installed directory, touch all non *.list files to make empty files.

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.

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 this pull request may close these issues.

Make it compatible with stable-x86_64-gnu rust toolchain + x64-mingw-dynamic vcpkg triplet ?
3 participants