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

Check dev-dependencies for bevy/bevy-internal only in test builds #2262

Closed
wants to merge 1 commit into from

Conversation

Waridley
Copy link
Contributor

This fixes a regression caused by #1875 where using bevy in [dev-dependencies] but not in [dependencies] (using, e.g., bevy_ecs instead) would cause derive macros to fail to compile in non-test builds.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Core Common functionality for all bevy apps C-Regression Functionality that used to work but no longer does. Add a test for this! labels May 27, 2021
fixes issue where using `bevy` in `[dev-dependencies]` but not `[dependencies]` would cause macros to fail to compile in normal builds
@Waridley
Copy link
Contributor Author

Wow, I had run cargo fmt, but then changed .map into .and_then and forgot to run it again and it barely pushed the line over the length limit 🤣

.or_else(|| deps_dev.and_then(find_in_deps))
.unwrap_or_else(|| get_path(name))
let path = deps.and_then(find_in_deps);
#[cfg(test)]
Copy link
Contributor

Choose a reason for hiding this comment

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

#[cfg(test)] is only set when the current crate is itself tested, not when any dependent crate is being tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RIP... I knew something had to be wrong with this approach. Any idea how to make this work, then?

Admittedly my specific use case is a bit of laziness, and not depending on bevy is saving me less as time goes on and I decide I want to use more of bevy's features... but this seems like something that might surprise other users...

@NathanSWard NathanSWard self-requested a review May 28, 2021 15:02
@NathanSWard
Copy link
Contributor

Is it possible to add a test for this?

@Waridley Waridley marked this pull request as draft May 28, 2021 15:54
@Waridley
Copy link
Contributor Author

Waridley commented Jun 9, 2021

While I originally said this is a regression, it's entirely possible I'm the only user this is affecting, and I don't see a way forward if it's not possible to know when [dev-dependencies] are being used in downstream crates. The workaround is just to either refactor your tests to use individual crates, or refactor your main project to use bevy. So I'm going to close this.

Although I have to wonder if Bevy is really the only crate that has to deal with this. It really seems like proc-macros need a robust way to refer to the crate they are generating code for...

@Waridley Waridley closed this Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Bug An unexpected or incorrect behavior C-Regression Functionality that used to work but no longer does. Add a test for this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants