Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Globally upgrade to syn 2.x and latest quote and proc_macro2 1x versions #13846

Merged
merged 10 commits into from
Apr 12, 2023
Merged

Globally upgrade to syn 2.x and latest quote and proc_macro2 1x versions #13846

merged 10 commits into from
Apr 12, 2023

Conversation

sam0x17
Copy link
Contributor

@sam0x17 sam0x17 commented Apr 6, 2023 •
edited
Loading

polkadot companion: paritytech/polkadot#7022

Description

We have a lot of different versions of dtolnay's quote/syn/proc-macro2 crates floating around. Dtolnay recently came out with syn 2.x

This PR sets all versions of proc-macro2, syn, and quote to the versions listed below, which at the time of writing are the latest 1.x version for each crate (or 2.x in the case of syn)

This should improve compile-times since only one version of these crates will be compiled globally, and will also reduce confusing edge cases due to version differences

Crates Upgraded

proc-macro2 => 1.0.56
syn => 2.0.14
quote => 1.0.26

@sam0x17 sam0x17 added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 6, 2023
@sam0x17 sam0x17 requested a review from a team April 6, 2023 15:29
@sam0x17 sam0x17 self-assigned this Apr 6, 2023
@sam0x17 sam0x17 requested review from athei and koute as code owners April 6, 2023 15:29
@sam0x17 sam0x17 marked this pull request as draft April 6, 2023 15:41
@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 6, 2023

converting to draft because it looks like we might need some companions

@sam0x17 sam0x17 mentioned this pull request Apr 6, 2023
Merged
@sam0x17 sam0x17 marked this pull request as ready for review April 6, 2023 19:34
@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 6, 2023 •
edited
Loading

cumulus is already on syn 2.x and the latest for the other crates so no need for a cumulus companion

@sam0x17 sam0x17 requested a review from KiChjang April 6, 2023 20:39
@bkchr
Copy link
Member

bkchr commented Apr 7, 2023

cumulus is already on syn 2.x and the latest for the other crates so no need for a cumulus companion

Then not updating syn to 2.x makes no sense.

@bkchr
Copy link
Member

bkchr commented Apr 7, 2023

Could you please just update to 2.x for syn?

In general, cargo unifies across patch/minor releases which you already see with your pr here. You changed multiple Cargo.toml files, but the Cargo.lock wasn't changed. This means that we already used these versions.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 7, 2023

Could you please just update to 2.x for syn?

In general, cargo unifies across patch/minor releases which you already see with your pr here. You changed multiple Cargo.toml files, but the Cargo.lock wasn't changed. This means that we already used these versions.

Yes, let me do that

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 7, 2023

actually @bkchr it seems to be non-trivial. Upgrading to syn 2.x I get a ton of errors on substrate. For some reason cumulus is fine with it though

@koute
Copy link
Contributor

koute commented Apr 7, 2023

Yes, we should update to syn 2, but it might not be just a trivial version bump as there are breaking API changes between v1 and v2.

@sam0x17 From a practical point of view it'd be the best to just update the deps to v2 one-by-one and fix the errors as you go.

@bkchr
Copy link
Member

bkchr commented Apr 8, 2023

actually @bkchr it seems to be non-trivial. Upgrading to syn 2.x I get a ton of errors on substrate. For some reason cumulus is fine with it though

Cumulus probably proc-macro isn't that complex and maybe uses an api that didn't changed!

@bkchr bkchr requested a review from andresilva as a code owner April 9, 2023 21:50
@sam0x17 sam0x17 mentioned this pull request Apr 10, 2023
Merged
@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 10, 2023

note, will need to get drahnr/expander#6 merged if we want to do syn 2.x

@bkchr
Copy link
Member

bkchr commented Apr 10, 2023

note, will need to get drahnr/expander#6 merged if we want to do syn 2.x

scale-codec also doesn't yet use syn 2.0. There are also probably plenty of other crates that are not yet ported. I don't see a reason to wait.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 10, 2023 •
edited
Loading

@bkchr I can't get polkadot to build with syn 2.0 without upgrading expander's syn to 2.0 as well. There is a very subtle conflict on syn::Error not being compatible between the latest 1.x and the latest 2.x, if you look at my latest push on the polkadot companion

Also, things that aren't ready to upgrade to syn 2 could just not use the new expander version and be fine I would think

side note: upgrading expander was as simple as changing the version in Cargo.toml, so should be backwards compatible generally (other than this particular issue I would think)

@bkchr
Copy link
Member

bkchr commented Apr 11, 2023

There is no commit on the companion.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 11, 2023

There is no commit on the companion.

Pushed an hour or so ago:

commit 8e840554ee01719067acbf68cf8dcf78251bad8b (HEAD -> sam-globally-upgrade-syn-quote, origin/sam-globally-upgrade-syn-quote)

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 11, 2023

This one causes the failure paritytech/polkadot@1c63df9

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 11, 2023 •
edited
Loading

This is the error @bkchr, took me a good bit to figure out that it is because of syn versions:

image

I tried a bunch of things to fix it including creating a new wrapper type TokenStreamResultWrapper that has the proper impls but it was impossible to satisfy the compiler because expander is actually talking about a different syn::Error

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 11, 2023

So he accepted my PR, though no new release has been cut yet. I just pushed up a test commit manually updating the version of expander to use the latest git just to confirm that CI passes with it. Will remove that once he cuts a new release

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 11, 2023

bot rebase

@paritytech-processbot
Copy link

Error: Command 'Command { std: cd "/storage/repositories/substrate" && "git" "merge" "origin/master" "--no-ff" "--no-edit", kill_on_drop: false }' failed with status Some(1); output: no output

@sam0x17 sam0x17 changed the title Globally upgrade/normalize syn/quote/proc-macro2 crate versions to latest 1.0.x Globally upgrade to syn 2.x and latest quote and proc_macro2 1x versions Apr 12, 2023
@sam0x17 sam0x17 force-pushed the sam-globally-upgrade-syn-quote branch from 21f5c41 to 445a619 Compare April 12, 2023 13:20
@KiChjang
Copy link
Contributor

So he accepted my PR, though no new release has been cut yet. I just pushed up a test commit manually updating the version of expander to use the latest git just to confirm that CI passes with it. Will remove that once he cuts a new release

You don't really need to wait for him to publish -- you said before that paritytech-devs are owners of his crate on crates.io as well, correct? If so, you can just run cargo publish locally with the new version, and it will Just Work.

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 12, 2023

So he accepted my PR, though no new release has been cut yet. I just pushed up a test commit manually updating the version of expander to use the latest git just to confirm that CI passes with it. Will remove that once he cuts a new release

You don't really need to wait for him to publish -- you said before that paritytech-devs are owners of his crate on crates.io as well, correct? If so, you can just run cargo publish locally with the new version, and it will Just Work.

he published an hour after he accepted my PR apparently so we good

@sam0x17
Copy link
Contributor Author

sam0x17 commented Apr 12, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit f0d3bbb into master Apr 12, 2023
@paritytech-processbot paritytech-processbot bot deleted the sam-globally-upgrade-syn-quote branch April 12, 2023 18:42
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@sam0x17 @bkchr @nathanwhit
Globally upgrade to syn 2.x and latest quote and proc_macro2 1x versi…
3e5e957 
…ons (paritytech#13846)

* globally upgrade quote to latest 1.0.x (1.0.26)

* globally upgrade syn to final 1.0.x version (1.0.109)

* globally upgrade proc-macro2 to 1.0.56

* upgrade to syn v2.0.13 and fix everything except NestedMeta

* fix parse nested metadata code in decl_runtime_apis.rs

* Port more stuff to syn 2.0

* Make the rest compile

* Ignore error

* update to syn 2.0.14

---------

Co-authored-by: Bastian Köcher <info@kchr.de>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bkchr bkchr bkchr approved these changes

@KiChjang KiChjang KiChjang approved these changes

@koute koute koute approved these changes

@athei athei Awaiting requested review from athei athei is a code owner

@andresilva andresilva Awaiting requested review from andresilva

Assignees

@sam0x17 sam0x17

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sam0x17 @bkchr @koute @KiChjang