Skip to content

program-test: remove solana-sdk dependency#6299

Merged
fkouteib merged 3 commits intoanza-xyz:masterfrom
fkouteib:sol-sdk-dep
May 28, 2025
Merged

program-test: remove solana-sdk dependency#6299
fkouteib merged 3 commits intoanza-xyz:masterfrom
fkouteib:sol-sdk-dep

Conversation

@fkouteib
Copy link
Copy Markdown

@fkouteib fkouteib commented May 23, 2025

Problem

As part of invalidator upstream sync, I hit CI failures related to these solana-sdk crate imports.

Summary of Changes

Remove the solana-sdk and solana-sdk-macro dependencies from program-test.

solana_commitment_config::CommitmentLevel,
solana_instruction::{AccountMeta, Instruction},
solana_msg::msg,
solana_program::program::{get_return_data, invoke, set_return_data},
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Per this guide from Kevin, the recomended replacement for solana_program::program is solana_cpi crate. However when I import the latter here, both tests fail. So I seem to be hitting a bug in that crate that I don't have the BW to debug at the moment. So I oped for using the "legacy" import for now to unblock my immediate problem. But I wanted to flag that here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

problem here is that the new implementations drop the sysvar_stubs module and just blindly return None. gonna need some thought to clear this one

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I looked into this a bit more. It's actually working as expected and I had an incorrect understanding of the two crates being functionally equivalent. The solana_cpi vs solana_{sdk; program}::program differences for these methods look intentional based on the implementation and comments.

The tests expect the overwrite as well.

/// Like [`solana_cpi::get_return_data`], but with support
/// for overwriting the `sol_get_return_data` syscall stub.
///
/// [`solana_cpi::get_return_data`]: https://docs.rs/solana-cpi/latest/solana_cpi/fn.get_return_data.html
pub fn get_return_data() -> Option<(Pubkey, Vec<u8>)> {
    #[cfg(target_os = "solana")]
    {
        solana_cpi::get_return_data()
    }

    #[cfg(not(target_os = "solana"))]
    crate::program_stubs::sol_get_return_data()
}

Considering this resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i mean solana-program is up next, but i guess this is a step in the right direction

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.8%. Comparing base (4ba219c) to head (0e9135e).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6299   +/-   ##
=======================================
  Coverage    82.8%    82.8%           
=======================================
  Files         846      846           
  Lines      378217   378215    -2     
=======================================
+ Hits       313234   313315   +81     
+ Misses      64983    64900   -83     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@t-nelson
Copy link
Copy Markdown

i think this one was superseded by #6302

Comment thread program-test/Cargo.toml Outdated
@fkouteib fkouteib changed the title More solana-sdk dependency deprecations program-test: remove solana-sdk dependency May 23, 2025
@fkouteib fkouteib requested review from kevinheavey and steviez May 27, 2025 21:01
@fkouteib
Copy link
Copy Markdown
Author

i think this one was superseded by #6302

The overlap was in 'core/src/banking_stage/latest_validator_vote_packet.rs', and got dropped in the rebase. So this is now strictly "program-test" clean-up.

@fkouteib
Copy link
Copy Markdown
Author

This is the last module dependent on solana-sdk. Once this PR lands, we can land #6333 then #6334 in that order.

Comment thread program-test/Cargo.toml
solana-sbpf = { workspace = true }
solana-sdk = { workspace = true }
solana-sdk-ids = { workspace = true }
solana-sdk-macro = { workspace = true }
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would expect this PR to remove solana-sdk from the list (2 lines up from here) as well

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Normally yes, but that's the last reference in the repo. If I do that, I'd have to pull in the changes from other PRs mentioned in my last comment, which are all the other cargo changes that are also deleted. I wanted to fix each in a separate PR to keep it clean and easier to review.

This is the last module dependent on solana-sdk. Once this PR lands, we can land #6333 then #6334 in that order.

Copy link
Copy Markdown

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

:shipit:

solana_commitment_config::CommitmentLevel,
solana_instruction::{AccountMeta, Instruction},
solana_msg::msg,
solana_program::program::{get_return_data, invoke, set_return_data},
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i mean solana-program is up next, but i guess this is a step in the right direction

@fkouteib fkouteib merged commit 3c82fba into anza-xyz:master May 28, 2025
58 checks passed
@fkouteib fkouteib deleted the sol-sdk-dep branch May 28, 2025 00:25
mircea-c pushed a commit to mircea-c/agave that referenced this pull request Jun 12, 2025
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 this pull request may close these issues.

5 participants