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

Run pkg-config on MSVC #39

Merged
merged 1 commit into from
Jul 23, 2023
Merged

Run pkg-config on MSVC #39

merged 1 commit into from
Jul 23, 2023

Conversation

ferjm
Copy link

@ferjm ferjm commented Aug 28, 2018

The root cause of the pkg-config failure on MSVC seems to be that pkg-config-rs is appending .lib, while it shouldn't because rustc already does that on Windows.

@sdroege is fixing this issue in rust-lang/pkg-config-rs#72.

@sdroege
Copy link

sdroege commented Aug 28, 2018

Confirmed in the servo build that this actually works

@alexcrichton
Copy link
Member

I think one thing I've also seen in the past is that if you build for MSVC in a MinGW environment then pkg-config can pick up the MinGW libraries which end up causing link errors later (as they're linked with MSVC). I think we may want to handle that sort of case here as well?

@sdroege
Copy link

sdroege commented Aug 28, 2018

Generally the ABI of MSVC and mingw is compatible for C (not C++!). Do you have more details?

@alexcrichton
Copy link
Member

Ah sorry I don't recall the logs offhand, but I believe it's moreso around linking than it is ABI issues. C code using the C standard library may link against libc from MinGW but fail to link against MSVC because the C standard library has slightly different internal ABIs

@sdroege
Copy link

sdroege commented Aug 29, 2018

For GStreamer, we're (currently) building our binaries with MinGW and people are using them with MSVC and MinGW.

The C standard library used by MinGW and MSVC is also the same (msvcrt), it might just be a different version. But that's not a problem on Windows, you can have different versions in the same process (you just can't e.g. malloc() from one and free() the same memory from another, or share fds). MinGW builds are however depending on libgcc_s and similar libraries in addition.

Not sure how to proceed here without further information :) How do you want to make sure a "correct" library is chosen?

@alexcrichton
Copy link
Member

Hm yeah I think DLLs work out, but you're not shipping objects, right? I'm worried, for example, about picking up libz.a, a static archive, compiled for MinGW. That may internally have something like assert(...) which likely compiles to different internal symbols on MSVC vs MinGW (I believe it's at least changed between MinGW versions). Later when we link everything together using link.exe I think it'd fail because it's expecting MinGW symbols to be in MSVC libs.

TBH I have no idea how one would even begin to configure pkg-config to work for MSVC, nor any idea even how common it is. In that sense I don't know how a MSVC pkg-config would look different from a MinGW pkg-config :(.

Could this be behind an off-by-default flag or env var? My experience so far is basically that if pkg-config is used on MSVC it inevitably breaks a bunch of AppVeyor CI for projects like Cargo, curl, RLS, etc.

@nirbheek
Copy link

nirbheek commented Aug 29, 2018

Hm yeah I think DLLs work out, but you're not shipping objects, right?

Yes, static linking to static libraries and objects that use the MinGW API will lead to undefined symbols since at link time the GCC linker automatically adds some compiler static libs to resolve those (libgcc_s.a and more). The list of compiler static is small though, and I have successfully linked MinGW static libraries with link.exe by providing those libraries. This is just an aside, and not something that we can expect users to do.

I'm worried, for example, about picking up libz.a, a static archive, compiled for MinGW.

I thought rustc transforms foo to foo.lib with MSVC? Wouldn't it be impossible for us to pick up libz.a in that case? Note that libfoo.a, a static library built with GCC that does not have any undefined symbols can be successfully used by link.exe.

TBH I have no idea how one would even begin to configure pkg-config to work for MSVC, nor any idea even how common it is. In that sense I don't know how a MSVC pkg-config would look different from a MinGW pkg-config :(.

I am not sure what you mean by this. The output of pkg-config is mostly compiler-agnostic, and we already handle the differences in the pkg-config crate. .pc files for libraries built with MSVC and with MinGW will be identical.

@sdroege
Copy link

sdroege commented Aug 30, 2018

I'm also not sure why pkg-config-rs should care about this, it seems like it's the user's job to make sure dependencies are found (i.e. PKG_CONFIG_PATH and friends are set correctly) and available in usable versions (i.e. if linking a static MinGW library with MSVC, the pkg-config files should have -lgcc_s/etc).

The simple reality is that linking libraries (see also DLL hell) on Windows is a mess and things need to be properly set-up in the environment so that it can succeed, independent of pkg-config or not.

@alexcrichton
Copy link
Member

Er sorry, so my point is that I've given up historically with MSVC and pkg-config precisely because all of the situations that aren't supposed to happen end up happening. For example AppVeyor builds (and rustc builds itself) run with MinGW tools in the environment which means that pkg-config points to libz.a (a static archive), meaning that if this were merged as-is then MSVC builds would pick up a statically linked copy of libz.a for MinGW, causing link errors. (that's what motivated this if condition in the first case)

It's on users for sure to set up pkg-config, but it's just too easy for users to accidentally pick up a MinGW pkg-config when compiling for MSVC. I would personally prefer to (perhaps in the pkg-config crate) detect this sort of situation and bail out, as it's basically guaranteed to not work.

@sdroege
Copy link

sdroege commented Aug 31, 2018

It's on users for sure to set up pkg-config, but it's just too easy for users to accidentally pick up a MinGW pkg-config when compiling for MSVC. I would personally prefer to (perhaps in the pkg-config crate) detect this sort of situation and bail out, as it's basically guaranteed to not work.

In my cases I'm actually using the same pkg-config (and the same .pc files) for both MSVC and MinGW builds, so it's not that easy :)

Not sure what to do here then, seems like it boils down again to linking and development on Windows in general being a mess. Every option I can think of will fail for someone or require non-obvious configuration via environment variables to change the behaviour of different crates' build.rs

@nirbheek
Copy link

Another thing to note is that Meson always uses libfoo.a for static libraries even when building with MSVC because otherwise you can't build both static and shared libraries at once, because there will be a naming collision between foo.lib (static library) and foo.lib (import library). So that libz.a could be a zlib built with MSVC using Meson. The format is the same (ar archive with objects) for lib.exe and ar so the two are interoperable.

@alexcrichton
Copy link
Member

Would it be possible to actually open up the archive and take a look inside? Perhaps there's an easy-to-find marker which differentiates MinGW from MSVC?

@nirbheek
Copy link

nirbheek commented Sep 3, 2018

We could extract an object file from the the archive, peek at the sections in it, and look for a .bss section. MSVC does not create that section, but GCC/MinGW and Clang/MinGW do. I'm checking to see if clang-cl also creates it.

@nirbheek
Copy link

nirbheek commented Sep 3, 2018

Okay, clang-cl also creates .bss sections, and I'm not sure if clang-cl static libraries are usable standalone. I would guess yes, but it might depend on other configuration such as a VC toolset.

The most reliable way to check for usability of a static library is to try to link to it with simple int main () { } C program. If there's a way for pkg-config-rs to do that, it should.

@joshtriplett
Copy link
Member

I don't think trying to sniff out usable libraries is safe, in general; down that path lies a lot of platform-specific pain.

@retep998, could we get some help figuring out the Right Thing to do here, when using pkg-config on Windows?

@nirbheek
Copy link

nirbheek commented Jul 1, 2020

Fwiw, I'm mostly describing what Meson does on Windows for library detection. All the bugs I've encountered with pkg-config-rs over the past 2 years (on all platforms) have been issues that Meson already solved. I was planning to duplicate that logic in pkg-config-rs. It would not involve peeking into libraries, but it would involve manual filesystem searching, then invoking the compiler to link to the library in question and using the full path to it instead of -L -l / -LIBPATH: foo.lib pairs.

@joshtriplett
Copy link
Member

@nirbheek Is that something Meson only does on Windows, or does it also attempt to do manual filesystem searching on other platforms? (One of the biggest potential problems with attempting to reimplement pkg-config is that distributions sometimes patch pkg-config for their particular library paths, such as for multiarch; manual searching would typically be unaware of the right paths to use and the paths to carefully not use. I've run into software that tries to link a library for the wrong architecture because it's aggressively searching in a directory it thinks should contain the library it wants.)

@retep998
Copy link
Member

retep998 commented Jul 1, 2020

In my opinion, using pkg-config on pc-windows-msvc will most likely not work unless the user intentionally goes through the effort of configuring pkg-config to provide msvc compatible libraries, at which point the user could also use alternative means to configure linking via build script overrides and such.

@nirbheek
Copy link

nirbheek commented Jul 1, 2020

@nirbheek Is that something Meson only does on Windows, or does it also attempt to do manual filesystem searching on other platforms?

Meson manually resolves the path to the library to link on all platforms because depending on the linker search path at compile-time is completely broken. If you have multiple sources for libraries to be picked up from (which is fairly common even on Linux), it's quite likely you'll pick up the wrong one.

(One of the biggest potential problems with attempting to reimplement pkg-config is that distributions sometimes patch pkg-config for their particular library paths, such as for multiarch; manual searching would typically be unaware of the right paths to use and the paths to carefully not use. I've run into software that tries to link a library for the wrong architecture because it's aggressively searching in a directory it thinks should contain the library it wants.)

The idea is not to reimplement pkg-config, but to bypass the linker's automatic library searching.

If you call pkg-config with PKG_CONFIG_ALLOW_SYSTEM_LIBS=1 in the env, it will always output -L args pointing to the system, so that solves the case of distros building pkg-config with --with-system-library-path that you're talking about.

Linking to the library solves the second issue of picking up a library for the wrong arch. F.ex., sometimes distros put both 32-bit libs and 64-bit libs in that path, or the user has made a mistake and pointed PKG_CONFIG_LIBDIR at the wrong directory.

But the PkgConfigDependency class in Meson (linked above) is a product of ~4 years of work. I don't expect to implement all of it in pkg-config-rs 😄

pkg-config-rs on Windows only needs a tiny subset to work with MSVC: convert -lfoo to foo.lib|libfoo.a, look for both in the path specified in -L flags inside the pkg-config file. If something isn't found, leave the flags in (after converting -L to -LIBPATH:). That's about it. The rest can happen when/if people report issues.

In my opinion, using pkg-config on pc-windows-msvc will most likely not work unless the user intentionally goes through the effort of configuring pkg-config to provide msvc compatible libraries, at which point the user could also use alternative means to configure linking via build script overrides and such.

I am not sure it's feasible to tell people to inject a dependency tree using build script overrides. Note that this is required for gstreamer on Windows; we have about 50 rust plugins that have to find gstreamer (+ deps) using pkg-config.

There really isn't an alternative on Windows. CMake dependency files are even worse since they're turing-complete and you literally have to create a temporary build directory and run cmake to parse them. We do this in Meson too, and I would not recommend it.

@nirbheek
Copy link

nirbheek commented Jul 1, 2020

In my opinion, using pkg-config on pc-windows-msvc will most likely not work unless the user intentionally goes through the effort of configuring pkg-config to provide msvc compatible libraries

Ah, I should also mention: by default pkg-config on Windows enables --define-prefix which means that all you need to do is point PATH to it, and it will automatically pick up .pc files from ..\lib\pkgconfig and rewrite the prefix= variable to point to the portable prefix. So you don't need to set any special env vars like PKG_CONFIG_PATH or PKG_CONFIG_LIBDIR. There is no special user configuration required 🙂

@sdroege
Copy link

sdroege commented Jul 1, 2020

You might also want to take a look at the system-deps crate (CC @gdesmott), which allows to specify the dependencies in a declarative way in Cargo.toml, provides a standardized way of overriding things (and defaults to pkg-config), and also provides hooks (and a standardized way to enable it) for building the system dependency as part of the crate. This is now used by all the rust-av, gtk-rs/gstreamer-rs crates for finding the system libraries.

For the general problem of pkg-config on Windows, I agree that there's not really any other option than that right now for a scalable solution. You don't really want people to specify things via custom environment variables for all the 100 system dependencies that might be in their dependency tree, and you also don't really want every crate to implement it's own artisanal way of finding the library manually (or environment variables to specify behaviour, see system-deps for a solution to that) that then requires the user to check what each crate is doing instead of just depending on setting up pkg-config correctly and the same way for everything.

@joshtriplett joshtriplett reopened this Aug 14, 2020
@joshtriplett joshtriplett changed the base branch from master to main August 14, 2020 22:03
@pkgw
Copy link

pkgw commented Oct 5, 2020

Is there a chance of merging this? I am running into issues building tectonic on Windows in conda-forge, which generally favors pkg-config, and if I patch in this change it fixes my build.

(edit: I should mention that the build error is redundant symbols from the "system" libz, found via pkg-config, and the version built inside libz-sys. It is very hard for me to see how to avoid the problem without including this patch.)

@barcharcraz
Copy link

Are there any updates on what's going to happen with this PR? I'm also interested in finding zlib on windows with pkg-config

@Byron
Copy link
Member

Byron commented Jun 16, 2023

This PR has been inactive for quite a while without resolution. Here is the GPT-summary (which I didn't validate) thus far:


The GitHub page you provided is a pull request on the libz-sys repository in the Rust programming language. Here's a summary of the contents:

  • The pull request, labeled as Run pkg-config on MSVC #39, is titled "Run pkg-config on MSVC" and is opened by a user named "ferjm" from their personal fork of the repository.
  • The user comments that the root cause of the pkg-config failure on MSVC (Microsoft Visual C++) seems to be that pkg-config-rs is appending .lib to the library name, while it shouldn't because rustc already does that on Windows.
  • They mention that another user named "sdroege" is fixing a similar issue in pkg-config-rs in the rust-lang/pkg-config-rs repository. The reference to that issue is rust-lang/pkg-config-rs#72.
  • Other contributors, including "alexcrichton" and "nirbheek," participate in the discussion. They discuss potential problems with picking up MinGW libraries when building for MSVC, differences between MSVC and MinGW ABIs, and the challenges of configuring pkg-config for MSVC on Windows.
  • The conversation goes on to discuss the usability of static libraries, the possibility of detecting library compatibility, and the difficulties of linking libraries on Windows.
  • The discussion continues with considerations about library detection, manual filesystem searching, and the limitations of using pkg-config on Windows with MSVC.
  • Contributors express different opinions on the feasibility and recommended approaches for using pkg-config on Windows with MSVC, discussing potential alternatives and the challenges of handling library dependencies.

Overall, the pull request and subsequent discussion revolve around the issues and challenges related to using pkg-config on Windows with MSVC, particularly in the context of the libz-sys Rust library.


Even though it seems there are various unresolved questions or problems with supporting this, I wonder if we can trust CI (which might have improved by now) to tell us if it's working or not.

Let me fix the conflict and see what CI has to say.

@Byron
Copy link
Member

Byron commented Jun 16, 2023

CI is (still) happy, but I wonder if the build environment actually has pkg-config installed so we would see if it works or not. It's clear that if we were to merge this, we could trigger breakage throughout the ecosystem if the comment // pkg-config just never works here is still valid.

So as a path forward, I hope we could validate that that…

  • pkg-config is actually used on windows.
  • … that pkg-config will work better (now) than it does 4 years ago when it's present on the windows host.

Maybe there is also a way to merge this with an 'opt-in' toggle so those who benefit can use it, without risk of breaking the world.

@barcharcraz
Copy link

pkg-config indeed sometimes is used on windows, if you look at the vcpkg zlib package you can see it installs the .pc file and the content therein is correct and works on windows, you can verify this, install the zlib port, get pkg-config or pkg-conf in your path (I used the versions in msys2, so long as you use the ucrt/clang/mingw versions things will work, the msys2 version will probably use cygwin style paths), set PKG_CONFIG_PATH to "\lib\pkgconfig" and run pkg-config --cflags --libs --msvc-syntax zlib.

Note that the pkg-config crate doesn't pass --msvc-syntax to pkg-config, because it parses the gcc style command line itself, it does seem to support msvc when it does so however, so things should "just work"

Let me clone this branch and see what happens

@barcharcraz
Copy link

I tried this out and it seems to work fine, although I'm just looking at the build script output. which follows

cargo:rerun-if-env-changed=LIBZ_SYS_STATIC
cargo:rerun-if-changed=build.rs
cargo:rerun-if-env-changed=ZLIB_NO_PKG_CONFIG
cargo:rerun-if-env-changed=PKG_CONFIG_x86_64-pc-windows-msvc
cargo:rerun-if-env-changed=PKG_CONFIG_x86_64_pc_windows_msvc
cargo:rerun-if-env-changed=HOST_PKG_CONFIG
cargo:rerun-if-env-changed=PKG_CONFIG
cargo:rerun-if-env-changed=ZLIB_STATIC
cargo:rerun-if-env-changed=ZLIB_DYNAMIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_STATIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_DYNAMIC
cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64-pc-windows-msvc
cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64_pc_windows_msvc
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_PATH
cargo:rerun-if-env-changed=PKG_CONFIG_PATH
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64-pc-windows-msvc
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64_pc_windows_msvc
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_LIBDIR
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64-pc-windows-msvc
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64_pc_windows_msvc
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_SYSROOT_DIR
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR
cargo:rerun-if-env-changed=SYSROOT
cargo:rerun-if-env-changed=ZLIB_STATIC
cargo:rerun-if-env-changed=ZLIB_DYNAMIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_STATIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_DYNAMIC
cargo:rustc-link-search=native=C:/Users/<my name>/source/vcpkg/installed/x64-windows/lib
cargo:rustc-link-lib=zlib
cargo:rerun-if-env-changed=PKG_CONFIG_x86_64-pc-windows-msvc
cargo:rerun-if-env-changed=PKG_CONFIG_x86_64_pc_windows_msvc
cargo:rerun-if-env-changed=HOST_PKG_CONFIG
cargo:rerun-if-env-changed=PKG_CONFIG
cargo:rerun-if-env-changed=ZLIB_STATIC
cargo:rerun-if-env-changed=ZLIB_DYNAMIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_STATIC
cargo:rerun-if-env-changed=PKG_CONFIG_ALL_DYNAMIC
cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64-pc-windows-msvc
cargo:rerun-if-env-changed=PKG_CONFIG_PATH_x86_64_pc_windows_msvc
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_PATH
cargo:rerun-if-env-changed=PKG_CONFIG_PATH
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64-pc-windows-msvc
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR_x86_64_pc_windows_msvc
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_LIBDIR
cargo:rerun-if-env-changed=PKG_CONFIG_LIBDIR
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64-pc-windows-msvc
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR_x86_64_pc_windows_msvc
cargo:rerun-if-env-changed=HOST_PKG_CONFIG_SYSROOT_DIR
cargo:rerun-if-env-changed=PKG_CONFIG_SYSROOT_DIR

@barcharcraz
Copy link

I also verified that indeed the build script is finding zlib via pkg-config, not vcpkg (even though I installed zlib via vcpkg).

I'm just going to point out that chatgpt kinda got things wrong. The issue with pkg-config-rs was fixed years ago and, as nirbheek pointed out there's a very low risk of mixing msvc and mingw libraries because rustc automatically appends .lib on -msvc toolchains and .a on -gnu toolchains.

as nirbheek points out projects like gstreamer-rs really, really need pkg-config to work on windows, because they have many, many C dependencies that they want to override to use the same versions as they already build for the gstreamer C libraries.

@Byron Byron requested review from joshtriplett and JohnTitor June 23, 2023 07:07
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for validating it and for the additional context and correction, @barcharcraz! I have asked for more reviewers to hopefully be able to make a decision on this PR.

My disposition here is to merge it, as times changed since it was first opened while it seems to work correctly both for CI and for those who try it on their machines.

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I'm not familiar with Windows (development environment) and I cannot say much more than CI agrees with it.
If it doesn't break the build on the minimal environment, I think it's okay (GitHub-hosted runner has some additional setup for convenience).

@Byron
Copy link
Member

Byron commented Jul 23, 2023

I have rebased this PR onto main which contains additional tests against flate2 which gives me all the confidence I need to merge this PR once CI is green.

It's also a single line change that can easily be undone as well should serious error occour. However, since it's now more than 4 years later, and I generally think software gets better, I think the chances of this now working are higher than while this PR was new.

@Byron
Copy link
Member

Byron commented Jul 26, 2023

It turns out that even 4 years later, the comment of the removed line in 491bd1e still holds true:

// pkg-config just never works here

The removal of this line led to #143 , at least now we know.

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.

10 participants