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

Change visibility of svm-internal structs and APIs #4449

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pgarg66
Copy link

@pgarg66 pgarg66 commented Jan 14, 2025

Problem

Some APIs and structs in SVM crates are only used internally (among these crates). The visibility should be limited to these crates.

Summary of Changes

Limit the visibility using svm-internal cfg feature.

Fixes #

Copy link

mergify bot commented Jan 14, 2025

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@pgarg66 pgarg66 force-pushed the limit-pub branch 5 times, most recently from 1923f3b to 1a2f777 Compare January 14, 2025 17:37
@pgarg66 pgarg66 marked this pull request as ready for review January 14, 2025 18:22
@pgarg66 pgarg66 requested review from a team as code owners January 14, 2025 18:22
Lichtso
Lichtso previously approved these changes Jan 15, 2025
@@ -35,19 +39,23 @@ pub struct NotMigratingBuiltinCost {
/// When migration completed, eg the feature gate is enabled everywhere, please
/// remove that builtin entry from MIGRATING_BUILTINS_COSTS.
#[derive(Clone)]
pub enum BuiltinCost {
#[cfg_attr(feature = "svm-internal", qualifiers(pub))]
enum BuiltinCost {
Migrating(MigratingBuiltinCost),
NotMigrating(NotMigratingBuiltinCost),
}

impl BuiltinCost {

Choose a reason for hiding this comment

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

I'm confused by these qualifiers. if they are not pub now why do we need them to be?

Copy link
Author

Choose a reason for hiding this comment

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

Removing the qualifiers here causes the following build error:

warning: type `BuiltinCost` is more private than the item `MIGRATING_BUILTINS_COSTS`
   --> builtins-default-costs/src/lib.rs:120:49
    |
120 |   #[cfg_attr(feature = "svm-internal", qualifiers(pub))]
    |  _________________________________________________^
121 | | const MIGRATING_BUILTINS_COSTS: &[(Pubkey, BuiltinCost)] = &[
    | |________________________________________________________^ constant `MIGRATING_BUILTINS_COSTS` is reachable at visibility `pub`
    |
note: but type `BuiltinCost` is only usable at visibility `pub(crate)`
   --> builtins-default-costs/src/lib.rs:42:1
    |
42  | enum BuiltinCost {
    | ^^^^^^^^^^^^^^^^
    = note: `#[warn(private_interfaces)]` on by default

   Compiling solana-compute-budget-instruction v2.2.0 (/Users/pankaj/workspace/solana/compute-budget-instruction)
warning: `solana-builtins-default-costs` (lib) generated 1 warning
error: type `solana_builtins_default_costs::BuiltinCost` is private
  --> compute-budget-instruction/src/compute_budget_instruction_details.rs:31:48
   |
31 |             migrating_builtin: [Saturating(0); MIGRATING_BUILTINS_COSTS.len()],
   |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ private type

error: could not compile `solana-compute-budget-instruction` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

And, MIGRATING_BUILTINS_COSTS is used by compute-budget-instruction/src/compute_budget_instruction_details.rs. So it needs to be public (at least within SVM internal crates).

It's a bit convoluted. I had to go through multiple compilation failures to get to this set of qualifiers. I wish it could be cleaner.

@pgarg66 pgarg66 requested a review from apfitzge January 15, 2025 16:04
@pgarg66 pgarg66 force-pushed the limit-pub branch 2 times, most recently from d42a313 to c11f31f Compare January 15, 2025 22:12
@pgarg66 pgarg66 added the CI Pull Request is ready to enter CI label Jan 16, 2025
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Jan 16, 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.

4 participants