bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Use rust-lld on windows rustdoc in config_fast_builds.toml

Open SkiFire13 opened this issue 1 year ago • 16 comments

Objective

  • Rustdoc doesn't seem to follow cargo's linker setting
  • Improves the situation in #12207

Solution

  • Explicitly set the linker in rustdoc flags

Testing

  • I tested this change on Windows and it significantly improves testing performances (can't give an exact estimate since they got stuck before this change)

Note: I avoided changing the settings on Linux and MacOS because I can't test on those platforms. It would be nice if someone could test similar changes there and report so they can be done on all major platforms.

SkiFire13 avatar May 28 '24 12:05 SkiFire13

Note: I avoided changing the settings on Linux and MacOS because I can't test on those platforms. It would be nice if someone could test similar changes there and report so they can be done on all major platforms.

I tested using LLD on a MacOS M1 chip, and it was slower. This is due to Apple's default ld64 being faster. I still think it should be kept for Windows, though, due to the performance gains.

If possible, could someone test on x86 MacOS and x86 Linux? I'd used:

[target.x86_64-apple-darwin]
rustflags = [
  # ...
]
rustdocflags = ["-Clink-arg=-fuse-ld=/usr/local/opt/llvm/bin/ld64.lld"]

[target.x86_64-unknown-linux-gnu]
linker = "clang"
rustflags = [
  # ...
]
rustdocflags = ["-Clink-arg=-fuse-ld=lld"] # Also try testing the Mold linker, too!

BD103 avatar May 28 '24 13:05 BD103

This file is here for Bevy users to set up their own projects, not really for Bevy contributors. Nowhere are they pointed to this file. This should go into the contributor guide rather than here.

I doubt we will have a lot of tests available for x86 macs, I'm ok with not mentioning them

mockersf avatar May 28 '24 19:05 mockersf

This file is here for Bevy users to set up their own projects, not really for Bevy contributors.

Why does it have an alias for cargo run -p ci then?

That said, would it make sense to split it into a config for contributors and one for users?

SkiFire13 avatar May 28 '24 21:05 SkiFire13

This file is here for Bevy users to set up their own projects, not really for Bevy contributors.

Why does it have an alias for cargo run -p ci then?

Because I didn't saw the PR that added that and don't think it should have been merged. This file doesn't have the default name specifically to not be used by Bevy contributors automatically

mockersf avatar May 28 '24 21:05 mockersf

Why does it have an alias for cargo run -p ci then?

I added that feature because I thought config_fast_builds.toml was targeted towards contributors. Honestly I still feel this way, and that a user-oriented version should be in the documentation instead.

BD103 avatar May 29 '24 01:05 BD103

@mockersf I agree with @janhohenheim: this is useful for Bevy users directly. Faster doc tests are valuable for library authors, especially since this is a super subtle footgun.

alice-i-cecile avatar Jul 01 '24 14:07 alice-i-cecile

Marking this as "Needs Testing:" someone with an x86-64 Windows machine should test the configuration that @janhohenheim used.

BD103 avatar Jul 03 '24 17:07 BD103

@janhohenheim which nightly version are you using (since you specified nightly-only flags)?

Also, are you testing that doctest in isolation (i.e. in its own crate and workspace, without dependencies) or are you testing it from inside Bevy? It would be nice if this was reproducible with a small crate so the root cause can be more easily identified.

SkiFire13 avatar Jul 03 '24 18:07 SkiFire13

@SkiFire13 nightly 0.18.0 Setting up a minimal crate right now.

@BD103 turns out my doctest does not run with dynamic linking even when my .cargo/config.toml is entirely empty, so this definitely has nothing to do with this PR. I'll open a new issue once I have the minimal example ready. Feel free to remove the "Needs Testing" label and merge, if you want.

janhohenheim avatar Jul 03 '24 19:07 janhohenheim

@SkiFire13 nightly 0.18.0

I'm not sure what you're talking about... Could you post the output of rustc +nightly --version?

@BD103 turns out my doctest does not run with dynamic linking even when my .cargo/config.toml is entirely empty, so this definitely has nothing to do with this PR. I'll open a new issue once I have the minimal example ready.

On windows if you enable the dynamic_linking feature you also need to enable optimizations (which are disabled by default for tests)

SkiFire13 avatar Jul 03 '24 19:07 SkiFire13

@SkiFire13 sorry, I was typing Bevy 0.14.0 so many times over the last days that I automatically started my version by typing 0.1, haha. I mean 1.80.0 :)

Wait, so this snippet here is ignored by tests?

# Enable a small amount of optimization in debug mode
[profile.dev]
opt-level = 1

# Enable high optimizations for dependencies (incl. Bevy), but not for our code:
[profile.dev.package."*"]
opt-level = 3

janhohenheim avatar Jul 03 '24 19:07 janhohenheim

@SkiFire13 sorry, I was typing Bevy 0.14.0 so many times over the last days that I automatically started my version by typing 0.1, haha. I mean 1.80.0 :)

That's kinda old, so I guess this is not some new rustc bug.

Wait, so this snippet here is ignored by tests?

That applies to the dev profile, which is the one used by cargo build/cargo run when not specifying --release. cargo test uses the test profile instead, so you need to also do this:

[profile.test]
opt-level = 1

[profile.test.package."*"]
opt-level = 3

SkiFire13 avatar Jul 03 '24 19:07 SkiFire13

@SkiFire13 that sounds very much like my issue then, thanks. I'll recompile everything with those settings and see if that fixes it. It will take a moment since I've disabled LLD, sccache, etc. for this.

janhohenheim avatar Jul 03 '24 19:07 janhohenheim

@SkiFire13 Here's a minimal repo of the issue. Could you confirm whether running cargo test --doc on it works on Windows? I tried adding the [profile.test] parts, but they did not help. In any case, I noticed that if the optimizations were the problem, I would probably have gotten a build-time error.

janhohenheim avatar Jul 03 '24 20:07 janhohenheim

@SkiFire13 Here's a minimal repo of the issue. Could you confirm whether running cargo test --doc on it works on Windows? I tried adding the [profile.test] parts, but they did not help. In any case, I noticed that if the optimizations were the problem, I would probably have gotten a build-time error.

@janhohenheim I can confirm that this results in the Test executable failed (exit code: 0xc0000135) error for me as well (tested with an empty .cargo/config.toml and no RUSTFLAGS or RUSTDOCFLAGS set).

I've also managed to further minimize the issue to remove the dependency on Bevy. You only need a crate depending on a dylib crate, and a doctest that uses any item from the dylib crate. https://github.com/SkiFire13/minimal_dylib_doctest_fail

I've also found this rust issue which seems to fit the problem https://github.com/rust-lang/rust/issues/39487. It has been closed in favour of https://github.com/rust-lang/rustup/issues/893, where it's mentioned that the issue is with the stdilb dll not being added to the path when running the test. Indeed if I either add %RUSTUP_HOME%\toolchains\stable-x86_64-pc-windows-msvc\lib\rustlib\x86_64-pc-windows-msvc\lib\ to the PATH or copy the std-****.dll file in that directory to the directory where I'm running cargo test --doc then it works. However I wonder why this issue doesn't appear on Linux (I've tested on WSL and the same minimalized reproduction works fine without the workaround).

SkiFire13 avatar Jul 04 '24 07:07 SkiFire13

@SkiFire13 thanks for the investigation! I'll open a new issue later. We should probably add this info somewhere so that new users don't fall into this trap.

Edit: opened up #14129

janhohenheim avatar Jul 04 '24 07:07 janhohenheim