Allow dylint-link to use the MSVC Linker

[MinerSebas]

This adds support for the MSVC Linker in dylint-link.

This uses the cc crate to get the correct Location of the Linker.

I also tried using the gnu toolchain on Windows, but there I always get man Link errors when building the examples, even without using dylint-link as the Linker:

C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: C:\Users\rust\Desktop\dylint\target\dylint\allow_clippy-44f7940205531209\release\deps\libclippy_utils-978e044f3e119bd8.rlib(clippy_utils-978e044f3e119bd8.clippy_utils.2cmv6gxw-cgu.11.rcgu.o):clippy_utils.2cmv6:(.text+0x5aea): undefined reference to `rustc_middle::hir::map::Map::body'

dylint-link and cargo-dylint did build successfully with it, so I don't know why it doesn't work.
Though it could be a personal error, as I never used the gnu toolchain before.

[smoelius]

This is awesome, @MinerSebas. Thank you.

However, this also looks like a significant change to dylint-link. So please be patient while I review it.

Quick question: the MSVC Linker free? (I couldn't tell from Googling.)

[MinerSebas]

However, this also looks like a significant change to dylint-link. So please be patient while I review it.

No Problem.

Quick question: the MSVC Linker free? (I couldn't tell from Googling.)

Yes its free. You can get it either by installing the Microsoft C++ Build Tools or using Visual Studio which includes those Build Tools.

I think I might have a Lead for the Linking Issues with the gnu toolchain.
The only example that correctly links is the clippy one.
I will test further why only this one works.

[smoelius]

I think I might have a Lead for the Linking Issues with the gnu toolchain.

Cool. Glad to hear it. Please ping me once things have stabilized.

[MinerSebas]

@smoelius The lead went nowhere, but I suspect it is a rustc Bug.

Explanation:
On Windows, the cmd line has a Limit of 8191 Characters.
If your command would exceed this you can instead use a Linker Response File to set arguments.

When doing the final Linking of the Library, only the Clippy example uses a Linker Response File and Links successfully.
All other Examples do not and fail to Link,

Manually creating a Linker Response File and providing it to the Linker also doesn't help.

[smoelius]

Thanks, for the detailed explanation @MinerSebas. I think it is fine to use just the MSVC Linker. I suspect that's what most people would use anyway.

I'll try to start reviewing this this weekend.

Have a good weekend. :)

[smoelius]

Is extract_out_path_from_linker_response_file_gnu still necessary, given that the GNU linker couldn't be made to work?

[MinerSebas]

Is extract_out_path_from_linker_response_file_gnu still necessary, given that the GNU linker couldn't be made to work?

Depends on how much support for the GNU chain is desired.

If the stance is: "Some Examples don't work -> Do not support GNU Toolchain", then it can be removed.
If it is instead: "Some Examples don't work -> You are on your own, if you get Linker errors", then I'd say that it should stay.
At least it is necessary if you want to test the Clippy Example without manually renaming the Library.

P.S. I am open to a better name for this function and its msvc sibling.

[smoelius]

First, given those two choices, I think I would prefer to not support the GNU linker at all (at least not for now).

But more generally, I am wondering if we could reduce the number of #[cfg(target_os = "windows")] that these changes require. The current number makes the code a little bit hard to follow.

Am I correct that the main difference between how the Windows and non-Windows code operates is in how the output path is found?

If so, then I am wondering if we could have a function like (roughly)

fn output_path<I>(iter: &mut I) -> Option<PathBuf>
where
    I: Iterator<Item = OsStr>,
{
    ....
}

that gets called from within dylint-link's main loop. The function examines each referenced argument, and if it specifies the output path, that path is returned. We'd have one version of this function for Windows and another for non-Windows. I think then dylint-link's original structure could be largely preserved. Moreover, there'd be just one #[cfg(target_os = "windows")] to choose an output_path implementation (and maybe one or two others).

Have I over simplified?

If you'd like me to take a crack at this (say, because my "vision" is unclear), I could.

[MinerSebas]

First, given those two choices, I think I would prefer to not support the GNU linker at all (at least not for now).

Done

But more generally, I am wondering if we could reduce the number of #[cfg(target_os = "windows")] that these changes require. The current number makes the code a little bit hard to follow.

Am I correct that the main difference between how the Windows and non-Windows code operates is in how the output path is found?

Aside from the Linker Finding, that's the major difference.

With the GNU Linker you provide with two arguments: cc -o OUTPUT_PATH
But MSVC you only use one: link /OUT:OUTPUT_PATH

Plus the existence of the Linker Response Files on Windows.

If so, then I am wondering if we could have a function like (roughly)

fn output_path<I>(iter: &mut I) -> Option<PathBuf>
where
    I: Iterator<Item = OsStr>,
{
    ....
}

that gets called from within dylint-link's main loop. The function examines each referenced argument, and if it specifies the output path, that path is returned. We'd have one version of this function for Windows and another for non-Windows. I think then dylint-link's original structure could be largely preserved. Moreover, there'd be just one #[cfg(target_os = "windows")] to choose an output_path implementation (and maybe one or two others).

Done

[smoelius]

@MinerSebas Just wanted to let you know that I haven't forgotten about this and I am actively working on it.

[smoelius]

@MinerSebas Your method for converting to UTF-16, is it from here? https://stackoverflow.com/a/57172592

[smoelius]

Also, do you recall what made the .replace("\\\"", "\"") necessary? I am asking because the files I see in my experiments don't seem to contain \".

[MinerSebas]

@MinerSebas Your method for converting to UTF-16, is it from here? stackoverflow.com/a/57172592

Yes, that was my concrete source. All others either used unsafe or provided false Results.
Should I have mentioned that as a comment?

Also, do you recall what made the .replace("\\\"", "\"") necessary? I am asking because the files I see in my experiments don't seem to contain \".

After testing I can confirm that the .replace("\\\"", "\"") is unnecessary.

The Source was the Console Output during debugging:

As you can see, the console showed the actual " as \" (and \ as \\, etc.), which made me think I needed to convert it back to a singular ".

[smoelius]

I like to cite sources, but you had no way of knowing that.

I am making a few changes, so no need to push anymore. I almost have the tests passing. I think we're close to being able to say we support Windows, thanks to you!!! 🙏🙏🙏

[smoelius]

OK, the tests pass locally on my VM.

@MinerSebas When you have a moment, please take a look at eabd298. Mostly, I changed things in ways that I think makes them more idiomatic. In a few places, I changed how/whether errors could occur. Please let me know if there is anything you don't like or object to.

Of course, you're welcome to review the other commits to (but only if you want to).

[MinerSebas]

Everything works as expected. I just have a minor Nitpick.

You added several Links which immediately succeeded by a Period.
The refined-github browser extension made the Links clickable, but saw that period as part of the URL and opened an Invalid Link.
I suspect a "careless" copy-paste would have the same problem.

In comparison, VSCode correctly identifies that the period isn't part.

I'd say either put a space between the URL and period or remove it completely.
(Or leave it, after all, it's just a Nitpick, even if the Lenght might convince you otherwise)

[smoelius]

The refined-github browser extension made the Links clickable, but saw that period as part of the URL and opened an Invalid Link.

Oof. Not a nitpick. Did you know of any that I missed?

[MinerSebas]

You only missed one in the .gitattributes File.
A quick search through the whole Repo for https:// Links found no other "broken" ones.

[smoelius]

Cool.

I'm going to push a new release soon. This is a significant milestone. I really, really appreciate all of your hard work on this, @MinerSebas. :)

[smoelius]

Just FYI, this is published now: https://crates.io/crates/dylint/1.0.2