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

Consider updating from syn@1 to syn@2 #8282

Closed
LikeLakers2 opened this issue Apr 1, 2023 · 11 comments · Fixed by #8573
Closed

Consider updating from syn@1 to syn@2 #8282

LikeLakers2 opened this issue Apr 1, 2023 · 11 comments · Fixed by #8573
Labels
C-Dependencies A change to the crates that Bevy depends on D-Complex Quite challenging from either a design or technical perspective. Ask for help!
Milestone

Comments

@LikeLakers2
Copy link

LikeLakers2 commented Apr 1, 2023

Hi! I noticed that several parts of Bevy depend on syn@1. Perhaps you could consider updating to use syn@2 wherever possible?

Some things worth noting for anyone who intends to take on this suggestion:

  • syn@2's MSRV is Rust 1.56, up from syn@1's Rust 1.31. This shouldn't be a problem, since the Cargo.toml in Bevy's main branch specifies a MSRV of Rust 1.67.
  • Updating to syn@2 may not be as simple as swapping the version number - there are a number of breaking changes in syn@2, listed at https://github.com/dtolnay/syn/releases/tag/2.0.0.
  • Some third-party crates that Bevy depends on may also use syn@1. For the moment, I would not worry about those unless it makes it impossible to update to syn@2 - the purpose of this issue is to recommend Bevy's transition to syn@2. (I will recommend those crates transition to syn@2 soon - if not try to PR the changes myself.)

P.S. If you're curious, here's the command I used, and the resulting log, to determine what creates are still depending on syn@1. This is done on a new blank project, with only bevy = "0.10" (and no additional features enabled beyond the default) added to Cargo.toml:

cargo tree --invert syn@1.0.109 --depth 1
syn v1.0.109
├── bevy_derive v0.10.1 (proc-macro)
├── bevy_ecs_macros v0.10.1 (proc-macro)
├── bevy_macro_utils v0.10.1
├── bevy_reflect_derive v0.10.1 (proc-macro)
├── bevy_render_macros v0.10.1 (proc-macro)
├── bevy_utils_proc_macros v0.10.1 (proc-macro)
├── encase_derive_impl v0.5.0
├── gltf-derive v1.1.0 (proc-macro)
├── windows-implement v0.44.0 (proc-macro)
└── windows-interface v0.44.0 (proc-macro)
@jakobhellermann
Copy link
Contributor

jakobhellermann commented Apr 1, 2023

cargo tree --invert syn@1.0.109 (Very long log!)

you can also use --depth 1 or --format '{lib}' to make it more concise:

syn
├── bevy_derive
├── bevy_ecs_macros
├── bevy_macro_utils
├── bevy_reflect_derive
├── bevy_render_macros
├── bevy_utils_proc_macros
├── encase_derive_impl
└── gltf_derive

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy C-Dependencies A change to the crates that Bevy depends on labels Apr 1, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Apr 1, 2023
@LikeLakers2
Copy link
Author

@jakobhellermann Thanks for the heads up! I've updated the main post with the shorter log. :) (Though I've kept the version numbers and (proc-macro) identifiers, just in case they're useful!)

@mockersf
Copy link
Member

mockersf commented Apr 1, 2023

Hi! I noticed that several parts of Bevy depend on syn@1. Perhaps you could consider updating to use syn@2 wherever possible?

It's definitely being considered, some people tried but didn't manage to do it yet though...

@boj
Copy link

boj commented Apr 3, 2023

I've been giving this a shot this evening, however, it's quite the lift. Might take several more hours before I have anything worthy of a PR.

Edit: As I push further up the stack I'm seeing a huge departure between syn@1's NestedMeta and how syn@2's syn::meta stuff behaves. Going to take a huge amount of refactoring to get this to work.

boj added a commit to boj/bevy that referenced this issue Apr 3, 2023
Incremental update which tackles the low hanging crates first.

- bevy_derive (no code changes required)
- bevy_macro_utils

Everything else is currently broken in this branch and requires heavy refactoring.

Attempt to tackle bevyengine#8282
@boj
Copy link

boj commented Apr 4, 2023

Well, after spending more hours on it tonight, I would definitely change the label to D-Complex. The bit of work pushed into my branch barely represents the time spent understanding the differences between syn 1 and 2, let along puzzling out how to refactor some of the more nightmarish files into something sensible.

The two I came across tonight which left me scratching my head were:

It's not a simple matter of replacing things, it's literally a new architecture which likely requires some rewrites of various sections. I suspect the newer approach in v2 would simplify things quite a bit, however, going to take quite a bit more thought on how to even tackle that design.

@james7132 james7132 added D-Complex Quite challenging from either a design or technical perspective. Ask for help! and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Apr 4, 2023
@B-Reif
Copy link
Contributor

B-Reif commented Apr 8, 2023

I also took a stab at this and kind of got stuck. It doesn't help that the 'breaking changes' listed in syn's patch notes omit details about how to migrate. I'll take the chance to leave some per-crate notes here about what I ran into.

bevy_encase_derive

This crate imports a version of syn from encase_derive_impl and then uses it with other Bevy types which also rely on syn. This usage constrains our version of syn to whichever version encase_derive is using. Specifically, this crate uses BevyManifest with syn types.

A first step to migrating syn should be to isolate our usage of encase_derive_impl from our other crates. This might mean refactoring the BevyManifest API to expose string paths instead of syn types. This would also help move us in a direction where bevy crates can be migrated individually.

bevy_macro_utils

It might be worth investigating whether we need this crate. There's not a lot in it, most of its functions aren't being re-used anywhere, and it seems like syn's new API makes these functions less needed. It could also help us migrate other crates individually.

Others

Other crates include bevy_derive, bevy_ecs, bevy_reflect, and bevy_render, which AFAIK don't re-export any syn types. Each of these has their own business logic for their use of syn. It's probably best for these to be migrated individually by people with experience in these domains.

It should be easier to migrate crate-by-crate if we refactor out whatever we have that's exporting syn. This includes BevyManifest and everything in bevy_macro_utils.

@LikeLakers2
Copy link
Author

@B-Reif Regarding being tied to encase_derive_impl's version of syn, they're working on upgrading to syn@2. teoxoy/encase#35

No idea if that'll change your game plan, but worth mentioning regardless.

@B-Reif
Copy link
Contributor

B-Reif commented Apr 8, 2023

Yeah that's my PR 😰 I got a little stuck on that too!

@LikeLakers2
Copy link
Author

LikeLakers2 commented Apr 8, 2023

Oh! Woops. My bad, I didn't realize that was your PR. 😅 Still, good to note for others who are reading this. :)

@LikeLakers2
Copy link
Author

For anyone looking at this issue, teoxoy/encase#35 (which migrates encase_derive_impl to syn@2) has been merged, and v0.6.0 of encase, encase_derive, and encase_derive_impl (which contain the changes made in teoxoy/encase#35) have been published.

I am unsure how/if this would change the feasability of bevy migrating to syn@2, but I figured it's worth mentioning.

@mockersf
Copy link
Member

mockersf commented May 8, 2023

I have Bevy mostly migrated to syn 2, but there's a bug in encase update: teoxoy/encase#42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Dependencies A change to the crates that Bevy depends on D-Complex Quite challenging from either a design or technical perspective. Ask for help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants