[package] Allow bytecode version to be set from input (#919)

Right now, the bytecode version can only be set by environment variable, which is a bit messy. This ensures that the input can be set from other ways than just the environment variable.
move-language · Mar 9, 2023 · 4daf3d1 · 4daf3d1
1 parent f5daa57
commit 4daf3d1
Show file tree

Hide file tree

Showing 41 changed files with 610 additions and 29 deletions.
diff --git a/language/move-analyzer/src/symbols.rs b/language/move-analyzer/src/symbols.rs
@@ -647,7 +647,7 @@ impl Symbolicator {
         let build_plan = BuildPlan::create(resolution_graph)?;
         let mut typed_ast = None;
         let mut diagnostics = None;
-        build_plan.compile_with_driver(&mut std::io::sink(), |compiler| {
+        build_plan.compile_with_driver(&mut std::io::sink(), None, |compiler| {
             let (files, compilation_result) = compiler.run::<PASS_TYPING>()?;
             let (_, compiler) = match compilation_result {
                 Ok(v) => v,

diff --git a/language/move-command-line-common/src/env.rs b/language/move-command-line-common/src/env.rs
@@ -10,10 +10,16 @@ const BYTECODE_VERSION_ENV_VAR: &str = "MOVE_BYTECODE_VERSION";
 
 /// Get the bytecode version from the environment variable.
 // TODO: This should be configurable via toml and command line flags. See also #129.
-pub fn get_bytecode_version_from_env() -> Option<u32> {
-    std::env::var(BYTECODE_VERSION_ENV_VAR)
-        .ok()
-        .and_then(|s| s.parse::<u32>().ok())
+pub fn get_bytecode_version_from_env(from_input: Option<u32>) -> Option<u32> {
+    // This allows for bytecode version to come from command line flags and
+    // other input locations, falls back to bytecode version if not provided
+    if from_input.is_some() {
+        from_input
+    } else {
+        std::env::var(BYTECODE_VERSION_ENV_VAR)
+            .ok()
+            .and_then(|s| s.parse::<u32>().ok())
+    }
 }
 
 pub fn read_env_var(v: &str) -> String {

diff --git a/language/tools/move-cli/src/base/test.rs b/language/tools/move-cli/src/base/test.rs
@@ -199,7 +199,7 @@ pub fn run_move_unit_tests<W: Write + Send>(
     // Move package system, to first grab the compilation env, construct the test plan from it, and
     // then save it, before resuming the rest of the compilation and returning the results and
     // control back to the Move package system.
-    build_plan.compile_with_driver(writer, |compiler| {
+    build_plan.compile_with_driver(writer, None, |compiler| {
         let (files, comments_and_compiler_res) = compiler.run::<PASS_CFGIR>().unwrap();
         let (_, compiler) =
             diagnostics::unwrap_or_report_diagnostics(&files, comments_and_compiler_res);

diff --git a/language/tools/move-cli/src/experimental/cli.rs b/language/tools/move-cli/src/experimental/cli.rs
@@ -100,7 +100,7 @@ impl ExperimentalCommand {
                 concretize,
             } => {
                 let state = PackageContext::new(&move_args.package_path, &move_args.build_config)?
-                    .prepare_state(storage_dir)?;
+                    .prepare_state(None, storage_dir)?;
                 experimental::commands::analyze_read_write_set(
                     &state,
                     module_file,

diff --git a/language/tools/move-cli/src/sandbox/cli.rs b/language/tools/move-cli/src/sandbox/cli.rs
@@ -203,6 +203,7 @@ impl SandboxCommand {
         move_args: &Move,
         storage_dir: &Path,
     ) -> Result<()> {
+        let bytecode_version = None;
         match self {
             SandboxCommand::Publish {
                 no_republish,
@@ -213,12 +214,13 @@ impl SandboxCommand {
             } => {
                 let context =
                     PackageContext::new(&move_args.package_path, &move_args.build_config)?;
-                let state = context.prepare_state(storage_dir)?;
+                let state = context.prepare_state(bytecode_version, storage_dir)?;
                 sandbox::commands::publish(
                     natives,
                     cost_table,
                     &state,
                     context.package(),
+                    bytecode_version,
                     *no_republish,
                     *ignore_breaking_changes,
                     *with_deps,
@@ -238,7 +240,7 @@ impl SandboxCommand {
             } => {
                 let context =
                     PackageContext::new(&move_args.package_path, &move_args.build_config)?;
-                let state = context.prepare_state(storage_dir)?;
+                let state = context.prepare_state(bytecode_version, storage_dir)?;
                 sandbox::commands::run(
                     natives,
                     cost_table,
@@ -251,6 +253,7 @@ impl SandboxCommand {
                     args,
                     type_args.to_vec(),
                     *gas_budget,
+                    bytecode_version,
                     *dry_run,
                     move_args.verbose,
                 )
@@ -269,7 +272,7 @@ impl SandboxCommand {
             ),
             SandboxCommand::View { file } => {
                 let state = PackageContext::new(&move_args.package_path, &move_args.build_config)?
-                    .prepare_state(storage_dir)?;
+                    .prepare_state(bytecode_version, storage_dir)?;
                 sandbox::commands::view(&state, file)
             }
             SandboxCommand::Clean {} => {
@@ -295,12 +298,12 @@ impl SandboxCommand {
             }
             SandboxCommand::Doctor {} => {
                 let state = PackageContext::new(&move_args.package_path, &move_args.build_config)?
-                    .prepare_state(storage_dir)?;
+                    .prepare_state(bytecode_version, storage_dir)?;
                 sandbox::commands::doctor(&state)
             }
             SandboxCommand::Generate { cmd } => {
                 let state = PackageContext::new(&move_args.package_path, &move_args.build_config)?
-                    .prepare_state(storage_dir)?;
+                    .prepare_state(bytecode_version, storage_dir)?;
                 handle_generate_commands(cmd, &state)
             }
         }

diff --git a/language/tools/move-cli/src/sandbox/commands/publish.rs b/language/tools/move-cli/src/sandbox/commands/publish.rs
@@ -22,6 +22,7 @@ pub fn publish(
     cost_table: &CostTable,
     state: &OnDiskStateView,
     package: &CompiledPackage,
+    bytecode_version: Option<u32>,
     no_republish: bool,
     ignore_breaking_changes: bool,
     with_deps: bool,
@@ -81,7 +82,7 @@ pub fn publish(
         }
     }
 
-    let bytecode_version = get_bytecode_version_from_env();
+    let bytecode_version = get_bytecode_version_from_env(bytecode_version);
 
     // use the the publish_module API from the VM if we do not allow breaking changes
     if !ignore_breaking_changes {

diff --git a/language/tools/move-cli/src/sandbox/commands/run.rs b/language/tools/move-cli/src/sandbox/commands/run.rs
@@ -25,6 +25,7 @@ use move_vm_runtime::move_vm::MoveVM;
 use move_vm_test_utils::gas_schedule::CostTable;
 use std::{fs, path::Path};
 
+#[allow(clippy::too_many_arguments)]
 pub fn run(
     natives: impl IntoIterator<Item = NativeFunctionRecord>,
     cost_table: &CostTable,
@@ -37,13 +38,14 @@ pub fn run(
     txn_args: &[TransactionArgument],
     vm_type_args: Vec<TypeTag>,
     gas_budget: Option<u64>,
+    bytecode_version: Option<u32>,
     dry_run: bool,
     verbose: bool,
 ) -> Result<()> {
     if !script_path.exists() {
         bail!("Script file {:?} does not exist", script_path)
     };
-    let bytecode_version = get_bytecode_version_from_env();
+    let bytecode_version = get_bytecode_version_from_env(bytecode_version);
 
     let bytecode = if is_bytecode_file(script_path) {
         assert!(

diff --git a/language/tools/move-cli/src/sandbox/utils/package_context.rs b/language/tools/move-cli/src/sandbox/utils/package_context.rs
@@ -34,8 +34,12 @@ impl PackageContext {
     /// NOTE: this is the only way to get a state view in Move CLI, and thus, this function needs
     /// to be run before every command that needs a state view, i.e., `publish`, `run`,
     /// `view`, and `doctor`.
-    pub fn prepare_state(&self, storage_dir: &Path) -> Result<OnDiskStateView> {
-        let bytecode_version = get_bytecode_version_from_env();
+    pub fn prepare_state(
+        &self,
+        bytecode_version: Option<u32>,
+        storage_dir: &Path,
+    ) -> Result<OnDiskStateView> {
+        let bytecode_version = get_bytecode_version_from_env(bytecode_version);
         let state = OnDiskStateView::create(self.build_dir.as_path(), storage_dir)?;
 
         // preload the storage with library modules (if such modules do not exist yet)

diff --git a/language/tools/move-package/src/compilation/build_plan.rs b/language/tools/move-package/src/compilation/build_plan.rs
@@ -103,13 +103,23 @@ impl BuildPlan {
     }
 
     /// Compilation results in the process exit upon warning/failure
-    pub fn compile<W: Write>(&self, writer: &mut W) -> Result<CompiledPackage> {
-        self.compile_with_driver(writer, |compiler| compiler.build_and_report())
+    pub fn compile<W: Write>(
+        &self,
+        bytecode_version: Option<u32>,
+        writer: &mut W,
+    ) -> Result<CompiledPackage> {
+        self.compile_with_driver(writer, bytecode_version, |compiler| {
+            compiler.build_and_report()
+        })
     }
 
     /// Compilation process does not exit even if warnings/failures are encountered
-    pub fn compile_no_exit<W: Write>(&self, writer: &mut W) -> Result<CompiledPackage> {
-        self.compile_with_driver(writer, |compiler| {
+    pub fn compile_no_exit<W: Write>(
+        &self,
+        bytecode_version: Option<u32>,
+        writer: &mut W,
+    ) -> Result<CompiledPackage> {
+        self.compile_with_driver(writer, bytecode_version, |compiler| {
             let (files, units_res) = compiler.build()?;
             match units_res {
                 Ok((units, warning_diags)) => {
@@ -131,6 +141,7 @@ impl BuildPlan {
     pub fn compile_with_driver<W: Write>(
         &self,
         writer: &mut W,
+        bytecode_version: Option<u32>,
         mut compiler_driver: impl FnMut(
             Compiler,
         )
@@ -176,6 +187,7 @@ impl BuildPlan {
             &project_root,
             root_package.clone(),
             transitive_dependencies,
+            bytecode_version,
             &self.resolution_graph,
             &mut compiler_driver,
         )?;

diff --git a/language/tools/move-package/src/compilation/compiled_package.rs b/language/tools/move-package/src/compilation/compiled_package.rs
@@ -324,6 +324,7 @@ impl OnDiskCompiledPackage {
         &self,
         package_name: Symbol,
         compiled_unit: &CompiledUnitWithSource,
+        bytecode_version: Option<u32>,
     ) -> Result<()> {
         let root_package = self.package.compiled_package_info.package_name;
         assert!(self.root_path.ends_with(root_package.as_str()));
@@ -349,7 +350,7 @@ impl OnDiskCompiledPackage {
                 .with_extension(MOVE_COMPILED_EXTENSION),
             compiled_unit
                 .unit
-                .serialize(get_bytecode_version_from_env())
+                .serialize(get_bytecode_version_from_env(bytecode_version))
                 .as_slice(),
         )?;
         self.save_under(
@@ -538,6 +539,7 @@ impl CompiledPackage {
             /* address mapping */ &ResolvedTable,
             /* whether source is available */ bool,
         )>,
+        bytecode_version: Option<u32>,
         resolution_graph: &ResolvedGraph,
         mut compiler_driver: impl FnMut(
             Compiler,
@@ -618,6 +620,7 @@ impl CompiledPackage {
                 deps_compiled_units.push((package_name, unit))
             }
         }
+        let bytecode_version = get_bytecode_version_from_env(bytecode_version);
 
         let mut compiled_docs = None;
         let mut compiled_abis = None;
@@ -642,7 +645,7 @@ impl CompiledPackage {
 
             if resolution_graph.build_options.generate_abis {
                 compiled_abis = Some(Self::build_abis(
-                    get_bytecode_version_from_env(),
+                    bytecode_version,
                     &model,
                     &root_compiled_units,
                 ));
@@ -662,7 +665,10 @@ impl CompiledPackage {
             compiled_abis,
         };
 
-        compiled_package.save_to_disk(project_root.join(CompiledPackageLayout::Root.path()))?;
+        compiled_package.save_to_disk(
+            project_root.join(CompiledPackageLayout::Root.path()),
+            bytecode_version,
+        )?;
 
         Ok(compiled_package)
     }
@@ -722,7 +728,11 @@ impl CompiledPackage {
         Ok(())
     }
 
-    pub(crate) fn save_to_disk(&self, under_path: PathBuf) -> Result<OnDiskCompiledPackage> {
+    pub(crate) fn save_to_disk(
+        &self,
+        under_path: PathBuf,
+        bytecode_version: Option<u32>,
+    ) -> Result<OnDiskCompiledPackage> {
         self.check_filepaths_ok()?;
         assert!(under_path.ends_with(CompiledPackageLayout::Root.path()));
         let root_package = self.compiled_package_info.package_name;
@@ -749,10 +759,10 @@ impl CompiledPackage {
         std::fs::create_dir_all(&on_disk_package.root_path)?;
 
         for compiled_unit in &self.root_compiled_units {
-            on_disk_package.save_compiled_unit(root_package, compiled_unit)?;
+            on_disk_package.save_compiled_unit(root_package, compiled_unit, bytecode_version)?;
         }
         for (dep_name, compiled_unit) in &self.deps_compiled_units {
-            on_disk_package.save_compiled_unit(*dep_name, compiled_unit)?;
+            on_disk_package.save_compiled_unit(*dep_name, compiled_unit, bytecode_version)?;
         }
 
         if let Some(docs) = &self.compiled_docs {

diff --git a/language/tools/move-package/src/lib.rs b/language/tools/move-package/src/lib.rs
@@ -134,6 +134,10 @@ pub struct BuildConfig {
     /// Skip fetching latest git dependencies
     #[clap(long = "skip-fetch-latest-git-deps", global = true)]
     pub skip_fetch_latest_git_deps: bool,
+
+    /// Bytecode version to compile move code
+    #[clap(long = "bytecode-version", global = true)]
+    pub bytecode_version: Option<u32>,
 }
 
 #[derive(Debug, Clone, Eq, PartialEq, PartialOrd)]
@@ -149,9 +153,10 @@ impl BuildConfig {
     /// Compile the package at `path` or the containing Move package. Exit process on warning or
     /// failure.
     pub fn compile_package<W: Write>(self, path: &Path, writer: &mut W) -> Result<CompiledPackage> {
+        let bytecode_version = self.bytecode_version;
         let resolved_graph = self.resolution_graph_for_package(path, writer)?;
         let mutx = PackageLock::lock();
-        let ret = BuildPlan::create(resolved_graph)?.compile(writer);
+        let ret = BuildPlan::create(resolved_graph)?.compile(bytecode_version, writer);
         mutx.unlock();
         ret
     }
@@ -163,9 +168,10 @@ impl BuildConfig {
         path: &Path,
         writer: &mut W,
     ) -> Result<CompiledPackage> {
+        let bytecode_version = self.bytecode_version;
         let resolved_graph = self.resolution_graph_for_package(path, writer)?;
         let mutx = PackageLock::lock();
-        let ret = BuildPlan::create(resolved_graph)?.compile_no_exit(writer);
+        let ret = BuildPlan::create(resolved_graph)?.compile_no_exit(bytecode_version, writer);
         mutx.unlock();
         ret
     }

diff --git a/language/tools/move-package/tests/test_runner.rs b/language/tools/move-package/tests/test_runner.rs
@@ -72,7 +72,7 @@ pub fn run_test(path: &Path) -> datatest_stable::Result<()> {
                 .into())
             }
             (true, _) => match BuildPlan::create(resolved_package)
-                .and_then(|bp| bp.compile(&mut Vec::new()))
+                .and_then(|bp| bp.compile(None, &mut Vec::new()))
             {
                 Ok(mut pkg) => {
                     pkg.compiled_package_info.source_digest =

diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps/Move.exp
@@ -17,5 +17,6 @@ CompiledPackageInfo {
         architecture: None,
         fetch_deps_only: false,
         skip_fetch_latest_git_deps: false,
+        bytecode_version: None,
     },
 }
diff --git a/...tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp b/...tools/move-package/tests/test_sources/compilation/basic_no_deps_address_assigned/Move.exp
@@ -19,5 +19,6 @@ CompiledPackageInfo {
         architecture: None,
         fetch_deps_only: false,
         skip_fetch_latest_git_deps: false,
+        bytecode_version: None,
     },
 }
diff --git a/.../test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp b/.../test_sources/compilation/basic_no_deps_address_not_assigned_with_dev_assignment/Move.exp
@@ -19,5 +19,6 @@ CompiledPackageInfo {
         architecture: None,
         fetch_deps_only: false,
         skip_fetch_latest_git_deps: false,
+        bytecode_version: None,
     },
 }
diff --git a/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp b/language/tools/move-package/tests/test_sources/compilation/basic_no_deps_test_mode/Move.exp
@@ -19,5 +19,6 @@ CompiledPackageInfo {
         architecture: None,
         fetch_deps_only: false,
         skip_fetch_latest_git_deps: false,
+        bytecode_version: None,
     },
 }
diff --git a/.../move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp b/.../move-package/tests/test_sources/compilation/diamond_problem_backflow_resolution/Move.exp
@@ -20,5 +20,6 @@ CompiledPackageInfo {
         architecture: None,
         fetch_deps_only: false,
         skip_fetch_latest_git_deps: false,
+        bytecode_version: None,
     },
 }
diff --git a/...ge/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp b/...ge/tools/move-package/tests/test_sources/compilation/diamond_problem_no_conflict/Move.exp
@@ -20,5 +20,6 @@ CompiledPackageInfo {
         architecture: None,
         fetch_deps_only: false,
         skip_fetch_latest_git_deps: false,
+        bytecode_version: None,
     },
 }
diff --git a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename/Move.exp
@@ -21,5 +21,6 @@ CompiledPackageInfo {
         architecture: None,
         fetch_deps_only: false,
         skip_fetch_latest_git_deps: false,
+        bytecode_version: None,
     },
 }
diff --git a/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp b/language/tools/move-package/tests/test_sources/compilation/multiple_deps_rename_one/Move.exp
@@ -21,5 +21,6 @@ CompiledPackageInfo {
         architecture: None,
         fetch_deps_only: false,
         skip_fetch_latest_git_deps: false,
+        bytecode_version: None,
     },
 }
diff --git a/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp b/language/tools/move-package/tests/test_sources/compilation/one_dep/Move.exp
@@ -19,5 +19,6 @@ CompiledPackageInfo {
         architecture: None,
         fetch_deps_only: false,
         skip_fetch_latest_git_deps: false,
+        bytecode_version: None,
     },
 }