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

Jump to bottom

allow system macro to use a local module for legion behind a feature instead of the crate #186

Closed
wants to merge 1 commit into from
Closed

allow system macro to use a local module for legion behind a feature instead of the crate #186

wants to merge 1 commit into from

Conversation

mockersf
Copy link

@mockersf mockersf commented Sep 9, 2020

fixes #178

This is due to all paths to legion in the macro starting with :: which means that legion has to be a crate and not a module.

I found this issue when playing with Amethyst where the system macro doesn't work with the following error:

error[E0432]: unresolved import `legion`
  --> examples/pong_tutorial_03/systems/paddle.rs:34:1
   |
34 | #[system]
   | ^^^^^^^^^ help: a similar path exists: `amethyst_core::legion`
   |
   = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0433]: failed to resolve: could not find `legion` in `{{root}}`
  --> examples/pong_tutorial_03/systems/paddle.rs:34:1
   |
34 | #[system]
   | ^^^^^^^^^ could not find `legion` in `{{root}}`
   |
   = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0433]: failed to resolve: could not find `legion` in `{{root}}`
  --> examples/pong_tutorial_03/systems/paddle.rs:34:1
   |
34 | #[system]
   | ^^^^^^^^^ not found in `legion::systems`
   |
   = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing one of these items
   |
1  | use amethyst::ecs::SystemBuilder;
   |
1  | use amethyst_core::ecs::SystemBuilder;
   |
1  | use crate::SystemBuilder;
   |

If I add legion as a direct dependency, it works, but it's not ideal because it means everyone using amethyst with legion would always need to keep both in sync.

I added a feature reexport that, when enabled, will switch the path prefix from :: to self::, which means I can now use a re-exported legion module

@mockersf mockersf changed the title allow system macro to use a local lefion behind a feature allow system macro to use a local module for legion behind a feature instead of the crate Sep 9, 2020
@0x6273
Copy link

0x6273 commented Oct 5, 2020

So amethyst users would have to do use amethyst::ecs as legion in order to use the macro?
If so, I think it's better to name the feature amethyst_reexport and have it use ::amethyst::ecs as the path for legion, to avoid that extra hassle for users.

0x6273
0x6273 suggested changes Oct 5, 2020
View reviewed changes
codegen/src/lib.rs
Comment on lines 317 to 326
if mutable {
#[cfg(not(feature = "reexport"))]
query.push(
parse_quote!(::legion::TryWrite<#elem>),
);
#[cfg(feature = "reexport")]
query.push(
parse_quote!(self::legion::TryWrite<#elem>),
);
} else {
Copy link

Choose a reason for hiding this comment

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

Why not put the legion path in a variable? That way you only need one pair of #[cfg(feature = ..)], and you avoid a lot of code duplication.

Copy link
Author

Choose a reason for hiding this comment

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

because I didn't think about using a path alias 👍

@mockersf
Copy link
Author

mockersf commented Oct 5, 2020

So amethyst users would have to do use amethyst::ecs as legion in order to use the macro?
Yes, there are three possibilities:

  • the user rename the import as legion
  • amethyst exports both as legion and ecs, but only show ecs in doc if it wants to continue using ecs as it sounds more generic
  • legion uses the exact path exported by amethyst under a feature

It depends on how linked legion and amethyst are going to be, and where this link is going to be expressed.

@mockersf
Copy link
Author

mockersf commented Nov 8, 2020 •
edited

there's also a crate_name function in crate proc-macro-crate that can give the name used for the import

this is used in bevy for example to know how to access the crate : https://github.com/bevyengine/bevy/blob/master/crates/bevy_ecs/hecs/macros/src/lib.rs#L54

bevy now uses find from crate find_crate: https://github.com/bevyengine/bevy/blob/master/crates/bevy_ecs/macros/src/lib.rs#L58

@mockersf mockersf mentioned this pull request Nov 19, 2020
Closed
23 tasks
@TriedAngle
Copy link

it has been some months, any updates on this?

@mockersf mockersf closed this Apr 24, 2021
@AlveLarsson
Copy link
Contributor

@mockersf Hello, thanks for the PR and sorry for the massive delay, Thomas is quite busy nowadays,
I'm currently working on a bigger usability update for amethyst itself, would you want to re-open this PR? I where looking into this issue and like this PR

@mockersf
Copy link
Author

mockersf commented Jul 9, 2021

Oh I assumed this had been fixed in another way as I saw Amethyst integration make progress and it was quite painful to write systems without that at the time...
I don't have my legion fork anymore, feel free to open a new PR inspired by this one if you want 👍

@AlveLarsson
Copy link
Contributor

Okay, thank you for the PR!
I will credit you in the new PRs for this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0x6273 0x6273 0x6273 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to use legion macros unless legion is a direct dependency
4 participants
@mockersf @0x6273 @TriedAngle @AlveLarsson