[build] Added additional diagnostics for fetching deps (move-language#643)

awelc · web-flow · commit e2e1a41f7da0 · 2022-11-04T10:13:43.000-07:00
* [build] Added additional diagnostics for fetching deps

* Fixes to EVM code and tests

* Addressed review comments
diff --git a/language/move-analyzer/src/symbols.rs b/language/move-analyzer/src/symbols.rs
@@ -624,7 +624,10 @@ impl Symbolicator {
 
         eprintln!("symbolicating {:?}", pkg_path);
 
-        let resolution_graph = build_config.resolution_graph_for_package(pkg_path)?;
+        // resolution graph diagnostics are only needed for CLI commands so ignore them by passing a
+        // vector as the writer
+        let resolution_graph =
+            build_config.resolution_graph_for_package(pkg_path, &mut Vec::new())?;
 
         // get source files to be able to correlate positions (in terms of byte offsets) with actual
         // file locations (in terms of line/column numbers)
diff --git a/language/tools/move-cli/src/base/build.rs b/language/tools/move-cli/src/base/build.rs
@@ -19,14 +19,14 @@ impl Build {
             if config.test_mode {
                 config.dev_mode = true;
             }
-            config.download_deps_for_package(&rerooted_path)?;
+            config.download_deps_for_package(&rerooted_path, &mut std::io::stdout())?;
             return Ok(());
         }
         let architecture = config.architecture.unwrap_or(Architecture::Move);
 
         match architecture {
             Architecture::Move | Architecture::AsyncMove => {
-                config.compile_package(&rerooted_path, &mut std::io::stderr())?;
+                config.compile_package(&rerooted_path, &mut std::io::stdout())?;
             }
 
             Architecture::Ethereum => {
diff --git a/language/tools/move-cli/src/base/info.rs b/language/tools/move-cli/src/base/info.rs
@@ -15,7 +15,7 @@ impl Info {
     pub fn execute(self, path: Option<PathBuf>, config: BuildConfig) -> anyhow::Result<()> {
         let rerooted_path = reroot_path(path)?;
         config
-            .resolution_graph_for_package(&rerooted_path)?
+            .resolution_graph_for_package(&rerooted_path, &mut std::io::stdout())?
             .print_info()
     }
 }
diff --git a/language/tools/move-cli/src/base/test.rs b/language/tools/move-cli/src/base/test.rs
@@ -160,8 +160,9 @@ pub fn run_move_unit_tests<W: Write + Send>(
     build_config.test_mode = true;
     build_config.dev_mode = true;
 
-    // Build the resolution graph
-    let resolution_graph = build_config.resolution_graph_for_package(pkg_path)?;
+    // Build the resolution graph (resolution graph diagnostics are only needed for CLI commands so
+    // ignore them by passing a vector as the writer)
+    let resolution_graph = build_config.resolution_graph_for_package(pkg_path, &mut Vec::new())?;
 
     // Note: unit_test_config.named_address_values is always set to vec![] (the default value) before
     // being passed in.
diff --git a/language/tools/move-cli/src/sandbox/commands/test.rs b/language/tools/move-cli/src/sandbox/commands/test.rs
@@ -126,12 +126,13 @@ fn pad_tmp_path(tmp_dir: &Path, pad_amount: usize) -> anyhow::Result<PathBuf> {
 // package manifest.
 fn copy_deps(tmp_dir: &Path, pkg_dir: &Path) -> anyhow::Result<PathBuf> {
     // Sometimes we run a test that isn't a package for metatests so if there isn't a package we
-    // don't need to nest at all.
+    // don't need to nest at all. Resolution graph diagnostics are only needed for CLI commands so
+    // ignore them by passing a vector as the writer.
     let package_resolution = match (BuildConfig {
         dev_mode: true,
         ..Default::default()
     })
-    .resolution_graph_for_package(pkg_dir)
+    .resolution_graph_for_package(pkg_dir, &mut Vec::new())
     {
         Ok(pkg) => pkg,
         Err(_) => return Ok(tmp_dir.to_path_buf()),
diff --git a/language/tools/move-cli/tests/build_tests/simple_build_with_docs/args.exp b/language/tools/move-cli/tests/build_tests/simple_build_with_docs/args.exp
@@ -1,5 +1,6 @@
 Command `new --path . Foo`:
 Command `build`:
+FETCHING GIT DEPENDENCY https://github.com/move-language/move.git
 INCLUDING DEPENDENCY MoveStdlib
 BUILDING Foo
 Command `docgen --template template.md --exclude-impl --exclude-private-fun --exclude-specs --include-call-diagrams --include-dep-diagrams --independent-specs --no-collapsed-sections --output-directory doc --references-file template.md --section-level-start 3 --toc-depth 3`:
diff --git a/language/tools/move-package/src/lib.rs b/language/tools/move-package/src/lib.rs
@@ -149,7 +149,7 @@ 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 resolved_graph = self.resolution_graph_for_package(path)?;
+        let resolved_graph = self.resolution_graph_for_package(path, writer)?;
         let mutx = PackageLock::lock();
         let ret = BuildPlan::create(resolved_graph)?.compile(writer);
         mutx.unlock();
@@ -163,7 +163,7 @@ impl BuildConfig {
         path: &Path,
         writer: &mut W,
     ) -> Result<CompiledPackage> {
-        let resolved_graph = self.resolution_graph_for_package(path)?;
+        let resolved_graph = self.resolution_graph_for_package(path, writer)?;
         let mutx = PackageLock::lock();
         let ret = BuildPlan::create(resolved_graph)?.compile_no_exit(writer);
         mutx.unlock();
@@ -172,7 +172,9 @@ impl BuildConfig {
 
     #[cfg(feature = "evm-backend")]
     pub fn compile_package_evm<W: Write>(self, path: &Path, writer: &mut W) -> Result<()> {
-        let resolved_graph = self.resolution_graph_for_package(path)?;
+        // resolution graph diagnostics are only needed for CLI commands so ignore them by passing a
+        // vector as the writer
+        let resolved_graph = self.resolution_graph_for_package(path, &mut Vec::new())?;
         let mutx = PackageLock::lock();
         let ret = BuildPlan::create(resolved_graph)?.compile_evm(writer);
         mutx.unlock();
@@ -189,27 +191,33 @@ impl BuildConfig {
         path: &Path,
         model_config: ModelConfig,
     ) -> Result<GlobalEnv> {
-        let resolved_graph = self.resolution_graph_for_package(path)?;
+        // resolution graph diagnostics are only needed for CLI commands so ignore them by passing a
+        // vector as the writer
+        let resolved_graph = self.resolution_graph_for_package(path, &mut Vec::new())?;
         let mutx = PackageLock::lock();
         let ret = ModelBuilder::create(resolved_graph, model_config).build_model();
         mutx.unlock();
         ret
     }
 
-    pub fn download_deps_for_package(&self, path: &Path) -> Result<()> {
+    pub fn download_deps_for_package<W: Write>(&self, path: &Path, writer: &mut W) -> Result<()> {
         let path = SourcePackageLayout::try_find_root(path)?;
         let toml_manifest =
             self.parse_toml_manifest(path.join(SourcePackageLayout::Manifest.path()))?;
         let mutx = PackageLock::lock();
         // This should be locked as it inspects the environment for `MOVE_HOME` which could
         // possibly be set by a different process in parallel.
         let manifest = manifest_parser::parse_source_manifest(toml_manifest)?;
-        ResolutionGraph::download_dependency_repos(&manifest, self, &path)?;
+        ResolutionGraph::download_dependency_repos(&manifest, self, &path, writer)?;
         mutx.unlock();
         Ok(())
     }
 
-    pub fn resolution_graph_for_package(mut self, path: &Path) -> Result<ResolvedGraph> {
+    pub fn resolution_graph_for_package<W: Write>(
+        mut self,
+        path: &Path,
+        writer: &mut W,
+    ) -> Result<ResolvedGraph> {
         if self.test_mode {
             self.dev_mode = true;
         }
@@ -220,7 +228,7 @@ impl BuildConfig {
         // This should be locked as it inspects the environment for `MOVE_HOME` which could
         // possibly be set by a different process in parallel.
         let manifest = manifest_parser::parse_source_manifest(toml_manifest)?;
-        let resolution_graph = ResolutionGraph::new(manifest, path, self)?;
+        let resolution_graph = ResolutionGraph::new(manifest, path, self, writer)?;
         let ret = resolution_graph.resolve();
         mutx.unlock();
         ret
diff --git a/language/tools/move-package/src/resolution/resolution_graph.rs b/language/tools/move-package/src/resolution/resolution_graph.rs
@@ -16,6 +16,7 @@ use crate::{
     BuildConfig,
 };
 use anyhow::{bail, Context, Result};
+use colored::Colorize;
 use move_command_line_common::files::{find_move_filenames, FileHash};
 use move_core_types::account_address::AccountAddress;
 use move_symbol_pool::Symbol;
@@ -25,6 +26,7 @@ use std::{
     cell::RefCell,
     collections::{BTreeMap, BTreeSet},
     fs,
+    io::Write,
     path::{Path, PathBuf},
     process::{Command, Stdio},
     rc::Rc,
@@ -90,10 +92,11 @@ pub struct ResolutionPackage<T> {
 }
 
 impl ResolvingGraph {
-    pub fn new(
+    pub fn new<W: Write>(
         root_package: SourceManifest,
         root_package_path: PathBuf,
         mut build_options: BuildConfig,
+        writer: &mut W,
     ) -> Result<ResolvingGraph> {
         if build_options.architecture.is_none() {
             if let Some(info) = &root_package.build {
@@ -109,7 +112,7 @@ impl ResolvingGraph {
         };
 
         resolution_graph
-            .build_resolution_graph(root_package.clone(), root_package_path, true)
+            .build_resolution_graph(root_package.clone(), root_package_path, true, writer)
             .with_context(|| {
                 format!(
                     "Unable to resolve packages for package '{}'",
@@ -189,11 +192,12 @@ impl ResolvingGraph {
         })
     }
 
-    fn build_resolution_graph(
+    fn build_resolution_graph<W: Write>(
         &mut self,
         package: SourceManifest,
         package_path: PathBuf,
         is_root_package: bool,
+        writer: &mut W,
     ) -> Result<()> {
         let package_name = package.package.name;
         let package_node_id = match self.package_table.get(&package_name) {
@@ -246,7 +250,7 @@ impl ResolvingGraph {
             self.graph.add_edge(package_node_id, dep_node_id, ());
 
             let (dep_renaming, dep_resolution_table) = self
-                .process_dependency(dep_name, dep, package_path.clone())
+                .process_dependency(dep_name, dep, package_path.clone(), writer)
                 .with_context(|| {
                     format!(
                         "While resolving dependency '{}' in package '{}'",
@@ -382,21 +386,23 @@ impl ResolvingGraph {
     // Process a dependency. `dep_name_in_pkg` is the name assigned to the dependent package `dep`
     // in the source manifest, and we check that this name matches the name of the dependency it is
     // assigned to.
-    fn process_dependency(
+    fn process_dependency<W: Write>(
         &mut self,
         dep_name_in_pkg: PackageName,
         dep: Dependency,
         root_path: PathBuf,
+        writer: &mut W,
     ) -> Result<(Renaming, ResolvingTable)> {
         Self::download_and_update_if_remote(
             dep_name_in_pkg,
             &dep,
             self.build_options.skip_fetch_latest_git_deps,
+            writer,
         )?;
         let (dep_package, dep_package_dir) =
             Self::parse_package_manifest(&dep, &dep_name_in_pkg, root_path)
                 .with_context(|| format!("While processing dependency '{}'", dep_name_in_pkg))?;
-        self.build_resolution_graph(dep_package.clone(), dep_package_dir, false)
+        self.build_resolution_graph(dep_package.clone(), dep_package_dir, false, writer)
             .with_context(|| {
                 format!("Unable to resolve package dependency '{}'", dep_name_in_pkg)
             })?;
@@ -515,10 +521,11 @@ impl ResolvingGraph {
         }
     }
 
-    pub fn download_dependency_repos(
+    pub fn download_dependency_repos<W: Write>(
         manifest: &SourceManifest,
         build_options: &BuildConfig,
         root_path: &Path,
+        writer: &mut W,
     ) -> Result<()> {
         // include dev dependencies if in dev mode
         let empty_deps;
@@ -534,24 +541,33 @@ impl ResolvingGraph {
                 *dep_name,
                 dep,
                 build_options.skip_fetch_latest_git_deps,
+                writer,
             )?;
 
             let (dep_manifest, _) =
                 Self::parse_package_manifest(dep, dep_name, root_path.to_path_buf())
                     .with_context(|| format!("While processing dependency '{}'", *dep_name))?;
             // download dependencies of dependencies
-            Self::download_dependency_repos(&dep_manifest, build_options, root_path)?;
+            Self::download_dependency_repos(&dep_manifest, build_options, root_path, writer)?;
         }
         Ok(())
     }
 
-    fn download_and_update_if_remote(
+    fn download_and_update_if_remote<W: Write>(
         dep_name: PackageName,
         dep: &Dependency,
         skip_fetch_latest_git_deps: bool,
+        writer: &mut W,
     ) -> Result<()> {
         if let Some(git_info) = &dep.git_info {
             if !git_info.download_to.exists() {
+                writeln!(
+                    writer,
+                    "{} {}",
+                    "FETCHING GIT DEPENDENCY".bold().green(),
+                    git_info.git_url
+                )?;
+
                 // If the cached folder does not exist, download and clone accordingly
                 Command::new("git")
                     .args([
diff --git a/language/tools/move-package/tests/package_hash_skips_non_move_files.rs b/language/tools/move-package/tests/package_hash_skips_non_move_files.rs
@@ -10,11 +10,14 @@ use tempfile::tempdir;
 fn package_hash_skips_non_move_files() {
     let path = Path::new("tests/test_sources/resolution/dep_good_digest");
 
+    // resolution graph diagnostics are only needed for CLI commands so ignore them in both cases by
+    // passing a vector as the writer
+
     let pkg1 = BuildConfig {
         install_dir: Some(tempdir().unwrap().path().to_path_buf()),
         ..Default::default()
     }
-    .resolution_graph_for_package(path)
+    .resolution_graph_for_package(path, &mut Vec::new())
     .unwrap();
 
     let dummy_path = path.join("deps_only/other_dep/sources/dummy_text");
@@ -27,7 +30,7 @@ fn package_hash_skips_non_move_files() {
         install_dir: Some(tempdir().unwrap().path().to_path_buf()),
         ..Default::default()
     }
-    .resolution_graph_for_package(path)
+    .resolution_graph_for_package(path, &mut Vec::new())
     .unwrap();
 
     std::fs::remove_file(&dummy_path).unwrap();
diff --git a/language/tools/move-package/tests/test_additional_addresses.rs b/language/tools/move-package/tests/test_additional_addresses.rs
@@ -30,6 +30,7 @@ fn test_additonal_addresses() {
             additional_named_addresses,
             ..Default::default()
         },
+        &mut Vec::new() /* empty writer as no diags needed */
     )
     .unwrap()
     .resolve()
@@ -42,6 +43,7 @@ fn test_additonal_addresses() {
             install_dir: Some(tempdir().unwrap().path().to_path_buf()),
             ..Default::default()
         },
+        &mut Vec::new() /* empty writer as no diags needed */
     )
     .unwrap()
     .resolve()
@@ -67,6 +69,7 @@ fn test_additonal_addresses_already_assigned_same_value() {
             additional_named_addresses,
             ..Default::default()
         },
+        &mut Vec::new() /* empty writer as no diags needed */
     )
     .unwrap()
     .resolve()
@@ -92,6 +95,7 @@ fn test_additonal_addresses_already_assigned_different_value() {
             additional_named_addresses,
             ..Default::default()
         },
+        &mut Vec::new() /* empty writer as no diags needed */
     )
     .is_err());
 }
diff --git a/language/tools/move-package/tests/test_runner.rs b/language/tools/move-package/tests/test_runner.rs
@@ -59,6 +59,7 @@ pub fn run_test(path: &Path) -> datatest_stable::Result<()> {
                     force_recompilation: false,
                     ..Default::default()
                 },
+                &mut Vec::new(), /* empty writer as no diags needed */
             )
         })
         .and_then(|rg| rg.resolve())

Original file line number	Diff line number	Diff line change
`@@ -19,14 +19,14 @@ impl Build {`
`19`	`19`	`if config.test_mode {`
`20`	`20`	`config.dev_mode = true;`
`21`	`21`	`}`
`22`		`- config.download_deps_for_package(&rerooted_path)?;`
	`22`	`+ config.download_deps_for_package(&rerooted_path, &mut std::io::stdout())?;`
`23`	`23`	`return Ok(());`
`24`	`24`	`}`
`25`	`25`	`let architecture = config.architecture.unwrap_or(Architecture::Move);`
`26`	`26`
`27`	`27`	`match architecture {`
`28`	`28`	`Architecture::Move \| Architecture::AsyncMove => {`
`29`		`- config.compile_package(&rerooted_path, &mut std::io::stderr())?;`
	`29`	`+ config.compile_package(&rerooted_path, &mut std::io::stdout())?;`
`30`	`30`	`}`
`31`	`31`
`32`	`32`	`Architecture::Ethereum => {`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ impl Info {`
`15`	`15`	`pub fn execute(self, path: Option<PathBuf>, config: BuildConfig) -> anyhow::Result<()> {`
`16`	`16`	`let rerooted_path = reroot_path(path)?;`
`17`	`17`	`config`
`18`		`- .resolution_graph_for_package(&rerooted_path)?`
	`18`	`+ .resolution_graph_for_package(&rerooted_path, &mut std::io::stdout())?`
`19`	`19`	`.print_info()`
`20`	`20`	`}`
`21`	`21`	`}`
Original file line number	Diff line number	Diff line change
`@@ -30,6 +30,7 @@ fn test_additonal_addresses() {`
`30`	`30`	`additional_named_addresses,`
`31`	`31`	`..Default::default()`
`32`	`32`	`},`
	`33`	`+ &mut Vec::new() /* empty writer as no diags needed */`
`33`	`34`	`)`
`34`	`35`	`.unwrap()`
`35`	`36`	`.resolve()`
`@@ -42,6 +43,7 @@ fn test_additonal_addresses() {`
`42`	`43`	`install_dir: Some(tempdir().unwrap().path().to_path_buf()),`
`43`	`44`	`..Default::default()`
`44`	`45`	`},`
	`46`	`+ &mut Vec::new() /* empty writer as no diags needed */`
`45`	`47`	`)`
`46`	`48`	`.unwrap()`
`47`	`49`	`.resolve()`
`@@ -67,6 +69,7 @@ fn test_additonal_addresses_already_assigned_same_value() {`
`67`	`69`	`additional_named_addresses,`
`68`	`70`	`..Default::default()`
`69`	`71`	`},`
	`72`	`+ &mut Vec::new() /* empty writer as no diags needed */`
`70`	`73`	`)`
`71`	`74`	`.unwrap()`
`72`	`75`	`.resolve()`
`@@ -92,6 +95,7 @@ fn test_additonal_addresses_already_assigned_different_value() {`
`92`	`95`	`additional_named_addresses,`
`93`	`96`	`..Default::default()`
`94`	`97`	`},`
	`98`	`+ &mut Vec::new() /* empty writer as no diags needed */`
`95`	`99`	`)`
`96`	`100`	`.is_err());`
`97`	`101`	`}`
Original file line number	Diff line number	Diff line change
`@@ -59,6 +59,7 @@ pub fn run_test(path: &Path) -> datatest_stable::Result<()> {`
`59`	`59`	`force_recompilation: false,`
`60`	`60`	`..Default::default()`
`61`	`61`	`},`
	`62`	`+ &mut Vec::new(), /* empty writer as no diags needed */`
`62`	`63`	`)`
`63`	`64`	`})`
`64`	`65`	`.and_then(\|rg\| rg.resolve())`