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

Rust BPF programs depend on Solana SDK #5819

Merged
merged 2 commits into from Sep 6, 2019

Conversation

@jackcmay
Copy link
Contributor

commented Sep 6, 2019

Problem

Rust BPF programs have their own SDK

Summary of Changes

Do the bare minimum to centralize the SDK in one crate

  • The bulk of the changed files are pointing the BPF test programs to solana_sdk
  • The interesting changes are in sdk/Cargo.toml and sdk/src/lib.rs
  • Also, in sdk/src/pubkey.rs the rand and file dependencies are #[cfg]'d out for now

The idea is that now that Rust BPF programs depend on solana_sdk they can be slowly moved over to `solana_sdk' types.

The SDK has also turned into a general inc folder containing files that maybe should not be in the SDK (native_loader.rs). These changes highlight that and the kitchen_sink should probably turn into client or something similar once those files are moved out of the SDK.

An alternative solution would be to break the SDK into 3 different crates. One for programs, one for clients, and one common one, where the first two depend on the last.

Fixes #

Copy link
Member

left a comment

I like this way more!

sdk/src/lib.rs Outdated Show resolved Hide resolved
pub mod transport;

// On-chain program modules
cfg_if! {

This comment has been minimized.

Copy link
@mvines

mvines Sep 6, 2019

Member

Do we run into issues if we also include these three modules in the kitchen sink? Just wondering

This comment has been minimized.

Copy link
@jackcmay

jackcmay Sep 6, 2019

Author Contributor

I think those three modules could be built along with the kitchen sink. They are program-specific, do you have a reason to do so?

This comment has been minimized.

Copy link
@mvines

mvines Sep 6, 2019

Member

I don't have a strong use case, loosely thinking that it could be useful to have access in a kitchen sink unit test. But nbd right now, we can do that later if needed

nit
@codecov

This comment has been minimized.

Copy link

commented Sep 6, 2019

Codecov Report

Merging #5819 into master will decrease coverage by 0.2%.
The diff coverage is 57.1%.

@@           Coverage Diff            @@
##           master   #5819     +/-   ##
========================================
- Coverage    75.7%   75.4%   -0.3%     
========================================
  Files         226     226             
  Lines       41866   41855     -11     
========================================
- Hits        31694   31593    -101     
- Misses      10172   10262     +90
@jackcmay

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

There is one issue with this method. Using cfg_if! to pull in modules with macros results in difficulty using those macros within the same crate. II couldn't figure out a solution so I just pulled pub mod pubkey out of the cfg_if! macro. The other option is to no use cfg_if! and create a local submodule and re-export it but cargo fmt barfs on it. The two best options I found are outlined here, open to any other solutions: https://stackoverflow.com/questions/43070430/is-there-a-way-to-use-the-cfgfeature-check-on-multiple-statements

@mvines
mvines approved these changes Sep 6, 2019
@jackcmay jackcmay merged commit e5f9023 into solana-labs:master Sep 6, 2019
10 checks passed
10 checks passed
Summary 1 rule matches and 7 potential rules
Details
buildkite/solana Build #11353 passed (32 minutes, 32 seconds)
Details
buildkite/solana/bench Passed (13 minutes, 22 seconds)
Details
buildkite/solana/checks Passed (7 minutes, 1 second)
Details
buildkite/solana/coverage Passed (25 minutes, 18 seconds)
Details
buildkite/solana/pipeline-upload Passed (2 seconds)
Details
buildkite/solana/shellcheck Passed (22 seconds)
Details
buildkite/solana/stable Passed (24 minutes, 2 seconds)
Details
buildkite/solana/stable-perf Passed (18 minutes, 44 seconds)
Details
ci-gate Pull Request accepted for CI pipeline
@jackcmay jackcmay deleted the jackcmay:rust-bpf-depend-on-solana-sdk branch Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.