-
Notifications
You must be signed in to change notification settings - Fork 157
Fixup build: remove stake-interface, bring system-interface back #264
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
Conversation
#### Problem The circular dependencies between the sdk, system-interface, and stake-interface make crate publishing very tricky, especially around breaking changes. #### Summary of changes To start breaking the chain, remove stake-interface from the repo. stake-interface will implement the sysvar trait itself as part of the next breaking change.
#### Problem Too many sdk crates depend on the system interface, which makes updating, building, and publishing a big headache. #### Summary of changes Declare defeat, and add the system interface back into the sdk repo. This will also remove circular dependencies completely, and make publishing a pleasure once again.
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
1 similar comment
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
9be72f2 to
e1eed08
Compare
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
| [features] | ||
| bincode = ["solana-system-interface/bincode"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks unrelated, but it's unfortunately needed.
The docs build fails without it, because example-mocks is re-exporting from system-interface, but doesn't have the bincode feature enabled, which means that the instructions referenced in the docs don't exist.
| [patch.crates-io] | ||
| # We include the following crates as our dependencies above from crates.io: | ||
| # | ||
| # * solana-system-interface | ||
| # | ||
| # They, in turn, depend on a number of crates that we also include directly | ||
| # using `path` specifications. For example, `solana-system-interface` depends | ||
| # on `solana-instruction`. And we explicitly specify `solana-instruction` above | ||
| # as a local path dependency: | ||
| # | ||
| # solana-instruction = { path = "instruction", version = "2.2.1" } | ||
| # | ||
| # Unfortunately, Cargo will try to resolve the `solana-system-interface` | ||
| # `solana-instruction` dependency only using what is available on crates.io. | ||
| # Crates.io normally contains a previous version of these crates, and we end up | ||
| # with two versions of `solana-instruction` and all of their dependencies in our | ||
| # build tree. | ||
| # | ||
| # If you are developing downstream using non-crates-io solana-program (local or | ||
| # forked repo, or from github rev, eg), duplicate the following patch statements | ||
| # in your Cargo.toml. If you still hit duplicate-type errors with the patch | ||
| # statements in place, run `cargo update -p solana-program` to remove extraneous | ||
| # versions from your Cargo.lock file. | ||
| solana-account = { path = "account" } | ||
| solana-clock = { path = "clock" } | ||
| solana-cpi = { path = "cpi" } | ||
| solana-frozen-abi = { path = "frozen-abi" } | ||
| solana-frozen-abi-macro = { path = "frozen-abi-macro" } | ||
| solana-instruction = { path = "instruction" } | ||
| solana-program-error = { path = "program-error" } | ||
| solana-pubkey = { path = "pubkey" } | ||
| solana-rent = { path = "rent" } | ||
| solana-signature = { path = "signature" } | ||
| solana-sysvar-id = { path = "sysvar-id" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the best part of this change, and the whole motivation
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
| //! [`Instruction`]: | ||
| //! https://docs.rs/solana-instruction/latest/solana_instruction/struct.Instruction.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only change in this file
| /// Handler for retrieving a slice of sysvar data from the `sol_get_sysvar` | ||
| /// syscall. | ||
| fn get_sysvar( | ||
| pub fn get_sysvar( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need this function when reimplementing the moved logic in solana-stake-interface, so make it public
sysvar/Cargo.toml
Outdated
| solana-frozen-abi-macro = { workspace = true, optional = true } | ||
| solana-hash = { workspace = true, features = ["bytemuck"] } | ||
| solana-instruction = { workspace = true } | ||
| solana-instruction = { workspace = true, features = ["std"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was being enabled through solana-stake-interface
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
rustopian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some improvements to error.rs, rm wasm as you mentioned, otherwise diffs are clean.
I noticed one sort-of-broken link in the docs. Up to you.
system-interface/README.md
Outdated
|
|
||
| # Solana System Interface | ||
|
|
||
| This crate contains instructions and constructors for interacting with the [System program](https://docs.solanalabs.com/runtime/programs#system-program). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really essential to this PR, but
| This crate contains instructions and constructors for interacting with the [System program](https://docs.solanalabs.com/runtime/programs#system-program). | |
| This crate contains instructions and constructors for interacting with the [System program](https://solana.com/docs/core/programs#core-programs). |
The old link has an awkward redirect notice... which arguably takes you to the wrong page anyway (account model).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me!
|
If this PR represents a change to the sysvar layout, please open a follow-up PR to update the JavaScript client |
#### Problem The stake-history module in solana-sysvar will be removed with anza-xyz#264, but there hasn't been a deprecation warning. #### Summary of changes Put in a deprecation warning.
|
For what it's worth, I know it kind of stinks to not use the program-specific repo for the system interface, but it's just too tied into the rest of the sdk unfortunately. I prefer the sanity of a self-contained repo over splitting things up. It'll also mean that tools like Also, since the plans for a BPF system program have been put on hold for a long time, the program-specific repo becomes a bit less necessary for the rust crates. We can certainly keep hosting the idl and kit library though. |
* sysvar: Deprecate stake-history and functions #### Problem The stake-history module in solana-sysvar will be removed with #264, but there hasn't been a deprecation warning. #### Summary of changes Put in a deprecation warning. * Improve deprecation notice
#### Problem As part of the upgrade to SDK v3, the cyclical dependency between the sdk crates and the system interface were too much of an annoyance, so the interface was migrated back into the SDK with anza-xyz/solana-sdk#264. But the interface code still lives in this repo as well. #### Summary of changes Remove the interface crate and all references to it.
#### Problem As part of the upgrade to SDK v3, the cyclical dependency between the sdk crates and the system interface were too much of an annoyance, so the interface was migrated back into the SDK with anza-xyz/solana-sdk#264. But the interface code still lives in this repo as well. #### Summary of changes Remove the interface crate and all references to it.
…a-xyz#264) * sysvar!: Remove stake-interface dependency The circular dependencies between the sdk, system-interface, and stake-interface make crate publishing very tricky, especially around breaking changes. To start breaking the chain, remove stake-interface from the repo. stake-interface will implement the sysvar trait itself as part of the next breaking change. * system-interface: Add back into the sdk repo Too many sdk crates depend on the system interface, which makes updating, building, and publishing a big headache. Declare defeat, and add the system interface back into the sdk repo. This will also remove circular dependencies completely, and make publishing a pleasure once again. * system-interface: Fixup post-migration * Add feature required for solana-instruction * Run cargo sort, specify circular dev deps with path * Pacify dep checker by specifying path dependency * Fixup docs build * Fix doc / powerset build issues * Run doctests with all features * Update broken link
Problem
The build is currently failing due to circular dependencies between the sdk, system-interface, and stake-interface. This is totally normal since we're publishing breaking changes, but in general, these circular dependencies can be tricky to manage. See #263 for more info.
Summary of changes
Remove the circular dependencies for good:
Work to do after: