Skip to content
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

Updated 'Enable Fast Compiles' section #137

Closed
wants to merge 30 commits into from

Conversation

lukors
Copy link

@lukors lukors commented Apr 23, 2021

I found this section a bit confusing and ran into some issues when I tried following it. So my goal with this PR is to make it clearer and less error prone.

  • Split it up into two sections to clearly show that you can either do this one simple step, or this one simple step AND these other more involved steps.
  • Turned the 3 more involved steps into a numbered sequence to show clearly that you can't do just one or two, you have to do all three. Which technically you don't, but if you skip a step you would need to edit the config file in the end, and if you skip the config file (step 3) you won't get much benefit. So they make sense as a sequence.
  • Added clang to the sudo apt-get install for Ubuntu as it was required for me on Pop!_OS 20.10 (based on Ubuntu 20.10).
  • Cleared up the final step, its headline was "Generic Sharing", but that's not a step, the actual step is to configure cargo, which to my understanding both activates Generic Sharing as well as LLD.


* **Enable Bevy's Dynamic Linking Feature**: This is the biggest iterative compilation time win and requires no special setup. When you have `bevy` as a dependency you can run your app with dynamic linking like this:
* **Bevy's Dynamic Linking Feature**: This is the biggest iterative compilation time win and requires no special setup. When you have `bevy` as a dependency you can run your app with dynamic linking like this:
Copy link
Contributor

@MinerSebas MinerSebas Apr 23, 2021

Choose a reason for hiding this comment

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

This should probably also mention that this feature doesn't always work on Windows.
At least it doesn't when you also use the Zshare-generics config: bevyengine/bevy#1126 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Lets make sure we close #131 if you make this change.

Copy link
Member

Choose a reason for hiding this comment

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

I think it probably makes sense to add this to the "configure cargo" section instead of to the "dynamic linking" section, just to keep the dynamic linking section simple.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I believe my recent changes resolves this.

@cart
Copy link
Member

cart commented Apr 23, 2021

This looks good to me! Once we add the "windows + dynamic linking + share generics" stipulation i think its good to go.

@lukors
Copy link
Author

lukors commented Apr 26, 2021

I've looked into this a bit closer and tested it on my Windows machine in an attempt to come up with an as simple solution as possible for people who follow the guide.

I made a pull request to turn off the "share generics" option in the config.toml for Windows users, since it doesn't seem to work (bevyengine/bevy#2016). This removes the need to mention this in the actual guide since that file should then work for every OS.

"Share generics" doesn't work on Windows, so the only gains to get for Windows users are the LLD linker, bevy/dynamic, and any optimizations nightly brings. bevy/dynamic doesn't work without the config.toml file containing the setting that turns off "share generics", and that option requires nightly to be used. So my conclusion is that as a Windows user you might as well follow the entire guide.

So the only changes I've made to the guide is to add a note for windows users at the top saying they need to follow the numbered steps for bevy/dynamic to work, and I've noted where some something is different for Windows users.

@lukors
Copy link
Author

lukors commented Apr 26, 2021

Thanks cart and MinerSebas! I believe this is ready for another look!

* **Enable Bevy's Dynamic Linking Feature**: This is the biggest iterative compilation time win and requires no special setup. When you have `bevy` as a dependency you can run your app with dynamic linking like this:
If you're using Windows, Bevy's dynamic linking feature won't work unless you've followed the three extra steps below first.

* **Bevy's Dynamic Linking Feature**: This is the biggest iterative compilation time win and requires no special setup unless you use Windows. When you have `bevy` as a dependency you can run your app with dynamic linking like this:
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually require special setup on windows? Shouldn't it work with the default setup? If this is just a reference to the "share generics" issue, we should just cover that in the "fast compiles configuration section", either by removing it like you did in your pr or by documenting the weirdness / potential errors in the fast compiles section.

I'd prefer it if the "dynamic linking" section was "simple and asterisk free"

Copy link
Author

@lukors lukors Apr 27, 2021

Choose a reason for hiding this comment

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

tl;dr: My understanding is that the problem is the share-generics issue. It seems like share-generics are on by default in debug compilation, so that's why it doesn't work on Windows without special setup.


I did some more testing to make sure if special setup is required, and in my tests I run into some variation of the errors linked in #131 when running with --features bevy/dynamic on Windows unless I run with..

Option 1

  • rustflags = ["-Zshare-generics=n"]
  • Nightly (required because of the -Z flag in rustflags)

Option 2

  • --release

Like you write, share-generics is off by default, but only with --release, otherwise it's apparently on by default. This is where I got this information rust-lang/rust#67276 and I've verified it on my machine by successfully doing cargo +stable run --features bevy/dynamic --release with no config.toml file. Incremental compiles with --features bevy/dynamic --release is slower for me than building in debug without bevy/dynamic.

So, following option 1, with nightly and the config file you've already followed 2 of the 3 "advanced" points, so you might as well do the remaining step of using LLD as well. If nothing else, just to keep the instructions simple.

I also want to keep these instructions as simple as possible, and it sucks to have the extra info for Windows users. But it seems to me like bevy/dynamic simply does not work on Windows unless you at least compile on nightly and with rustflags = ["-Zshare-generics=n"].

I thought the share-generics was an opt-in nightly thing, but yeah, I don't know.

I want to add that I don't know what I'm actually doing here, so take anything I present with a grain of salt. I'm just working off my own tests, some Internet searching, a lot of assumptions and no prior experience with Rust nightly or config.toml files. So thanks for probing to make sure this documentation becomes as good as possible! :)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the breakdown! I'll run a few tests on my machine just to get a feel for the various permutations / see if these behaviors are reproducible.

It does sound like we'll probably need to roll with the approach in this pr 😄

@alice-i-cecile
Copy link
Member

This should be rebased on top of #154 in the new-book branch. I've moved the fast compile instructions much later (to avoid tripping up beginners quite so aggressively), so you'll have to modify what I have there instead.

@lukors
Copy link
Author

lukors commented May 25, 2021

@alice-i-cecile just so I don't do anything wrong, I should at this time rebase this PR's branch (https://github.com/lukors/bevy-website/tree/patch-1) onto your branch (https://github.com/alice-i-cecile/bevy-website/tree/book-setup)?

Including moving the changes in this PR to where they should be in the new book etc.

Won't that include all your changes into this PR? Or maybe that's fine?

@lukors
Copy link
Author

lukors commented May 29, 2021

@alice-i-cecile I couldn't figure out how to preserve the history of my commits, so I just put the changes in a new commit on top of the branch and force pushed.

Also made sure formatting looked good after going through Zola and did some other minor improvements.

I hope this is ok and I did it right?

@alice-i-cecile
Copy link
Member

@alice-i-cecile I couldn't figure out how to preserve the history of my commits, so I just put the changes in a new commit on top of the branch and force pushed.

Yep, that's totally fine here :) Thanks!

It looks like you need to change which branch you're making the PR to though; this is still targeting master (hence the giant diff).

@lukors lukors changed the base branch from master to new-book May 29, 2021 19:23
@lukors
Copy link
Author

lukors commented May 29, 2021

Great! :)

Ah ok, I think I did that now.

@alice-i-cecile alice-i-cecile mentioned this pull request May 29, 2021
@lukors
Copy link
Author

lukors commented Jun 3, 2021

A merge conflict appeared for some reason when #154 was merged. I fixed it, so this is ready on my end.

As a reminder for cart, this is what you wrote about it most recently, so you don't forget to run those tests:

Thanks for the breakdown! I'll run a few tests on my machine just to get a feel for the various permutations / see if these behaviors are reproducible.

It does sound like we'll probably need to roll with the approach in this pr 😄

If anyone else wants to test it that would be great as well! AFAIK I'm the only one who's checked thoroughly, so would be nice to make sure it's all good before it is merged.

@cart cart force-pushed the new-book branch 2 times, most recently from 04bc036 to 477268e Compare August 6, 2021 21:29
bors bot pushed a commit to bevyengine/bevy that referenced this pull request Aug 30, 2021
It seems like this option needs to be off on Windows: bevyengine/bevy-website#131

This change also simplifies the instructions required for the Fast Compiles section of the book: bevyengine/bevy-website#137
@SevenO2
Copy link

SevenO2 commented Aug 30, 2021

@lukors

If anyone else wants to test it that would be great as well! AFAIK I'm the only one who's checked thoroughly, so would be nice to make sure it's all good before it is merged.

I did so. Results here: bevyengine/bevy#2016 (comment)

@lukors
Copy link
Author

lukors commented Aug 31, 2021

@SevenO2 Great news! Thanks a lot! 😄

Looks like some conflicts have popped up, I'll fix these soon, and then hopefully this will be ready to merge!

bilsen pushed a commit to bilsen/bevy that referenced this pull request Sep 2, 2021
It seems like this option needs to be off on Windows: bevyengine/bevy-website#131

This change also simplifies the instructions required for the Fast Compiles section of the book: bevyengine/bevy-website#137
@lukors
Copy link
Author

lukors commented Sep 7, 2021

I merged this on top of the new-book branch. I'm assuming that's what I was supposed to do, but please let me know if that's wrong. :)

I also want to add that I'm not 100% confident that I merged it correctly. I think I did, but I very rarely do these types of merges so I'm not sure.

I guess this is ready to be merged again if all is looking good!

@alice-i-cecile
Copy link
Member

I merged this on top of the new-book branch. I'm assuming that's what I was supposed to do, but please let me know if that's wrong. :)

Yes please; that's the most useful spot for it right now. Hopefully we can merge in #176 soon, and then rebase this on that.

```sh
# Install the nightly toolchain
rustup toolchain install nightly
# EITHER configure your current project to use nightly (run this mmand within the project)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's "mmand"? typo?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah it's a typo, should be "command", fixed and thanks! :D

rustup default nightly
```

You can use `cargo +nightly ...` if you don't want to change the default to nightly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can use `cargo +nightly ...` if you don't want to change the default to nightly.
You can use `cargo +nightly ...` if you don't want to change the default to nightly but just want to use it once for the current command.

Copy link
Author

Choose a reason for hiding this comment

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

This is a good addition, added, thanks! :D

alice-i-cecile added a commit to alice-i-cecile/bevy-website that referenced this pull request Oct 13, 2021
@alice-i-cecile
Copy link
Member

I've incorporated the body of this PR into #176 with credit <3 It will probably be simpler just to merge that, if you're alright with that.

@lukors
Copy link
Author

lukors commented Oct 15, 2021

@alice-i-cecile Yeah that works for me, thanks! :D

Should I close this PR now then? :)

@alice-i-cecile
Copy link
Member

Yep; we'll close this down and give you credit in the release notes!

@lukors lukors deleted the patch-1 branch October 15, 2021 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants