svm repo split: decouple dev deps: feature-set #7461
Conversation
buffalojoec
changed the title
buffalojoec
force-pushed
the
svm-repo-split-dedev-feature-set
branch
from
232a458 to
76b5210
Compare
buffalojoec
force-pushed
the
svm-repo-split-dedev-feature-set
branch
from
76b5210 to
09d3bd6
Compare
| impl From<&SVMFeatureSet> for FeatureSet { | ||
| fn from(svm_features: &SVMFeatureSet) -> Self { |
There was a problem hiding this comment.
I don't see any case when one should be converting the SVM feature set to the Agave feature set. How about marking this function as dev-context-only-utils?
There was a problem hiding this comment.
I definitely agree with this, SVM -> agave feature set conversion is lossy and should only be available in dev-context if we need it.
There was a problem hiding this comment.
Yeah but I can't without still requiring it as a dev dependency. Where would I specify I need the feature?
There was a problem hiding this comment.
Perhaps you could implement that in the test file?
There was a problem hiding this comment.
SVM -> agave feature set conversion is lossy
How is it lossy? Lossy would actually be the other direction.
There was a problem hiding this comment.
Perhaps you could implement that in the test file?
If I use FeatureSet in the test file, then solana-svm will require agave-feature-set as a dev dependency.
There was a problem hiding this comment.
"lossy" is the wrong word I guess. SVM -> Agave is not a straight-forward conversion. you have to make some assumption of what the other features in agave are. Should they all be active? inactive? there's no obvious choice to me which one is "correct".
There was a problem hiding this comment.
If I use FeatureSet in the test file, then solana-svm will require agave-feature-set as a dev dependency.
I don't understand though. The only place this into is used is in the svm integration tests.
It has to be a dev dependency already with this setup, no?
There was a problem hiding this comment.
like maybe it's not a direct dev-dependency. but it's somehow dependent on it because the process_compute_budget_instructions takes it. so that function depends on FeatureSet
There was a problem hiding this comment.
like maybe it's not a direct dev-dependency. but it's somehow dependent on it because the
process_compute_budget_instructionstakes it. so that function depends on FeatureSet
Yeah, maybe I need to go a few levels out to get rid of the dependency(ies). I'm trying to avoid Agave crates depending on SVM crates, which in turn depend on Agave crates for testing. It'll be a nightmare.
buffalojoec
force-pushed
the
svm-repo-split-dedev-feature-set
branch
from
09d3bd6 to
07c4474
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7461 +/- ##
=========================================
Coverage ? 83.3%
=========================================
Files ? 803
Lines ? 363677
Branches ? 0
=========================================
Hits ? 302959
Misses ? 60718
Partials ? 0 🚀 New features to boost your workflow:
|
Problem
We have to decouple any uses of
agave-crates in the dev-dependencies of the crates slated to move to the new SVM repo.Summary of Changes
Simply uses
SVMFeatureSetinstead of the runtime-levelFeatureSetfor SVM integration testing. This is actually more accurate, since no features outside ofSVMFeatureSetshould ever be required to test functionality within SVM.Part of #7317