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

New default compilation profiles for web commands #199

Merged
merged 18 commits into from
Dec 31, 2024
Merged

Conversation

TimJentzsch
Copy link
Collaborator

@TimJentzsch TimJentzsch commented Dec 25, 2024

Objective

Closes #197.

The bevy build web and bevy run web commands currently use the same default profiles as the cargo commands, so either dev or release if the --release flag is provided.
However, the demands of web apps can be vastly different than native apps when it comes to compilation.
For example, it's a lot more effective to optimize for low binary size on the web, because it significantly reduces the loading time of the app.

This causes two problems with the current implementation:

  • The defaults are bad. If the user doesn't know about the differences, they will probably have high loading times for their apps or potentially longer compile times.
  • Fixing the problem results in boilerplate. Once you set up custom profiles for a better configuration, you will have to provide the --profile={profile_name} argument every time you execute bevy run web... which is both a hassle and easy to forget.

Solution

The web commands now use the web or web-release profiles by default. This means we can provide better defaults, while still allowing the user to fully customize the configuration (by just defining these profiles themselves in Cargo.toml).
It also means the user doesn't have to pass the --profile flag to the web commands, in most cases.

Of course, by default there won't be any definitions for the web and web-release profiles and we don't want to edit the user's Cargo.toml directly. So if we configured --profile=web we would get an error.
We circumvent this problem with --config flag overrides, which essentially allow you to add additional entries to the Cargo.toml.
For example, --config profile.web.inherits="dev" allows us to add the web profile. It can be further customized with default settings in a similar way.

I just set up very basic default configurations for the new profiles, optimizing them will be a task for a future PR.

@TimJentzsch TimJentzsch added A-Run Related to the bevy run command A-Build Related to the bevy build command A-Web Building or running Bevy apps targeting the browser labels Dec 25, 2024
@TimJentzsch TimJentzsch force-pushed the 197-custom-web-profiles branch from 07753df to 3e5fffe Compare December 26, 2024 13:10
@TimJentzsch TimJentzsch marked this pull request as ready for review December 26, 2024 13:12
@TimJentzsch TimJentzsch added this to the `bevy_cli` 0.1.0 milestone Dec 27, 2024
@TimJentzsch TimJentzsch added C-Feature Make something new possible C-Breaking-Change A breaking change of the public API labels Dec 27, 2024
@BD103
Copy link
Member

BD103 commented Dec 27, 2024

This feature should be added to the user-facing documentation in #204, once that's worked on :)

@TimJentzsch
Copy link
Collaborator Author

Hmm, on Windows, the wasm-opt-sys build seems to fail now, not sure how to fix it...

Maybe we should install the CLI of it after all, then we don't need to install it in our CI

@BD103
Copy link
Member

BD103 commented Dec 29, 2024

The error on Windows is:

Error: C++ compiler does not support `/std:c++17` flag

Maybe the version of MSVC installed is too old? We use the windows-latest runner, which currently is windows-2022. Maybe look over the installed software to see if anything's outdated?

If you run Windows locally, are you able to compile it yourself? You may be able to cross-reference your MSVC version with the Windows runner.

@TimJentzsch
Copy link
Collaborator Author

The error on Windows is:

Error: C++ compiler does not support `/std:c++17` flag

Maybe the version of MSVC installed is too old? We use the windows-latest runner, which currently is windows-2022. Maybe look over the installed software to see if anything's outdated?

If you run Windows locally, are you able to compile it yourself? You may be able to cross-reference your MSVC version with the Windows runner.

All the requirements listed by wasm-opt should be available.
I checked the workflow file they use and the only difference I could spot is that they use windows-2019 instead of latest. I'll check if that is the cause of the issue.

@TimJentzsch
Copy link
Collaborator Author

Wait a second, this isn't even the wasm-opt PR, this PR shouldn't have changed anything 🤨
The CI in #206 has passed

@BD103
Copy link
Member

BD103 commented Dec 30, 2024

Wait a second, this isn't even the wasm-opt PR, this PR shouldn't have changed anything 🤨
The CI in #206 has passed

What's interesting is that only 38d8f09, where you updated from the main branch, failed. All previous commits passed. Maybe it was the changes to Cargo.lock?

@TimJentzsch
Copy link
Collaborator Author

I restored the previous Cargo.lock and it appears to work now :)

@BD103
Copy link
Member

BD103 commented Dec 30, 2024

Huh, it works now! I looked over the diff in 8ede832, and these 3 changes stood out to me:

  • cc 1.2.2 -> 1.2.6
  • libc 0.2.167 -> 0.2.169
  • wasm-bindgen 0.2.95 -> 0.2.99

Since it was a compilation error, I think some change made between the versions of cc caused this failure. From the issue tracker, it looks like this issue was reported in rust-lang/cc-rs#1324.

@TimJentzsch
Copy link
Collaborator Author

Thanks for tracking down the cause!
I guess since its unrelated to the PR, is it ready to merge now?

@BD103
Copy link
Member

BD103 commented Dec 31, 2024

Could you pin cc to v1.2.2, so cargo update doesn't break the build? You can do so by adding the following to Cargo.toml:

[build-dependencies]
# We don't use `cc` directly, but our dependency `wasm-opt-sys` fails to compile on Windows when
# using a newer version. This can be removed when https://github.com/rust-lang/cc-rs/issues/1324 is
# fixed. 
cc = "=1.2.2"

@TimJentzsch
Copy link
Collaborator Author

Sure, done!

@TimJentzsch
Copy link
Collaborator Author

Lol, now cargo-sweep failed when trying to delete the old build script for wasm-opt-sys, I think? :D
@BD103 I wonder if this should actually be a CI error of it cargo-sweep should ignore it?
Might have unintentional side-effects to ignore it though.

@BD103 BD103 enabled auto-merge (squash) December 31, 2024 18:30
@BD103
Copy link
Member

BD103 commented Dec 31, 2024

@BD103 I wonder if this should actually be a CI error of it cargo-sweep should ignore it?
Might have unintentional side-effects to ignore it though.

Yup, I opened BD103/cargo-sweep#13 for this! For now, I purged the entire CI cache so we wouldn't hit this error.

@BD103 BD103 merged commit d5f6bf7 into main Dec 31, 2024
8 checks passed
@BD103 BD103 deleted the 197-custom-web-profiles branch December 31, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build Related to the bevy build command A-Run Related to the bevy run command A-Web Building or running Bevy apps targeting the browser C-Breaking-Change A breaking change of the public API C-Feature Make something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve profile selection for web builds
2 participants