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

Add some more helpful errors to BevyManifest when it doesn't find Cargo.toml #9207

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Add some more helpful errors to BevyManifest when it doesn't find Cargo.toml #9207

merged 1 commit into from
Jul 19, 2023

Conversation

photex
Copy link
Contributor

@photex photex commented Jul 19, 2023

When building Bevy using Bazel, you don't need a 'Cargo.toml'... except Bevy requires it currently. Hopefully this can help illuminate the requirement.

Objective

I recently started exploring Bazel and Buck2. Currently Bazel has some great advantages over Cargo for me and I was pretty happy to find that things generally work quite well!

Once I added a target to my test project that depended on bevy but didn't use Cargo, I didn't create a Cargo.toml file for it and things appeared to work, but as soon as I went to derive from Component the build failed with the cryptic error:

ERROR: /Users/photex/workspaces/personal/mb-rogue/scratch/BUILD:24:12: Compiling Rust bin hello_bevy (0 files) failed: (Exit 1): process_wrapper failed: error executing command (from target //scratch:hello_bevy) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/rules_rust/util/process_wrapper/process_wrapper --arg-file ... (remaining 312 arguments skipped)
error: proc-macro derive panicked
 --> scratch/hello_bevy.rs:5:10
  |
5 | #[derive(Component)]
  |          ^^^^^^^^^
  |
  = help: message: called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }
error: proc-macro derive panicked
 --> scratch/hello_bevy.rs:8:10
  |
8 | #[derive(Component)]
  |          ^^^^^^^^^
  |
  = help: message: called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }

Solution

After poking around I realized that the proc macros in Bevy all use bevy_macro_utils::BevyManifest, which was attempting to load a Cargo manifest that doesn't exist.

This PR doesn't address the Cargo requirement (I'd love to see if there was a way to support more than Cargo transparently), but it does replace some calls to unwrap with expect and hopefully the error messages will be more helpful for other folks like me hoping to pat down a new trail:

ERROR: /Users/photex/workspaces/personal/mb-rogue/scratch/BUILD:23:12: Compiling Rust bin hello_bevy (0 files) failed: (Exit 1): process_wrapper failed: error executing command (from target //scratch:hello_bevy) bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/rules_rust/util/process_wrapper/process_wrapper --arg-file ... (remaining 312 arguments skipped)
error: proc-macro derive panicked
 --> scratch/hello_bevy.rs:5:10
  |
5 | #[derive(Component)]
  |          ^^^^^^^^^
  |
  = help: message: Unable to read cargo manifest: /private/var/tmp/_bazel_photex/135f23dc56826c24d6c3c9f6b688b2fe/execroot/__main__/scratch/Cargo.toml: Os { code: 2, kind: NotFound, message: "No such file or directory" }
error: proc-macro derive panicked
 --> scratch/hello_bevy.rs:8:10
  |
8 | #[derive(Component)]
  |          ^^^^^^^^^
  |
  = help: message: Unable to read cargo manifest: /private/var/tmp/_bazel_photex/135f23dc56826c24d6c3c9f6b688b2fe/execroot/__main__/scratch/Cargo.toml: Os { code: 2, kind: NotFound, message: "No such file or directory" }

@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 ✨

When building Bevy using Bazel, you don't need a 'Cargo.toml'... except Bevy requires it currently.
Hopefully this can help illuminate the requirement.
@nicopap
Copy link
Contributor

nicopap commented Jul 19, 2023

Looks good to me. Looks like this code exists only to make sure macros work with both bevy_foobar and bevy::foobar. Adding the requirement to have a manifest just for this seems ridiculous.

It was added by #1875 (but the behavior predates it).

IMO there are several saner approaches:

  • Add feature flags to the macro crates so that when bevy re-exports the macros, it can tell the macro crates to use the bevy-based paths.
  • By default, use bevy::foobar and add a macro attribute to override that path (this is the most common approach I saw in the rust ecosystem)

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

I'll go with the better error messages until we figure something better out.

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 19, 2023
@photex
Copy link
Contributor Author

photex commented Jul 19, 2023

Thanks for explaining this @nicopap. I was just starting to investigate its intended function to propose an alternative that works with all build systems.
I’m happy to experiment with your suggested approaches above if nobody else minds.

@mockersf
Copy link
Member

If you want to experiment, I would suggest:

  • check if there's a macro attribute specifying a path. if there is, use it
  • if not, fallback to trying to read the cargo.toml file to get the path
  • if not, fallback to bevy::
  • if all failed, helpful error message

we can merge this PR soon and you experiment in another, or you can experiment in this one, as you prefer

@photex
Copy link
Contributor Author

photex commented Jul 19, 2023

Thanks @mockersf I will experiment in another branch and let this branch be merged/closed. 👍

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

I agree with @nicopap that this does seem like overkill for this particular use case. Though we should temper expectations here: given the status of alternative build systems in the Rust ecosystem, I don't think we should formally support them without including it in our CI in some way and making a commitment to ensuring Bevy's developer experience with them is up to par. With that said, improving compilation errors and accepting PRs for "best effort" support is definitely doable. Approved.

@james7132 james7132 added this pull request to the merge queue Jul 19, 2023
Merged via the queue into bevyengine:main with commit a0972c2 Jul 19, 2023
@photex
Copy link
Contributor Author

photex commented Jul 19, 2023

Thanks @james7132! I totally agree with you. My hope is only to address whatever amount of these quality of life issues that I can, within the constraint that it works for Cargo primarily as the defacto build system at the present time.
If ever the project is willing to officially support Bazel or Buck2 in the future, then having all the little things out of the way would definitely be useful. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

4 participants