[Feature] [Package System] Add option to override standard library version #13318
Conversation
⏱️ 2h 16m total CI duration on this PR
|
fEst1ck
requested review from
gregnazario,
banool,
davidiw,
movekevin and
wrwg
as code owners
fEst1ck
requested review from
vgao1996
and removed request for
davidiw,
gregnazario,
banool and
movekevin
| } else { | ||
| bail!( | ||
| "Invalid verion for standard libraries to override: {}", | ||
| version_str |
There was a problem hiding this comment.
Could we say here the expected strings? See also my remark about ValueEnum derive below.
| } | ||
|
|
||
| /// Represents a standard library version. | ||
| pub enum StdVersion { |
There was a problem hiding this comment.
You could use #[derive(ValueEnum)] here and then use that type directly in the clap fields instead of strings. I believe this will give an error message about what are the allowed variants.
| Some(version) | ||
| } else { | ||
| bail!( | ||
| "Invalid verion for standard libraries to override: {}", |
| /// Returns the dependency for the standard library with the given version. | ||
| pub fn dependency(&self, version: &StdVersion) -> Dependency { | ||
| let local = | ||
| PathBuf::from(MOVE_HOME.clone()).join(format!("{}_{}", self.as_str(), version.rev())); |
There was a problem hiding this comment.
Is this the same location which is chosen if someone explicitly uses the github dep in their Move.toml? I'm worried that we are potentially double caching the package here. The local path should be the same to avoid this.
| @@ -109,6 +109,10 @@ pub struct BuildConfig { | |||
| #[clap(name = "test-mode", long = "test", global = true)] | |||
| pub test_mode: bool, | |||
|
|
|||
| /// Whether to override the standard library with the given version. | |||
| #[clap(long = "override-std", global = true, value_parser)] | |||
| pub override_std: Option<String>, | |||
There was a problem hiding this comment.
Yeah as @wrwg mentioned, you could change this to pub override_std: Option<StdVersion> if you implement the required traits for StdVersion.
| subdir = "aptos-move/framework/aptos-framework" | ||
|
|
||
| [dependencies.B] | ||
| local = "./deps_only" |
There was a problem hiding this comment.
Nit: the directory name "deps_only" seems a bit confusing to me. Wonder if it would be better to rename this to "intermediate_dep".
| subst: None, | ||
| version: None, | ||
| digest: None, | ||
| git_info: Some(GitInfo { |
There was a problem hiding this comment.
@wrwg Just to double check: what is the local caching behavior of the git dependencies? Do we update every time the build command is run, or it has to be triggered by some other events?
Specifically I want to understand what's going to happen when we update the branches in the repo.
Description
Closes #13326
Implements a
--override-std mainnet/testnet/devoption for overriding the version of standard library (aptos framework, aptos stdlib, move stdlib) as dependencies. For example, if--override-std mainnetis specified, a dependencywill effectively be overridden by the one from the well-know location for stdlib with the specified version
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Two new tests
third_pary/move/tools/move-package/tests/test_sources/compilation/std-lib-{conflicts, override}one showing the problem, one showing the solution.