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

Bump borsh to 0.10.3 #30975

Merged
merged 2 commits into from
May 30, 2023
Merged

Bump borsh to 0.10.3 #30975

merged 2 commits into from
May 30, 2023

Conversation

kevinji
Copy link
Contributor

@kevinji kevinji commented Mar 29, 2023

Problem

borsh is on 0.9.3 right now.

Summary of Changes

Bumps all uses of borsh to 0.10.3. The only breaking change is to the interface of BorshDeserialize, which was updated in one spot, but otherwise the serialization remains the same.

transaction-status relies on SPL which still requires borsh 0.9, so until SPL also gets updated that package alone will use an older version of borsh.

Fixes #

@mergify mergify bot added community Community contribution need:merge-assist labels Mar 29, 2023
@mergify mergify bot requested a review from a team March 29, 2023 21:46
@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Apr 3, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Apr 3, 2023
Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look great, thanks for doing this! Just one tiny nit to address.

There's also currently an error in CI because the Cargo.lock changes haven't been propagated to the other lockfile in programs/sbf.

To update the other lockfile, you'll need to run ./scripts/cargo-for-all-lock-files.sh tree from the top-level of the repo and then commit your changes.

I ran clippy, fmt, and the tests against this locally, so it should pass after that. Ping me here once you've updated the branch so I can kick off CI!

transaction-status/Cargo.toml Show resolved Hide resolved
@kevinji
Copy link
Contributor Author

kevinji commented Apr 5, 2023

@joncinque Just pushed a commit addressing your comments.

@joncinque joncinque added the CI Pull Request is ready to enter CI label Apr 5, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Apr 5, 2023
@kevinji
Copy link
Contributor Author

kevinji commented Apr 5, 2023

The build failures seem to be because of SPL compile issues which would be expected for now until SPL also switches over to borsh 0.10. Is there anything that should be done here?

@joncinque
Copy link
Contributor

@kevinji we had a discussion about this internally. When we upgrade borsh, we need to disable the downstream CI jobs because of the new incompatibility, e.g. #18218 (comment) --

To fix that, you'll need to comment out the lines for spl and openbook-dex and reorder at https://github.com/solana-labs/solana/blob/master/.buildkite/scripts/build-downstream-projects.sh

Since we'll need to disable CI, we'd like to hold off merging this until we're about to branch 1.16.0. The timeline for that isn't clear unfortunately, so we may need to keep this open for some time.

@kevinji
Copy link
Contributor Author

kevinji commented Apr 7, 2023

Makes sense; feel free to leave this PR alone until that happens.

@joncinque joncinque added the CI Pull Request is ready to enter CI label Apr 7, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Apr 7, 2023
@joncinque
Copy link
Contributor

Thanks so much for taking care of this so quickly and for the great work!

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #30975 (b444824) into master (a787831) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #30975     +/-   ##
=========================================
- Coverage    81.5%    81.4%   -0.1%     
=========================================
  Files         728      728             
  Lines      205895   205986     +91     
=========================================
+ Hits       167824   167841     +17     
- Misses      38071    38145     +74     

@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 24, 2023
@joncinque joncinque removed the stale [bot only] Added to stale content; results in auto-close after a week. label Apr 26, 2023
@github-actions github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label May 11, 2023
@joncinque joncinque removed the stale [bot only] Added to stale content; results in auto-close after a week. label May 12, 2023
@CriesofCarrots CriesofCarrots added the do-not-close Add this tag to exempt an issue/PR from being closed by the stalebot label May 19, 2023
@joncinque
Copy link
Contributor

It looks like 1.16 branch cut is imminent so we should rebase and land this asap -- do you have time to do that @kevinji ? If not, I can take care of it.

Any objections about that @CriesofCarrots ?

@kevinji
Copy link
Contributor Author

kevinji commented May 30, 2023

I don’t currently have the bandwidth for this so if you could take care of the rebase that would be great!

transaction-status relies on SPL which still requires borsh 0.9, so
until SPL also gets updated that package alone will use an older version of
borsh.
@joncinque
Copy link
Contributor

@willhickey this one

@joncinque
Copy link
Contributor

@kevinji thanks for being so responsive! I went ahead and rebased. @CriesofCarrots can you take one more look?

@joncinque joncinque added the v1.16 PRs that should be backported to v1.16 label May 30, 2023
@joncinque joncinque merged commit 8c73a2c into solana-labs:master May 30, 2023
mergify bot pushed a commit that referenced this pull request May 30, 2023
* Bump borsh to 0.10.3

transaction-status relies on SPL which still requires borsh 0.9, so
until SPL also gets updated that package alone will use an older version of
borsh.

* ci: Temporarily disable spl and openbook-dex builds

(cherry picked from commit 8c73a2c)
mergify bot added a commit that referenced this pull request May 30, 2023
Bump borsh to 0.10.3 (#30975)

* Bump borsh to 0.10.3

transaction-status relies on SPL which still requires borsh 0.9, so
until SPL also gets updated that package alone will use an older version of
borsh.

* ci: Temporarily disable spl and openbook-dex builds

(cherry picked from commit 8c73a2c)

Co-authored-by: Kevin Ji <1146876+kevinji@users.noreply.github.com>
yihau added a commit to yihau/solana that referenced this pull request May 31, 2023
@kevinji kevinji deleted the borsh-0.10 branch June 4, 2023 13:21
@acheroncrypto
Copy link
Contributor

This bump has caused massive amounts of dependency issues for all programs that depend on other versions of borsh.

Not only this has broken all older program dependencies, it has also broken publishing to crates.io when you have various dependencies with different borsh versions.

Even with the same Solana version(1.16.0) crates differ on which borsh version they use, e.g. solana-transaction-status is still on 0.9.3 when others are not.

@joncinque
Copy link
Contributor

It might be worth creating a separate issue to discuss this -- do you have an example setup that shows the issue? solana-transaction-status uses both versions

# NOTE: Use the workspace version once spl-associated-token-account uses borsh 0.10.
borsh0-9 = { package = "borsh", version = "0.9.3" }
to get around the issue for example.

It might be that certain projects can't upgrade to 1.16 until all of the other dependencies do too, or they might need to use 1.14 and 1.16 concurrently to get around dep issues.

We've had these bumps before, and it's certainly annoying and takes time (see #18218) but it's doable

@KirillLykov
Copy link
Contributor

@joncinque this PR disabled the spl and opendex downstream builds, are we ready to re-enable them and if not, are you aware if we have an issue to track this (if not I will create one)?

@joncinque
Copy link
Contributor

Correct, we can re-enable them, but we'll have to add a fix to maintain borsh's transitive dependency on hashbrown to 0.12.3, since cargo will automatically upgrade it during the downstream build, causing the program builds to fail.

@KirillLykov
Copy link
Contributor

Correct, we can re-enable them, but we'll have to add a fix to maintain borsh's transitive dependency on hashbrown to 0.12.3, since cargo will automatically upgrade it during the downstream build, causing the program builds to fail.

I've created an issue to track this #32378

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Community contribution do-not-close Add this tag to exempt an issue/PR from being closed by the stalebot need:merge-assist v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants