search

project-serum / serum-dex

Project Serum Rust Monorepo
https://projectserum.com
Apache License 2.0
625 stars 327 forks source link

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

Closed ryoqun closed 3 years ago

ryoqun commented 3 years ago

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: https://github.com/near/borsh-rs/issues/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.

https://github.com/project-serum/serum-dex/blob/d47058600c3ba8ffe83669ebca1e50c49f780dc5/pool/Cargo.toml#L20

https://github.com/project-serum/serum-dex/blob/d47058600c3ba8ffe83669ebca1e50c49f780dc5/Cargo.lock#L238-L275

These changes seems to be introduced by this pr: https://github.com/project-serum/serum-dex/pull/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 commented 3 years ago

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 https://github.com/near/borsh-rs/issues/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 commented 3 years ago

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

ryoqun commented 3 years ago

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