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

Consider consolidating the msvc and gnullvm target import libs #2018

Closed
glandium opened this issue Sep 13, 2022 · 16 comments
Closed

Consider consolidating the msvc and gnullvm target import libs #2018

glandium opened this issue Sep 13, 2022 · 16 comments
Labels
enhancement New feature or request

Comments

@glandium
Copy link
Contributor

It looks like the gnullvm targets are still there. Should they be removed now?

Originally posted by @kennykerr in #2016 (comment)

There are two ways to go about this:

  • Make the gnullvm targets use the msvc target crates (as in Use the msvc target crates for gnullvm targets #1961)
  • Make the msvc targets use the gnullvm target crates (which in essence would amount to the same thing as having rust auto-generate the import libraries via raw-dylib)

An advantage of the latter is that it allows the generation of all the import libraries through a single command (vs. at least 4 right now, 3 of which are with 3 different Visual Studio environments).

The difference between the gnullvm and msvc import libraries are outlined in #1964 (comment).

@kennykerr
Copy link
Collaborator

So we could support gnu/gnullvm/msvc all with a single lib per target?

@mati865
Copy link
Contributor

mati865 commented Sep 13, 2022

You mean per arch?

Technically yes if we use legacy import lib format (like the one used by gnu).
To use short import libraries (like LLVM and MSVC does) we'd need to workaround issue recently fixed on Binutils master: bminor/binutils-gdb@46bbc1c
But using common library for LLVM and MSVC and second one for GNU is achievable.

@kennykerr
Copy link
Collaborator

Interesting. As raw-dylib is nearing stabilization, I don't see a compelling reason to push this much further but we probably need to be able to support a few more releases with explicit libs. We haven't published a release in a few months, so I'd like to settle on something pretty quickly. It looks like we've resolved all the linker issues for APIs, but the repo is not in a happy state as the gnullvm targets are still present but the tool to generate them was removed in #2016.

In the short term we could just remove the orphaned gnullvm targets, publish a release, and then consolidate them somehow.

@mati865
Copy link
Contributor

mati865 commented Sep 13, 2022

but the repo is not in a happy state as the gnullvm targets are still present but the tool to generate them was removed in #2016.

Tool to generate gnullvm was merged with gnu (so we are mostly back to how I originally made them 😄) so it generates both gnullvm and gnu target crates.

@glandium
Copy link
Contributor Author

So we could support gnu/gnullvm/msvc all with a single lib per target?

Technically, that's possible, but I was more thinking about two libs per cpu-type: gnu/msvc or gnu/gnullvm.

As raw-dylib is nearing stabilization

raw-dylib may be near stabilization, but it's not desirable to use it as soon as it is stable, as that would mean a forced MSRV that many prospective users of the crate would not appreciate.

@glandium
Copy link
Contributor Author

You mean per arch?

Technically yes if we use legacy import lib format (like the one used by gnu). To use short import libraries (like LLVM and MSVC does) we'd need to workaround issue recently fixed on Binutils master: bminor/binutils-gdb@46bbc1c But using common library for LLVM and MSVC and second one for GNU is achievable.

Does this imply binutils actually supports short import libs, provided the archive members are named a different way?

@mati865
Copy link
Contributor

mati865 commented Sep 13, 2022

Does this imply binutils actually supports short import libs, provided the archive members are named a different way?

Binutils have sort of supported short import libs for quite some time, it just had multiple bugs. So this would require some testing with oldest Binutils version that windows-rs wants to support but it might be possible to make it work that way. Not sure if it's work the hassle though.

@riverar
Copy link
Collaborator

riverar commented Sep 14, 2022

To use short import libraries (like LLVM and MSVC does) we'd need to workaround issue recently fixed on Binutils master: bminor/binutils-gdb@46bbc1c

This code incorrectly assumes libs always point to ".dlls" when they can also point to .cpl, .ax, etc. We may want to reach out and get this fixed.

@kennykerr
Copy link
Collaborator

raw-dylib may be near stabilization, but it's not desirable to use it as soon as it is stable, as that would mean a forced MSRV that many prospective users of the crate would not appreciate.

For windows-sys that is unfortunately the reality as it is geared toward systems libraries. For windows we generally push the language as fast as practical as adoption is generally more focused on applications rather than libraries and already requires pretty recent features necessitating a MSRV of 1.61 at present.

But yes, I would expect we have to keep supporting libs for some time to come if only for windows-sys.

@mati865
Copy link
Contributor

mati865 commented Sep 24, 2022

To use short import libraries (like LLVM and MSVC does) we'd need to workaround issue recently fixed on Binutils master: bminor/binutils-gdb@46bbc1c

This code incorrectly assumes libs always point to ".dlls" when they can also point to .cpl, .ax, etc. We may want to reach out and get this fixed.

I don't have contacts within Binutils so feel free to reach out to them.

@kennykerr
Copy link
Collaborator

Sounds like the tooling's not quite there yet. Can we close this issue?

@mati865
Copy link
Contributor

mati865 commented Sep 28, 2022

Binutils issue prevents windows-rs from using single import lib for each arch but reducing to 2 import libs per arch is doable (gnu + gnullvm/msvc) if @glandium wants to continue working on this.

@kennykerr
Copy link
Collaborator

I'm not sure I'm ready to ditch the msvc tooling entirely if that's what is required. It's clunky but it is probably the only one the Visual Studio team officially supports for our internal customers.

@mati865
Copy link
Contributor

mati865 commented Sep 29, 2022

Do you mean you want to keep generating import libraries in target crates {arch}_msvc with MSVC instead of LLVM?
I think LLVM would be as safe option as MSVC especially considering the fact Rust already uses LLVM to generate import libraries when using raw-dylib feature with MSVC targets but naturally you are the maintainer here.

There is also option for using {arch}_msvc crates (perhaps with adjusted name?) for gnullvm targets which according to #1964 (comment) shouldn't cause problems.

@kennykerr
Copy link
Collaborator

Part of my reluctance has to do with the current experience for generating the gnu/llvm libs using mingw which I have not been able to run successfully and is not very Windows-friendly. Agreed that LLVM itself is part of the toolchain so I don't have a problem with that in itself. Happy to continue to make progress here - just a little cautious I suppose. 😉

@kennykerr
Copy link
Collaborator

Closing for now. Feel free to keep the conversation going - love the improvements thus far!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants