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.

Sign up for GitHub

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

[CLI] Set Move 2 as default in Aptos CLI and add move-1 flag #15431

Merged
merged 6 commits into from
Jan 13, 2025
+65 −25
Merged

[CLI] Set Move 2 as default in Aptos CLI and add move-1 flag #15431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a51f84
add move-1 flag
rahxephon89 Nov 26, 2024
03bd97e
governance
rahxephon89 Jan 6, 2025
1748792
set default as v2
rahxephon89 Jan 7, 2025
38a1522
Update crates/aptos/src/move_tool/mod.rs
rahxephon89 Jan 8, 2025
7c7159a
Merge branch 'main' into teng/add-move-1-flags
rahxephon89 Jan 12, 2025
318452a
Merge branch 'main' into teng/add-move-1-flags
rahxephon89 Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion crates/aptos/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ All notable changes to the Aptos CLI will be captured in this file. This project
# Unreleased
- Add flag `--benchmark` to `aptos move prove`, which allows to benchmark verification times of individual functions in a package.
- Add flag `--only <name>` to `aptos move prove`, which allows to scope verification to a function.

- Fix `aptos init` to show the explorer link for accounts when account is already created on chain instead of prompting to fund the account.
- Set Compiler v2 as the default compiler and Move 2 as the default language version.
- Add new `--move-1` flag to use Compiler v1 and Move 1.

## [5.1.0] - 2024/12/13
- More optimizations are now default for compiler v2.
Expand Down
22 changes: 16 additions & 6 deletions crates/aptos/src/common/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ use aptos_types::{
};
use aptos_vm_types::output::VMOutput;
use async_trait::async_trait;
use clap::{Parser, ValueEnum};
use clap::{ArgGroup, Parser, ValueEnum};
use hex::FromHexError;
use indoc::indoc;
use move_core_types::{
Expand Down Expand Up @@ -1110,6 +1110,7 @@ impl FromStr for OptimizationLevel {

/// Options for compiling a move package dir
#[derive(Debug, Clone, Parser)]
#[clap(group = ArgGroup::new("move-version").args(&["move_1", "move_2"]).required(false))]
pub struct MovePackageDir {
/// Path to a move package (the folder with a Move.toml file). Defaults to current directory.
#[clap(long, value_parser)]
Expand Down Expand Up @@ -1179,25 +1180,33 @@ pub struct MovePackageDir {

/// ...or --compiler COMPILER_VERSION
/// Specify the version of the compiler.
/// Defaults to `1`, unless `--move-2` is selected.
/// Defaults to the latest stable compiler version (at least 2)
#[clap(long, value_parser = clap::value_parser!(CompilerVersion),
alias = "compiler",
default_value = LATEST_STABLE_COMPILER_VERSION,
rahxephon89 marked this conversation as resolved.
Show resolved Hide resolved
default_value_if("move_2", "true", LATEST_STABLE_COMPILER_VERSION),
default_value_if("move_1", "true", "1"),
rahxephon89 marked this conversation as resolved.
Show resolved Hide resolved
verbatim_doc_comment)]
pub compiler_version: Option<CompilerVersion>,

/// ...or --language LANGUAGE_VERSION
/// Specify the language version to be supported.
/// Defaults to `1`, unless `--move-2` is selected.
/// Defaults to the latest stable language version (at least 2)
#[clap(long, value_parser = clap::value_parser!(LanguageVersion),
alias = "language",
default_value = LATEST_STABLE_LANGUAGE_VERSION,
default_value_if("move_2", "true", LATEST_STABLE_LANGUAGE_VERSION),
default_value_if("move_1", "true", "1"),
verbatim_doc_comment)]
pub language_version: Option<LanguageVersion>,

/// Select bytecode, language, and compiler versions to support the latest Move 2.
#[clap(long, verbatim_doc_comment)]
pub move_2: bool,

/// Select bytecode, language, and compiler versions for Move 1.
#[clap(long, verbatim_doc_comment)]
pub move_1: bool,
}

impl Default for MovePackageDir {
Expand All @@ -1216,11 +1225,12 @@ impl MovePackageDir {
override_std: None,
skip_fetch_latest_git_deps: true,
bytecode_version: None,
compiler_version: None,
language_version: None,
compiler_version: Some(CompilerVersion::latest_stable()),
language_version: Some(LanguageVersion::latest_stable()),
skip_attribute_checks: false,
check_test_code: false,
move_2: false,
move_2: true,
move_1: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a move_version field that takes an enum instead? Because I assume it's invalid to set both to true or false?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, but perhaps we should have either just keep this as move_2 with true the default, or remove one of the flags. Enum seems overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @banool, I was thinking of the same idea but we may still need to keep the move-2 flag in case some eco system projects depend on this flag to compile their smart contracts in CI? Enum seems be an overkill because move-1 will only be used in urgent time and will be removed when V1 retires. Also, external users cannot set both of them to the same value because they are put in the same ArgGroup.

optimize: None,
experiments: vec![],
}
Expand Down
24 changes: 19 additions & 5 deletions crates/aptos/src/governance/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,17 +915,29 @@ pub struct CompileScriptFunction {
#[clap(long, default_value_if("move_2", "true", "7"))]
pub bytecode_version: Option<u32>,

/// Specify the version of the compiler.
/// Defaults to the latest stable compiler version (at least 2)
#[clap(long, value_parser = clap::value_parser!(CompilerVersion),
default_value_if("move_2", "true", LATEST_STABLE_COMPILER_VERSION))]
default_value = LATEST_STABLE_COMPILER_VERSION,
default_value_if("move_2", "true", LATEST_STABLE_COMPILER_VERSION),
default_value_if("move_1", "true", "1"),)]
pub compiler_version: Option<CompilerVersion>,

/// Specify the language version to be supported.
/// Defaults to the latest stable language version (at least 2)
#[clap(long, value_parser = clap::value_parser!(LanguageVersion),
default_value_if("move_2", "true", LATEST_STABLE_LANGUAGE_VERSION))]
default_value = LATEST_STABLE_LANGUAGE_VERSION,
default_value_if("move_2", "true", LATEST_STABLE_LANGUAGE_VERSION),
default_value_if("move_1", "true", "1"),)]
pub language_version: Option<LanguageVersion>,

/// Select bytecode, language, compiler for Move 2
#[clap(long)]
#[clap(long, default_value_t = true)]
pub move_2: bool,

/// Select bytecode, language, and compiler versions for Move 1.
#[clap(long, default_value_t = false)]
pub move_1: bool,
}

impl CompileScriptFunction {
Expand Down Expand Up @@ -971,8 +983,10 @@ impl CompileScriptFunction {
&self.framework_package_args,
prompt_options,
self.bytecode_version,
self.language_version,
self.compiler_version,
self.language_version
.or_else(|| Some(LanguageVersion::latest_stable())),
self.compiler_version
.or_else(|| Some(CompilerVersion::latest_stable())),
)
}
}
Expand Down
9 changes: 7 additions & 2 deletions crates/aptos/src/move_tool/coverage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use move_coverage::{
summary::summarize_inst_cov,
};
use move_disassembler::disassembler::Disassembler;
use move_model::metadata::{CompilerVersion, LanguageVersion};
use move_package::{compilation::compiled_package::CompiledPackage, BuildConfig, CompilerConfig};

/// Display a coverage summary for all modules in a package
Expand Down Expand Up @@ -175,8 +176,12 @@ fn compile_coverage(
move_options.bytecode_version,
move_options.language_version,
),
compiler_version: move_options.compiler_version,
language_version: move_options.language_version,
compiler_version: move_options
.compiler_version
.or_else(|| Some(CompilerVersion::latest_stable())),
language_version: move_options
.language_version
.or_else(|| Some(LanguageVersion::latest_stable())),
experiments: experiments_from_opt_level(&move_options.optimize),
},
..Default::default()
Expand Down
32 changes: 21 additions & 11 deletions crates/aptos/src/move_tool/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,8 +564,14 @@ impl CliCommand<&'static str> for TestPackage {
self.move_options.bytecode_version,
self.move_options.language_version,
),
compiler_version: self.move_options.compiler_version,
language_version: self.move_options.language_version,
compiler_version: self
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these changes needed? This should just be the same logic, based on different defaults in MoveOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this change mainly to make sure that so long as move_options is not set to V1 explicitly, it will always pass V2 to the compiler config.

.move_options
.compiler_version
.or_else(|| Some(CompilerVersion::latest_stable())),
language_version: self
.move_options
.language_version
.or_else(|| Some(LanguageVersion::latest_stable())),
experiments: experiments_from_opt_level(&self.move_options.optimize),
},
..Default::default()
Expand Down Expand Up @@ -717,8 +723,12 @@ impl CliCommand<&'static str> for DocumentPackage {
move_options.bytecode_version,
move_options.language_version,
),
compiler_version: move_options.compiler_version,
language_version: move_options.language_version,
compiler_version: move_options
.compiler_version
.or_else(|| Some(CompilerVersion::latest_stable())),
language_version: move_options
.language_version
.or_else(|| Some(LanguageVersion::latest_stable())),
skip_attribute_checks: move_options.skip_attribute_checks,
check_test_code: move_options.check_test_code,
known_attributes: extended_checks::get_all_attribute_names().clone(),
Expand Down Expand Up @@ -899,20 +909,20 @@ impl IncludedArtifacts {
let override_std = move_options.override_std.clone();
let bytecode_version =
fix_bytecode_version(move_options.bytecode_version, move_options.language_version);
let compiler_version = move_options.compiler_version;
let language_version = move_options.language_version;
let compiler_version = move_options
.compiler_version
.or_else(|| Some(CompilerVersion::latest_stable()));
let language_version = move_options
.language_version
.or_else(|| Some(LanguageVersion::latest_stable()));
let skip_attribute_checks = move_options.skip_attribute_checks;
let check_test_code = move_options.check_test_code;
let optimize = move_options.optimize.clone();
let mut experiments = experiments_from_opt_level(&optimize);
experiments.append(&mut move_options.experiments.clone());
experiments.append(&mut more_experiments);

// TODO(#14441): Remove `None |` here when we update default CompilerVersion
rahxephon89 marked this conversation as resolved.
Show resolved Hide resolved
if matches!(
move_options.compiler_version,
Option::None | Some(CompilerVersion::V1)
) {
if matches!(compiler_version, Some(CompilerVersion::V1)) {
if !matches!(optimize, Option::None | Some(OptimizationLevel::Default)) {
return Err(CliError::CommandArgumentError(
"`--optimization-level`/`--optimize` flag is not compatible with Move Compiler V1"
Expand Down
Loading