-
Notifications
You must be signed in to change notification settings - Fork 469
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
build passes invalid -nologo
and -out:
flags to ar
tool if specified in AR
env var.
#762
Comments
Here's the stdout of the full build: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8795327228707936417/+/u/package_rust/stdout And of interest this part:
|
ar
tool if specified in AR
env var.-nologo
and -out:
flags to ar
tool if specified in AR
env var.
Thanks for the report, this is indeed a problem that needs solving. If you do have any ideas for a patch then PRs are very much welcome! This crate recently changed to new maintainers who are still working through existing issues so any help at this time would be very much appreciated. I think the problem here is that it needs to know what flavour of arguments to pass? So maybe it'd be enough to inspect the executable name? |
Hans pointed out on the Chromium CL linked above that using However, in our case we are using So I can set our So this may transform the issue to $ARFLAGS is not honoured. I will report back next week probably (testing this takes a long time for me). |
No, set your |
Does it mean that you use it elsewhere? And/or that you want to use it here? Or that llvm-lib is not an option in your installation? |
In case |
FBOFW llvm-lib is not available in the Chromium dev environment, we build and distribute lld-link, which includes llvm-lib functionality. Right, my first problem was finding link.exe and it's working now. Typically we don't use that at all - but since it's required for Rustc (thanks for confirming that), there's perhaps no harm in using it for the C sources inside the Rust compiler/stdlib build. |
No, disregard this remark. It would work if you use |
To clarify, we're using clang (I need to change it to Lines 2670 to 2689 in 0e51f6d
|
I don't quite follow. Who links your final executable? If rustc, then it's supposed to find it all by itself, without you looking for it. And by extension cc-rs should find lib.exe if you direct it toward it. As for Rust using link.exe. @ChrisDenton has pointed out [elsewhere] that it is possible to make rustc use lld-link instead, but it's not obvious that it would pass as an officially supported option :-) |
We control the linker with rustc inside our chromium build, but for building the toolchain I am trying to not ruffle too many feathers inside the workings of x.py. So for the purposes of x.py, which is using the
I think either one of those requires specifying it in |
We have to recognize that the decision to use link.exe is rustc's own, cc-rs has no say in it. [Even the possibility to use lld-link is outside cc-rs's control.] cc-rs only compiles C and collects them to libraries, that's all(*). To do that it needs a compiler and a librarian, it doesn't need no linker. In other words, cc-rs in a way "distance" itself from linker :-) Which is why the suggestion to set AR to a linker sounds so to say "alien"... As for llvm-lib not being available. Kind of unfair, don't you think? :-) But on a more practical note, is not setting AR an option? Despite the "disregard" note, it does appear to work... [In case you wonder, the original concern was that (*) Well, it also locates MSVC installation[s], but it's beside the point. |
Ok so by great luck I have determined that However:
It may be safe to assume any linker on Windows must accept link.exe format arguments ( And I should note, for the curious, why this isn't a problem in Chromium builds in general, where we have no |
Are you looking for a street fight or something? :-) :-) :-) But on a serious note. ARFLAGS is a |
lol, I see, that's fair.. that there's maybe code with ARFLAGS set not expecting to have it used now. So that would be API breaking. I guess a special var could be used instead like CC_RUST_ARFLAGS or something but it's a bit much and it's not needed it looks like for Chromium so maybe best to put this on ice for now, but hopefully the info is helpful should someone else run into it. |
One can probably make a case for defaulting for |
See the discussion on: https://chromium-review.googlesource.com/c/chromium/src/+/4087043 And the bug: rust-lang/cc-rs#762 [email protected] Bug: 1271215 Change-Id: I92f862acace5bb46e2488bb06d063e77c97b2cb0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4089656 Reviewed-by: Hans Wennborg <[email protected]> Commit-Queue: danakj <[email protected]> Cr-Commit-Position: refs/heads/main@{#1081764}
I apologise for my absence from this conversation. I've had a busy couple of months and it's taken me awhile to catch up. Some notes. We do support Lines 2697 to 2707 in e5bbdfa
We also do some work to detect the right ar to use (comments are confusing because the code is shared with that which finds ranlib): Lines 2790 to 2822 in e5bbdfa
It's notable that Lines 2121 to 2143 in e5bbdfa
I think the least invasive change we could make is to make the above |
#763 has also worked to address this issue and should have tagged this probably. Specifically it now allows setting |
Ah, thanks for pointing that out. As I said I'm only just catching up on things. |
Found this here: https://chromium-review.googlesource.com/c/chromium/src/+/4087043/4//COMMIT_MSG#26
The
AR
env var can override the tool:cc-rs/src/lib.rs
Lines 2651 to 2653 in 0e51f6d
However the flags passed to it include "-nologo" and
-out:
which then fails with invalid flag.cc-rs/src/lib.rs
Lines 2107 to 2111 in 0e51f6d
It seems like if AR is specified in the env var, it shouldn't assume it can change the arguments based on the target being msvc?
The text was updated successfully, but these errors were encountered: