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

Should we allow breaking changes to crate interfaces outside of major version bumps #3724

Open
willhickey opened this issue Nov 20, 2024 · 9 comments

Comments

@willhickey
Copy link

PR 3377 has ignited a philosophical debate which warrants public input.

Context

The PR makes small breaking changes to the public interface on crates. The crates in question were not really intended to be a public interface, but we know they are being used directly outside of the Agave codebase.

The argument against PR 3377:

The interfaces are public, regardless of our preconceptions about which ones should be used. Since we're no longer in beta (v0.x) we should only break public interfaces with major version bumps. Changes like this should be rolled out with new functions that wrap the old ones and deprecations on the old functions.

The argument in favor of PR 3377:

We should only guarantee the interfaces on crates that we intended for public use. Developers interfacing directly with other crates can do so at their own risk. Introducing friction for changing interfaces reduces the likelihood that Agave developers will take the time to clean up existing code (eg functions with #[allow(clippy::too_many_arguments)])

@t-nelson
Copy link

how this should probably work is that we featurize everything non-public under agave-internal, make that feature non-default, then anything we're comfortable adhering to semver with moves out from under the feature

@steviez
Copy link

steviez commented Dec 9, 2024

how this should probably work is that we featurize everything non-public under agave-internal, make that feature non-default,

I like this idea and am in favor of it. We want separate crates within our repo for better organization and modularization, but this stuff really shouldn't be public. So, pub(crate) is not an option for us for cross-crate deps (ie core and runtime)

then anything we're comfortable adhering to semver with moves out from under the feature

Along this line, I think it would be interesting to hear what teams are using. We can certainly hear them out if they want us to move things out of internal feature (or really just have stable interface), but I think we need to protect our ability to clean things up and iterate quickly too

@steviez
Copy link

steviez commented Dec 10, 2024

On a similar line of thought, once solana-sdk is evicted from the monorepo, I think it makes sense to still have an agave-sdk or agave-internal-sdk crate. A place for stuff like Packet, things common between ledger-tool and validator, etc to live

@t-nelson
Copy link

On a similar line of thought, once solana-sdk is evicted from the monorepo, I think it makes sense to still have an agave-sdk or agave-internal-sdk crate. A place for stuff like Packet, things common between ledger-tool and validator, etc to live

junk drawer libraries are an anti-pattern

@steviez
Copy link

steviez commented Dec 10, 2024

junk drawer libraries are an anti-pattern

The status quo is this random stuff scattered across sdk, ledger, core, runtime and who knows where else. Breaking each individual item into its' own crate is an option, but I'd be curious if you or anyone else reading this have any alternatives in mind

@t-nelson
Copy link

it's simple. each crate that has public api has it's own sdk module where the public interface lives. everything else is private

solana-core is also a junk drawer library <norman-rockwell.jpg>

@nickfrosty
Copy link

anything that is a breaking change should follow semver, that way ecosystem developers can get a consistent experience across the solana stack (like any dev would expect with any other tool). the solana ecosystem and the agave client is mature enough for this. lets stop shooting application layer devs in the foot

intended or not, some things are public and being used by the ecosystem. even if something was not meant to be public and used, it should only be removed (or made private) in a major version bump. if its public, some dev is bound to use it (like it or not). the only clean way to remove it (or make not public) is in a major version bump

it's simple. each crate that has public api has it's own sdk module where the public interface lives. everything else is private

wholeheartedly agree with @t-nelson here! it also helps keep iteration nimble like @steviez (and im sure the rest of the engineers) want. reduce the public surface area for any random person to use (because if its available, someone will inevitably use it for some reason). cut a new major version of each crate and make only the minimally desired things public. then continuing to follow semver becomes really easy

@steviez - we need to protect our ability to clean things up and iterate quickly too

if the public surface area is reduced, then it should help keep iteration nimble. if its public, expect at least someone to use it for some random reason. therefore must follow semver

@steviez
Copy link

steviez commented Dec 18, 2024

The status quo is this random stuff scattered across sdk, ledger, core, runtime and who knows where else. Breaking each individual item into its' own crate is an option, but I'd be curious if you or anyone else reading this have any alternatives in mind

it's simple. each crate that has public api has it's own sdk module where the public interface lives. everything else is private

Either I'm missing something in your response or I don't think you answered my question. Let's consider the type Packet as a concrete example:

  • This type is currently in solana-sdk (sdk/src/packet.rs) but probably shouldn't be
  • This type is used by solana-gossip, solana-turbine, solana-core, solana-perf and probably some others. It doesn't really matter exactly how many, just that it is more than 1 crate

Where does the type Packet live; in a solo crate ?

@steveluscher
Copy link

Where does the type Packet live; in a solo crate ?

This is spiritually similar to what we do on the JavaScript side; types like Lamports and Commitment live in their very own npm package.

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

No branches or pull requests

5 participants