Skip to content

Use lld for rustdoc on Linux in config_fast_builds.toml #14839

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 22, 2024
Merged

Use lld for rustdoc on Linux in config_fast_builds.toml #14839

merged 4 commits into from
Aug 22, 2024

Conversation

roblesch
Copy link
Contributor

Objective

Running cargo ci on Fedora 40 causes a system crash due to many ld processes consuming all available memory when performing doc tests.

Solution

This PR extends #13553.

Testing

Crashing configuration

  • config.toml 
    [target.x86_64-unknown-linux-gnu]
linker = "clang"
rustflags = ["-Clink-arg=-fuse-ld=lld"]

[alias]
ci = "run --package ci --"
  • Test command
    cargo ci

Working configuration

  • config.toml 
    [target.x86_64-unknown-linux-gnu]
linker = "clang"
rustflags = ["-Clink-arg=-fuse-ld=lld"]
rustdocflags = ["-Clink-arg=-fuse-ld=lld"]

[alias]
ci = "run --package ci --"
  • Test command
    cargo ci

@github-actions GitHub Actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@TrialDragon TrialDragon added C-Bug An unexpected or incorrect behavior A-Build-System Related to build systems or continuous integration O-Linux Specific to the Linux desktop operating system S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 21, 2024
@TrialDragon
Copy link
Member

TrialDragon commented Aug 21, 2024 •
edited
Loading

This is why my system crashed! I just figured I didn't have the specs to test Bevy locally. I'm going to test & review this ASAP. Thank you for working to fix this.

@TrialDragon
Copy link
Member

TrialDragon commented Aug 21, 2024 •
edited
Loading

Hmm, I still froze my computer and effectively crashed it with this PR on Fedora 40.

I completely missed that I need to add the config.toml myself, I guess. I'll test this, properly this time, in the morning.

BD103
BD103 reviewed Aug 21, 2024
View reviewed changes
.cargo/config_fast_builds.toml Outdated
Comment on lines 158 to 159
# Some systems may experience linker performance issues when running doc tests.
# See https://github.com/bevyengine/bevy/issues/12207 for details.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right place for this comment. From how I read it, it looks like you're commenting on the alias and not rustdoc itself. I would either move it to where you specify rustdocflags, or remove it entirely.

.cargo/config_fast_builds.toml
@@ -83,12 +83,19 @@ rustflags = [
# - Ubuntu: `sudo apt-get install mold clang`
# - Fedora: `sudo dnf install mold clang`
# - Arch: `sudo pacman -S mold clang`
# "-Clink-arg=-fuse-ld=/usr/bin/mold",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! When I wrote this I didn't realize /usr/bin was automatically in the PATH.

CONTRIBUTING.md
@@ -423,7 +423,7 @@ If you're new to Bevy, here's the workflow we use:
2. Make your changes in a local clone of your fork, typically in its own new branch.
1. Try to split your work into separate commits, each with a distinct purpose. Be particularly mindful of this when responding to reviews so it's easy to see what's changed.
2. Tip: [You can set up a global `.gitignore` file](https://docs.github.com/en/get-started/getting-started-with-git/ignoring-files#configuring-ignored-files-for-all-repositories-on-your-computer) to exclude your operating system/text editor's special/temporary files. (e.g. `.DS_Store`, `thumbs.db`, `*~`, `*.swp` or `*.swo`) This allows us to keep the `.gitignore` file in the repo uncluttered.
3. To test CI validations locally, run the `cargo run -p ci` command. This will run most checks that happen in CI, but can take some time. You can also run sub-commands to iterate faster depending on what you're contributing:
3. To test CI validations locally, run the `cargo run -p ci` command. This will run most checks that happen in CI, but can take some time. Some systems may experience [linker performance issues when running doc tests](https://github.com/bevyengine/bevy/issues/12207). You can also run sub-commands to iterate faster depending on what you're contributing:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're slowly moving information from here to the Contributing Guide. I'm fine with this being added for now, but once bevyengine/bevy-website#1605 is merged we should copy this information over.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting this may be fixed if rust-lld makes it into 1.82/rust 2024; this issue no longer occurs on nightly.

@roblesch
Copy link
Contributor Author

roblesch commented Aug 21, 2024 •
edited
Loading

This is why my system crashed! I just figured I didn't have the specs to test Bevy locally. I'm going to test & review this ASAP. Thank you for working to fix this.

If you still run out of memory after switching your linker, you may also need to throttle the test threads e.g. cargo ci --test-threads=4

Edit: --test-threads isn't a valid input to the ci package, but you could run the ci doctests command directly with cargo test --workspace --doc -- --test-threads=4

TrialDragon
TrialDragon approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@TrialDragon TrialDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, tested properly and it works 🎉 . Once the website's Contributing Guide is published, I'll get this mention moved ASAP since I feel this is pretty core to being able to properly contribute for some people (like me).

@TrialDragon TrialDragon added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 21, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Aug 22, 2024 •
edited
Loading

Can you remake the advice parts of this PR on the bevy-website repo? The contributing book is live now, and I'd like to get this in.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 22, 2024
Merged via the queue into bevyengine:main with commit 35fb54f Aug 22, 2024
27 checks passed
@roblesch roblesch deleted the doc_tests_linker_tip branch August 23, 2024 06:28
@alice-i-cecile alice-i-cecile mentioned this pull request Aug 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TrialDragon TrialDragon TrialDragon approved these changes

@BD103 BD103 BD103 approved these changes

Assignees
No one assigned
Labels
A-Build-System Related to build systems or continuous integration C-Bug An unexpected or incorrect behavior O-Linux Specific to the Linux desktop operating system S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@roblesch @TrialDragon @alice-i-cecile @BD103