fix: lld-link (MSVC) fix flags including -l prefix

[rickvanprim]

Fixes rustc passing a verbatim -l<library name> to lld-link (or link.exe) instead of just passing the library name.

[rickvanprim]

Just to make sure this isn't a discrepancy between lld-link and link.exe, I just tested:

> link.exe -lfoo.lib
Microsoft (R) Incremental Linker Version 14.35.32217.1
Copyright (C) Microsoft Corporation.  All rights reserved.

LINK : warning LNK4044: unrecognized option '/lfoo.lib'; ignored
LINK : warning LNK4001: no object files specified; libraries used
LINK : warning LNK4068: /MACHINE not specified; defaulting to X64
LINK : fatal error LNK1561: entry point must be defined

[UebelAndre]

Thanks so much for working on this!

Do you know if this correct for both msvc and gnu windows platforms?

[rickvanprim]

Good catch @UebelAndre. You're right this is wrong for the gnu version (just checked locally and it wants to invoke x86_64-w64-mingw32-gcc). I'm thinking that instead of checking OS, checking the target triple for the ABI msvc might be a better approach. I'm not sure how I'd get that in native_deps_test.bzl, so I made the tests use the prefix+suffix match so look for -Clink-arg= + <libname>. Seems like the best way to detect it would be if @platforms had an OS entry for mingw, but that's a much larger scope 😅

While I was working on this, I noticed that a similar incompatible flag is being passed to GCC:

if lib.alwayslink:
    ret.extend(["-C", "link-arg=/WHOLEARCHIVE:%s" % get_preferred_artifact(lib, use_pic).path])

Just a thought, but it might be easier to eventually structure the code around "linker flavor" than "Windows"/"Darwin"/"Default".

[UebelAndre]

Style nit: Could you make a list that you conditionally append the right thing into? This conditional seems kinda long.

[rickvanprim]

Will do. Is the msvc_flavor change okay or would it be better to curry in a reference to toolchain and get rid of for_windows and for_darwin in favor of target_triple checks?

[UebelAndre]

Hi! Would you be willing to do a rebase of these changes? 🙏

[rickvanprim]

@UebelAndre of course 🙂 I'm just looking for guidance on:

1. "Get the toolchain" (or target_triple, etc.) inside of the tests.
2. With target_os how to gate for msvc. I'm assuming target_os governs for_windows and then msvc = target_triple && target_triple.abi == "msvc".

[UebelAndre]

@UebelAndre of course 🙂 I'm just looking for guidance on:

1. "Get the toolchain" (or `target_triple`, etc.) inside of the tests.

2. With `target_os` how to gate for `msvc`.  I'm assuming `target_os` governs `for_windows` and then `msvc = target_triple && target_triple.abi == "msvc"`.

1. You can add a _rust_toolchain attribute to the rule that defaults to //rust/toolchain:current_rust_toolchain and you can index that should match the toolchain the rules were built with.
2. I think this code is the one place that needs to be aware of both exec and target triples. My understanding is the bug you're seeing is that we need to format flags differently based on what linker will be used. I believe the exec_triple would represent what linker would be used. But in terms of what to link, that would still be target_triple. Though, there's still a chance target_triple is not set so there may need to be a target_env parameter added that represents this. Then you could detect msvc where needed? But if the target platform doesn't matter exec_triple is now mandatory and can be treated as a source of truth. Simply check rust_toolchain.exec_triple.abi wherever you need to know this info.

[rickvanprim]

@UebelAndre in theory I've updated this as discussed. Crate Universe Examples on Windows is failing for reasons I'm not sure about. My attempts to build locally on Windows fail on main right now (though the PR that merged the latest commit passed on it). Not sure if you have any insight? I'll rebase and try again if I see any new commits to main.

[UebelAndre]

Do you have a way to test cross compiling from, say exec_triple = "x86_64-pc-windows-msvc" to target_triple = "x86_64-pc-windows-gnu"? I'm curious if the use of target_os is correct. I suspect the exec_triple needs to be taken under consideration.

As for the build failure, looks like some infra failure? I'm not sure, if you make any new commit then it'll trigger new builds. If it doesn't pass then then I can ping some folks.

[rickvanprim]

Doesn't seem like an infra thing given that it fails for me locally. I'm not sure why Buildkite isn't showing any useful logs. This is the error I get locally (running on main):

ERROR: C:/users/<user>/_bazel_<user>/rocexn33/external/rules_rust_rust_analyzer__anyhow-1.0.68/BUILD.bazel:51:19: Running Cargo build script anyhow failed: (Exit 101): cargo_build_script_runner.exe failed: error executing command (from target @rules_rust_rust_analyzer__anyhow-1.0.68//:anyhow_build_script) bazel-out\x64_windows-opt-exec-2B5CBBC6\bin\cargo\cargo_build_script_runner\cargo_build_script_runner.exe ... (remaining 10 arguments skipped)
thread 'main' panicked at 'Unable to start binary: Os { code: 267, kind: NotADirectory, message: "The directory name is invalid." }', cargo/cargo_build_script_runner/lib.rs:127:41
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The expanded command is:

bazel-out\x64_windows-opt-exec-2B5CBBC6\bin\cargo\cargo_build_script_runner\cargo_build_script_runner.exe
bazel-out/x64_windows-opt-exec-2B5CBBC6/bin/external/rules_rust_util_import__memchr-2.5.0/memchr_build_script_.exe
bazel-out/x64_windows-fastbuild/bin/external/rules_rust_util_import__memchr-2.5.0/memchr_build_script.out_dir
bazel-out/x64_windows-fastbuild/bin/external/rules_rust_util_import__memchr-2.5.0/memchr_build_script.env
bazel-out/x64_windows-fastbuild/bin/external/rules_rust_util_import__memchr-2.5.0/memchr_build_script.flags
bazel-out/x64_windows-fastbuild/bin/external/rules_rust_util_import__memchr-2.5.0/memchr_build_script.linkflags
bazel-out/x64_windows-fastbuild/bin/external/rules_rust_util_import__memchr-2.5.0/memchr_build_script.linksearchpaths
bazel-out/x64_windows-fastbuild/bin/external/rules_rust_util_import__memchr-2.5.0/memchr_build_script.depenv
bazel-out/x64_windows-fastbuild/bin/external/rules_rust_util_import__memchr-2.5.0/memchr_build_script.stdout.log
bazel-out/x64_windows-fastbuild/bin/external/rules_rust_util_import__memchr-2.5.0/memchr_build_script.stderr.log

And none of those files exist. There is a directory under opt-exec with a runfiles manifest but nothing else. Searching through the build output, there is no memchr listed anywhere.

Regarding testing x86-64-pc-windows-gnu from x86_64-pc-windows-msvc, I think I can do that with bazel build --extra_toolchains=@rust_windows_x86_64__x86_64-pc-windows-gnu__stable//:toolchain //.... This is failing presently on main as well with some missing symbols trying to link process_wrapper. Once the first issue is sorted out, I'll try again.

I don't suppose any of this is ringing any bells? I can try to debug further if necessary.

[UebelAndre]

The error you're getting looks different. I filed #1965 to try and get CI fixed and seems like that might be the case? Wanna push one more commit and find out?

[UebelAndre]

Awesome work! Thank you so much!