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

Add more comments about solana-program patch (downstream) #33965

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

CriesofCarrots
Copy link
Contributor

Problem

I ran into the errors described at #33424 while developing a geyser plugin, despite having the patch statements in my Cargo.toml already. I didn't think to wipe my Cargo.lock until @joncinque suggested it, and it fixed all my problems.

The behavior feels a bit unintuitive. If having compiled successfully, you comment out the patch statements and recompile (fails), cargo adds the crates.io dependencies to the Cargo.lock. If you subsequently uncomment the patch statements and recompile, cargo does not remove these declarations from the Cargo.lock, so the compilation continues to fail.

Summary of Changes

We have helpful comments around the patch statements in the monorepo Cargo.tomls; extend them with some tips about using downstream.

Fixes #33424

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #33965 (337da6b) into master (ec0ddc9) will decrease coverage by 0.1%.
Report is 4 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #33965     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         811      811             
  Lines      219333   219333             
=========================================
- Hits       179762   179680     -82     
- Misses      39571    39653     +82     

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.

Just the one question, otherwise looks great!

Cargo.toml Outdated Show resolved Hide resolved
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.

Looks great to me, thanks for clarifying this!

@CriesofCarrots
Copy link
Contributor Author

I'm going to merge this, but feedback still welcome, @ilya-bobyr !

@CriesofCarrots CriesofCarrots merged commit e93725e into solana-labs:master Nov 7, 2023
28 checks passed
@ilya-bobyr
Copy link
Contributor

I'm going to merge this, but feedback still welcome, @ilya-bobyr !

Looks great to me!
Sorry, I did not review this yesterday.

I wish there would be a way to reduce these hacks, so that it would just work :)

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