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

Fix wasm builds with file_watcher enabled #10589

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

Leinnan
Copy link
Contributor

@Leinnan Leinnan commented Nov 16, 2023

Objective

Solution

  • Remove notify-debouncer-full dependency on WASM platform entirely.
  • Compile with "file_watcher" feature now on platform wasm32 gives meaningful compile error.

Changelog

Fixed

  • Compile with "file_watcher" feature now on platform wasm32 gives meaningful compile error.

@Leinnan
Copy link
Contributor Author

Leinnan commented Nov 16, 2023

Repro steps:

  • Create new project
  • Add bevy dependency with feature "file_watcher"
  • Try build for wasm platform(cargo build --target wasm32-unknown-unknown)
  • Build failed before, now it works.

@nicopap nicopap added A-Assets Load files from disk to use for things like images, models, and sounds O-Web Specific to web (WASM) builds labels Nov 16, 2023
@nicopap
Copy link
Contributor

nicopap commented Nov 16, 2023

Add "Fixes #10507" to the PR description please.

Now the difficult question. Is this what we want? What's the problem with failing to compile invalid code?

The proposed solution of having hot-reloading silently become broken on WASM targets seems far from ideal. When a user compiles their game with the file_watcher feature, they expect hot-reloading to work. Nothing else. With this change, it sure compiles, but it won't work, breaking user expectation.

Here is a suggestion:

#[cfg(all(feature = "file_watcher", target_arch = "wasm32"))]
compile_error!(
    "The \"file_watcher\" feature for hot reloading does not work \
    on WASM.\nDisable \"file_watcher\" (or \"embedded_watcher\") \
    when compiling to WASM"
);

Question: does this print the error message when trying to compile with the setup you describe? If so I think it might be preferable.


What's the issue with the failing compilation? That it is unclear how to fix it? If so, the compile_error! trick should fix the problem.

@nicopap nicopap added the P-Compile-Failure A failure to compile Bevy apps label Nov 16, 2023
@Leinnan
Copy link
Contributor Author

Leinnan commented Nov 17, 2023

@nicopap I did not about this macro, it's great! So I did remove most of the changes and added the macro. One thing that I left is I still disabled notify-debouncer-full dependency, so it is not compiled on wasm. I did it because without that change, it does not even get to the macro compilation and error is not obvious:

   Compiling notify-debouncer-full v0.3.1
error[E0432]: unresolved import `file_id::get_file_id`
   --> C:\Users\rybop\.cargo\registry\src\index.crates.io-6f17d22bba15001f\notify-debouncer-full-0.3.1\src\cache.rs:6:15
    |
6   | use file_id::{get_file_id, FileId};
    |               ^^^^^^^^^^^ no `get_file_id` in the root
    |
note: found an item that was configured out
   --> C:\Users\rybop\.cargo\registry\src\index.crates.io-6f17d22bba15001f\file-id-0.2.1\src\lib.rs:111:8
    |
111 | pub fn get_file_id(path: impl AsRef<Path>) -> io::Result<FileId> {
    |        ^^^^^^^^^^^
    = note: the item is gated behind the `unix` feature
note: found an item that was configured out
   --> C:\Users\rybop\.cargo\registry\src\index.crates.io-6f17d22bba15001f\file-id-0.2.1\src\lib.rs:121:8
    |
121 | pub fn get_file_id(path: impl AsRef<Path>) -> io::Result<FileId> {
    |        ^^^^^^^^^^^
    = note: the item is gated behind the `windows` feature

For more information about this error, try `rustc --explain E0432`.
error: could not compile `notify-debouncer-full` (lib) due to previous error

And now on my branch it fails nicely:

   Compiling bevy_asset v0.12.0 (C:\PROJECTS\bevy\crates\bevy_asset)
error: The "file_watcher" feature for hot reloading does not work on WASM.
       Disable "file_watcher" when compiling to WASM
 --> C:\PROJECTS\bevy\crates\bevy_asset\src\io\mod.rs:2:1
  |
2 | / compile_error!(
3 | |     "The \"file_watcher\" feature for hot reloading does not work \
4 | |     on WASM.\nDisable \"file_watcher\" \
5 | |     when compiling to WASM"
6 | | );
  | |_^

error: could not compile `bevy_asset` (lib) due to previous error

You guessed the point of the change, right- I just want to make clear what build is failing if it has to fail.

@nicopap
Copy link
Contributor

nicopap commented Nov 17, 2023

Add "Fixes #10507" to the PR description please. So that the issue gets automatically closed.

You can click on the … on the top left of your PR description, and there should be an "Edit" option.

@nicopap nicopap added this to the 0.12.1 milestone Nov 17, 2023
@nicopap
Copy link
Contributor

nicopap commented Nov 17, 2023

This seems completely non-breaking, as we still have the same behavior, just a different compilation error message. So I think it's a candidate for 0.12.1

@Leinnan
Copy link
Contributor Author

Leinnan commented Nov 17, 2023

Add "Fixes #10507" to the PR description please. So that the issue gets automatically closed.

You can click on the … on the top left of your PR description, and there should be an "Edit" option.

Description updated

@cart cart added this pull request to the merge queue Nov 17, 2023
Merged via the queue into bevyengine:main with commit 2b32de9 Nov 17, 2023
25 checks passed
cart pushed a commit that referenced this pull request Nov 30, 2023
# Objective

- Currently, in 0.12 there is an issue that it is not possible to build
bevy for Wasm with feature "file_watcher" enabled. It still would not
compile, but now with proper explanation.
- Fixes #10507


## Solution

- Remove `notify-debouncer-full` dependency on WASM platform entirely.
- Compile with "file_watcher" feature now on platform `wasm32` gives
meaningful compile error.

---

## Changelog

### Fixed

- Compile with "file_watcher" feature now on platform `wasm32` gives
meaningful compile error.
@Leinnan Leinnan deleted the fix/file-watcher-wasm branch November 30, 2023 08:30
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Currently, in 0.12 there is an issue that it is not possible to build
bevy for Wasm with feature "file_watcher" enabled. It still would not
compile, but now with proper explanation.
- Fixes bevyengine#10507


## Solution

- Remove `notify-debouncer-full` dependency on WASM platform entirely.
- Compile with "file_watcher" feature now on platform `wasm32` gives
meaningful compile error.

---

## Changelog

### Fixed

- Compile with "file_watcher" feature now on platform `wasm32` gives
meaningful compile error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds O-Web Specific to web (WASM) builds P-Compile-Failure A failure to compile Bevy apps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

file_watcher feature fails to build for wasm32 because notify-debounder-full is only for native
3 participants