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

Update syn to 2.0 #1342

Merged
merged 1 commit into from
Jun 22, 2024
Merged

Update syn to 2.0 #1342

merged 1 commit into from
Jun 22, 2024

Conversation

yotamofek
Copy link
Contributor

Requires no adaptations.

@CAD97

This comment was marked as outdated.

@yotamofek
Copy link
Contributor Author

yotamofek commented Jan 14, 2024

Ah great, now it failed due to a different spurious and unrelated problem 😅
Let's run CI again..

Edit: seems the previous failure was not spurious and was fixed by 7866bce?

@djc
Copy link

djc commented Mar 18, 2024

Friendly ping? Would be nice to get this merged (and ideally, released) to reduce compile-times for downstream projects.

@Andlon
Copy link
Collaborator

Andlon commented Mar 18, 2024

Friendly ping? Would be nice to get this merged (and ideally, released) to reduce compile-times for downstream projects.

I agree. However, if I run cargo tree -p nalgebra --all-features, there are a few (opt-in) dependencies that still seem to rely on syn 1.0. Having both syn 1.0 and syn 2.0 in the dependency tree seems strictly worse. Perhaps we can upgrade these too though. It's not clear to me how much effort is required, as I suspect some packages might simply not have been updated to support syn 2.0.

@Andlon
Copy link
Collaborator

Andlon commented Mar 18, 2024

Actually, I realized that I hadn't updated dependencies in my dev environment for a while. After running cargo update, I see that the only other dependency that has not been updated to use syn 2.0 is rkyv. Since we anyway might have both syn 1.0 and syn 2.0 in the tree at this point due to serde using 2.0, I think we should go ahead and merge this as-is, and then log an issue for rkyv to also upgrade its syn version to 2.0.

@Andlon
Copy link
Collaborator

Andlon commented Mar 18, 2024

@sebcrozet: it's unclear to me how to handle this correctly with respect to release. I think we need to:

  • bump nalgebra-macros to v0.2.2 (I believe patch version should be sufficient?)
  • change nalgebra to depend on nalgebra-macros v0.2.2.

However, as far as I can tell, we don't actually need a new nalgebra release for this. If we release nalgebra-macros with a new patch version, then existing nalgebra users should get the change through cargo update.

@djc
Copy link

djc commented Mar 18, 2024

However, as far as I can tell, we don't actually need a new nalgebra release for this. If we release nalgebra-macros with a new patch version, then existing nalgebra users should get the change through cargo update.

Yes, that sounds correct. I think that means you don't even need to change nalgebra to bump the nalgebra-macros dependency.

@Andlon Andlon added patch version Merging PR only requires patch version bump merge ready PR is ready for merging labels May 9, 2024
@bash
Copy link

bash commented Jun 11, 2024

Resolves #1403 (I forgot to check PRs before opening an issue ^^)

@sebcrozet sebcrozet merged commit 5cb9dcb into dimforge:dev Jun 22, 2024
11 checks passed
@sebcrozet
Copy link
Member

Thank you all for making/reviewing this PR!

@yotamofek yotamofek deleted the syn2 branch June 22, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge ready PR is ready for merging patch version Merging PR only requires patch version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants