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

Forked borsh crate needs to rebase onto v0.8.1 #93

Closed
ryoqun opened this issue Jan 25, 2021 · 3 comments · Fixed by #96
Closed

Forked borsh crate needs to rebase onto v0.8.1 #93

ryoqun opened this issue Jan 25, 2021 · 3 comments · Fixed by #96
Assignees

Comments

@ryoqun
Copy link

ryoqun commented Jan 25, 2021

Hi! I'm an engineer from solana. :)

To make long story short, could you update your forked borsh crate to be rebased onto borsh-rs v0.8.1?

We noticed updating serde to v1.0.122 at the solana monorepo would break serum-dex build: https://buildkite.com/solana-labs/solana/builds/38795#a018ae71-7d4c-4dc2-a9a9-fab840bcd3ef/1360-1834. Don't worry. We won't do that soon. This is just maintenance update at our side. :)

If there is better way for you like bumping solana-program and/or spl, please let us know.

Details

Well, there are various crates involved.

Firstly, solana-program crates use serde. And serde v1.0.122 now depends on syn >= v1.0.60 specifically and it broke build of borsh-rs < v0.8.0: near/borsh-rs#2 (this pr should be in borsh-rs v0.8.0)

Then, serum-dex uses both solana-program and borsh-rs, which would be unable to build with conflicting versions.

serum-borsh = { git = "https://github.com/project-serum/borsh", branch = "serum" }

serum-dex/Cargo.lock

Lines 238 to 275 in d470586

[[package]]
name = "borsh"
version = "0.7.2"
source = "git+https://github.com/project-serum/borsh?branch=serum#337732a185f052d5ee0424127b04b89d455ffa81"
dependencies = [
"borsh-derive",
"solana-sdk",
]
[[package]]
name = "borsh-derive"
version = "0.7.2"
source = "git+https://github.com/project-serum/borsh?branch=serum#337732a185f052d5ee0424127b04b89d455ffa81"
dependencies = [
"borsh-derive-internal",
"borsh-schema-derive-internal",
"syn 1.0.53",
]
[[package]]
name = "borsh-derive-internal"
version = "0.7.2"
source = "git+https://github.com/project-serum/borsh?branch=serum#337732a185f052d5ee0424127b04b89d455ffa81"
dependencies = [
"proc-macro2 1.0.24",
"quote 1.0.7",
"syn 1.0.53",
]
[[package]]
name = "borsh-schema-derive-internal"
version = "0.7.2"
source = "git+https://github.com/project-serum/borsh?branch=serum#337732a185f052d5ee0424127b04b89d455ffa81"
dependencies = [
"proc-macro2 1.0.24",
"quote 1.0.7",
"syn 1.0.53",
]

These changes seems to be introduced by this pr: #51

Also, it seems that you're also pinning syn at forked branch:
https://github.com/project-serum/borsh/blob/90fcdf51f4c0333002a00a2db7367cc672882dc5/borsh-rs/borsh-derive-internal/Cargo.toml#L17

by this commit https://github.com/project-serum/borsh/pull/1/commits/884c56711c732d172f5d65374feff1fb796d4841

It may become unnecessary?

Also, updated borsh-rs isn't pinning syn it seems: https://github.com/near/borsh-rs/blob/3dddc6d7af2e66bf4f5947c0187a9d1f7dae6888/borsh-schema-derive-internal/Cargo.toml#L16

So, I'm thinking you can just update borsh-rs to v0.8.1, considering it's not pinning syn version: https://crates.io/crates/borsh-derive-internal/0.8.1

(A bit odd thing is that it seems spl succeeded to build with syn v1.0.60 and it seems pinning syn isn't working in your repository for some reason...)

CC: @armaniferrante (mentioning based on relevant commit's author info, nice to meet you!), @mvines (my dependable rust dependency-hell solving expert.)

@armaniferrante
Copy link
Contributor

Thanks for the issue @ryoqun. I'm happy to update our borsh fork and no longer pin syn (we pinned it as a temporary workaround for near/borsh-rs#2). I'll probably get to it sometime next week, likely. If it's more urgent, please let me know and I'm happy to to get to it sooner.

@armaniferrante armaniferrante linked a pull request Jan 31, 2021 that will close this issue
@armaniferrante
Copy link
Contributor

@ryoqun I've updated our borsh fork. If there's any other issues, let me know.

@ryoqun
Copy link
Author

ryoqun commented Feb 5, 2021

@armaniferrante Thanks for updating. :) Yeah, this fixed the build failure: https://buildkite.com/solana-labs/solana/builds/39724#4ff4ad78-e425-4e79-8e03-3467705a5708/1403

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants