Ensure scarb clean only works in a workspace (#672)

scarb/src/bin/scarb/commands/external.rs

@@ -3,10 +3,13 @@ use std::ffi::OsString;
 use anyhow::{anyhow, Result};
 
 use scarb::core::Config;
+use scarb::ops;
 use scarb::ops::execute_external_subcommand;
 
 #[tracing::instrument(skip_all, level = "info")]
 pub fn run(args: Vec<OsString>, config: &Config) -> Result<()> {
+    let ws = ops::read_workspace(config.manifest_path(), config)?;
+
     let Some((cmd, args)) = args.split_first() else {
         panic!("`args` should never be empty.")
     };
@@ -15,5 +18,5 @@ pub fn run(args: Vec<OsString>, config: &Config) -> Result<()> {
         .to_str()
         .ok_or_else(|| anyhow!("command name must be valid UTF-8"))?;
 
-    execute_external_subcommand(cmd, args, config, None)
+    execute_external_subcommand(cmd, args, None, &ws)
 }

scarb/src/compiler/compilation_unit.rs

@@ -5,7 +5,7 @@ use cairo_lang_filesystem::cfg::CfgSet;
 use smol_str::SmolStr;
 
 use crate::compiler::Profile;
-use crate::core::{Config, ManifestCompilerConfig, Package, PackageId, Target};
+use crate::core::{ManifestCompilerConfig, Package, PackageId, Target, Workspace};
 use crate::flock::Filesystem;
 use crate::internal::stable_hash::StableHasher;
 
@@ -77,8 +77,8 @@ impl CompilationUnit {
         &self.main_component().target
     }
 
-    pub fn target_dir<'c>(&self, config: &'c Config) -> Filesystem<'c> {
-        config.target_dir().child(self.profile.as_str())
+    pub fn target_dir<'c>(&self, ws: &'c Workspace<'_>) -> Filesystem<'c> {
+        ws.target_dir().child(self.profile.as_str())
     }
 
     pub fn is_sole_for_package(&self) -> bool {

scarb/src/compiler/compilers/lib.rs

@@ -50,7 +50,7 @@ impl Compiler for LibCompiler {
             );
         }
 
-        let target_dir = unit.target_dir(ws.config());
+        let target_dir = unit.target_dir(ws);
 
         let compiler_config = build_compiler_config(&unit, ws);
 

scarb/src/compiler/compilers/starknet_contract.rs

@@ -204,7 +204,7 @@ impl Compiler for StarknetContractCompiler {
             );
         }
 
-        let target_dir = unit.target_dir(ws.config());
+        let target_dir = unit.target_dir(ws);
 
         let compiler_config = build_compiler_config(&unit, ws);
 

scarb/src/core/config.rs

@@ -18,15 +18,14 @@ use crate::compiler::{CompilerRepository, Profile};
 use crate::core::AppDirs;
 #[cfg(doc)]
 use crate::core::Workspace;
-use crate::flock::{AdvisoryLock, RootFilesystem};
+use crate::flock::AdvisoryLock;
 use crate::internal::fsx;
-use crate::DEFAULT_TARGET_DIR_NAME;
 use crate::SCARB_ENV;
 
 pub struct Config {
     manifest_path: Utf8PathBuf,
     dirs: Arc<AppDirs>,
-    target_dir: RootFilesystem,
+    target_dir_override: Option<Utf8PathBuf>,
     app_exe: OnceCell<PathBuf>,
     ui: Ui,
     creation_time: Instant,
@@ -64,14 +63,6 @@ impl Config {
             }
         }
 
-        let target_dir =
-            RootFilesystem::new_output_dir(b.target_dir_override.unwrap_or_else(|| {
-                b.manifest_path
-                    .parent()
-                    .expect("parent of manifest path must always exist")
-                    .join(DEFAULT_TARGET_DIR_NAME)
-            }));
-
         let compilers = b.compilers.unwrap_or_else(CompilerRepository::std);
         let compiler_plugins = b.cairo_plugins.unwrap_or_else(CairoPluginRepository::std);
         let profile: Profile = b.profile.unwrap_or_default();
@@ -83,7 +74,7 @@ impl Config {
         Ok(Self {
             manifest_path: b.manifest_path,
             dirs,
-            target_dir,
+            target_dir_override: b.target_dir_override,
             app_exe: OnceCell::new(),
             ui,
             creation_time,
@@ -116,8 +107,8 @@ impl Config {
         &self.dirs
     }
 
-    pub fn target_dir(&self) -> &RootFilesystem {
-        &self.target_dir
+    pub fn target_dir_override(&self) -> Option<&Utf8PathBuf> {
+        self.target_dir_override.as_ref()
     }
 
     pub fn app_exe(&self) -> Result<&Path> {

scarb/src/core/workspace.rs

@@ -10,7 +10,7 @@ use crate::core::config::Config;
 use crate::core::package::Package;
 use crate::core::PackageId;
 use crate::flock::RootFilesystem;
-use crate::MANIFEST_FILE_NAME;
+use crate::{DEFAULT_TARGET_DIR_NAME, MANIFEST_FILE_NAME};
 
 /// The core abstraction for working with a workspace of packages.
 ///
@@ -19,9 +19,10 @@ use crate::MANIFEST_FILE_NAME;
 pub struct Workspace<'c> {
     config: &'c Config,
     members: BTreeMap<PackageId, Package>,
-    root_package: Option<PackageId>,
     manifest_path: Utf8PathBuf,
     profiles: Vec<Profile>,
+    root_package: Option<PackageId>,
+    target_dir: RootFilesystem,
 }
 
 impl<'c> Workspace<'c> {
@@ -36,11 +37,19 @@ impl<'c> Workspace<'c> {
             .iter()
             .map(|p| (p.id, p.clone()))
             .collect::<BTreeMap<_, _>>();
+        let target_dir = config.target_dir_override().cloned().unwrap_or_else(|| {
+            manifest_path
+                .parent()
+                .expect("parent of manifest path must always exist")
+                .join(DEFAULT_TARGET_DIR_NAME)
+        });
+        let target_dir = RootFilesystem::new_output_dir(target_dir);
         Ok(Self {
             config,
             manifest_path,
-            root_package,
             profiles,
+            root_package,
+            target_dir,
             members: packages,
         })
     }
@@ -52,6 +61,7 @@ impl<'c> Workspace<'c> {
     ) -> Result<Self> {
         let manifest_path = package.manifest_path().to_path_buf();
         let root_package = Some(package.id);
+
         Self::new(
             manifest_path,
             vec![package].as_ref(),
@@ -77,7 +87,7 @@ impl<'c> Workspace<'c> {
     }
 
     pub fn target_dir(&self) -> &RootFilesystem {
-        self.config.target_dir()
+        &self.target_dir
     }
 
     /// Returns the current package of this workspace.

scarb/src/ops/clean.rs

@@ -2,10 +2,12 @@ use anyhow::{Context, Result};
 
 use crate::core::Config;
 use crate::internal::fsx;
+use crate::ops;
 
 #[tracing::instrument(skip_all, level = "debug")]
 pub fn clean(config: &Config) -> Result<()> {
-    let path = config.target_dir().path_unchecked();
+    let ws = ops::read_workspace(config.manifest_path(), config)?;
+    let path = ws.target_dir().path_unchecked();
     if path.exists() {
         fsx::remove_dir_all(path).context("failed to clean generated artifacts")?;
     }

scarb/src/ops/scripts.rs

@@ -19,7 +19,7 @@ pub fn execute_script(
     cwd: &Utf8Path,
     custom_env: Option<HashMap<OsString, OsString>>,
 ) -> Result<()> {
-    let env_vars = get_env_vars(ws.config())?
+    let env_vars = get_env_vars(ws)?
         .into_iter()
         .map(|(k, v)| {
             (

scarb/src/ops/subcommands.rs

@@ -14,14 +14,14 @@ use crate::ops;
 use crate::process::{exec_replace, is_executable};
 use crate::subcommands::{get_env_vars, EXTERNAL_CMD_PREFIX, SCARB_MANIFEST_PATH_ENV};
 
-#[tracing::instrument(level = "debug", skip(config))]
+#[tracing::instrument(level = "debug", skip(ws))]
 pub fn execute_external_subcommand(
     cmd: &str,
     args: &[OsString],
-    config: &Config,
     custom_env: Option<HashMap<OsString, OsString>>,
+    ws: &Workspace<'_>,
 ) -> Result<()> {
-    let Some(cmd) = find_external_subcommand(cmd, config) else {
+    let Some(cmd) = find_external_subcommand(cmd, ws.config()) else {
         // TODO(mkaput): Reuse clap's no such command message logic here.
         bail!("no such command: `{cmd}`");
     };
@@ -32,7 +32,7 @@ pub fn execute_external_subcommand(
 
     let mut cmd = Command::new(cmd);
     cmd.args(args);
-    cmd.envs(get_env_vars(config)?);
+    cmd.envs(get_env_vars(ws)?);
     if let Some(env) = custom_env {
         cmd.envs(env);
     }
@@ -65,7 +65,7 @@ pub fn execute_test_subcommand(
             &format!("cairo-test {package_name}"),
         ));
         let args = args.iter().map(OsString::from).collect::<Vec<_>>();
-        execute_external_subcommand("cairo-test", args.as_ref(), ws.config(), env)
+        execute_external_subcommand("cairo-test", args.as_ref(), env, ws)
     }
 }
 

scarb/src/subcommands.rs

@@ -1,4 +1,4 @@
-use crate::core::Config;
+use crate::core::Workspace;
 use crate::SCARB_ENV;
 use std::collections::HashMap;
 use std::ffi::OsString;
@@ -7,7 +7,8 @@ pub const EXTERNAL_CMD_PREFIX: &str = "scarb-";
 pub const SCARB_MANIFEST_PATH_ENV: &str = "SCARB_MANIFEST_PATH";
 
 /// Defines env vars passed to external subcommands.
-pub fn get_env_vars(config: &Config) -> anyhow::Result<HashMap<OsString, OsString>> {
+pub fn get_env_vars(ws: &Workspace<'_>) -> anyhow::Result<HashMap<OsString, OsString>> {
+    let config = ws.config();
     Ok(HashMap::from_iter([
         ("PATH".into(), config.dirs().path_env()),
         (
@@ -25,7 +26,7 @@ pub fn get_env_vars(config: &Config) -> anyhow::Result<HashMap<OsString, OsStrin
         ),
         (
             "SCARB_TARGET_DIR".into(),
-            config.target_dir().path_unchecked().into(),
+            ws.target_dir().path_unchecked().into(),
         ),
         ("SCARB_PROFILE".into(), config.profile().as_str().into()),
         (

scarb/tests/clean.rs

@@ -1,11 +1,13 @@
 use assert_fs::prelude::*;
+use assert_fs::TempDir;
+use indoc::indoc;
 
 use scarb_test_support::command::Scarb;
 use scarb_test_support::project_builder::ProjectBuilder;
 
 #[test]
 fn simple() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     ProjectBuilder::start().build(&t);
 
     Scarb::quick_snapbox()
@@ -22,3 +24,26 @@ fn simple() {
         .success();
     t.child("target").assert(predicates::path::missing());
 }
+
+#[test]
+fn requires_workspace() {
+    let t = TempDir::new().unwrap();
+    t.child("target/dev/some.sierra.json")
+        .write_str("Lorem ipsum.")
+        .unwrap();
+
+    Scarb::quick_snapbox()
+        .arg("clean")
+        .current_dir(&t)
+        .assert()
+        .failure()
+        .stdout_matches(indoc! {r#"
+            error: failed to read manifest at: [..]/Scarb.toml
+
+            Caused by:
+                No such file or directory (os error 2)
+        "#});
+
+    t.child("target/dev/some.sierra.json")
+        .assert(predicates::path::is_file());
+}

scarb/tests/flock.rs

@@ -24,10 +24,12 @@ fn locking_build_artifacts() {
     let config = Scarb::test_config(manifest);
 
     thread::scope(|s| {
-        let lock = config
-            .target_dir()
-            .child(config.profile().to_string())
-            .open_rw("hello.sierra.json", "artifact", &config);
+        let ws = scarb::ops::read_workspace(config.manifest_path(), &config).unwrap();
+        let lock = ws.target_dir().child(config.profile().to_string()).open_rw(
+            "hello.sierra.json",
+            "artifact",
+            &config,
+        );
         let barrier = Arc::new(Barrier::new(2));
 
         s.spawn({

scarb/tests/subcommand.rs

@@ -8,17 +8,22 @@ use indoc::{formatdoc, indoc};
 
 use scarb_test_support::command::Scarb;
 use scarb_test_support::filesystem::{path_with_temp_dir, write_script, write_simple_hello_script};
+use scarb_test_support::project_builder::ProjectBuilder;
 
 #[test]
 #[cfg_attr(
     not(target_family = "unix"),
     ignore = "This test should write a Rust code, because currently it only assumes Unix."
 )]
 fn subcommand() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     write_simple_hello_script("hello", &t);
 
+    let p = TempDir::new().unwrap();
+    ProjectBuilder::start().build(&p);
+
     Scarb::quick_snapbox()
+        .current_dir(&p)
         .args(["hello", "beautiful", "world"])
         .env("PATH", path_with_temp_dir(&t))
         .assert()
@@ -76,8 +81,12 @@ fn env_variables_are_passed() {
         &t,
     );
 
+    let p = TempDir::new().unwrap();
+    ProjectBuilder::start().build(&p);
+
     Scarb::quick_snapbox()
-        .args(["env"])
+        .current_dir(&p)
+        .arg("env")
         .env("PATH", path_with_temp_dir(&t))
         .assert()
         .success();
@@ -107,7 +116,11 @@ fn env_scarb_log_is_passed_verbatim() {
         &t,
     );
 
+    let p = TempDir::new().unwrap();
+    ProjectBuilder::start().build(&p);
+
     Scarb::quick_snapbox()
+        .current_dir(&p)
         .args(["-vvvv", "env"])
         .env("PATH", path_with_temp_dir(&t))
         .env("SCARB_LOG", "test=filter")
@@ -179,7 +192,7 @@ fn ctrl_c(child: &mut Child) {
     ignore = "This test should write a Rust code, because currently it only assumes Unix."
 )]
 fn can_find_scarb_directory_scripts_without_path() {
-    let t = assert_fs::TempDir::new().unwrap();
+    let t = TempDir::new().unwrap();
     write_simple_hello_script("hello", &t);
 
     // Set scarb path to folder containing hello script
@@ -189,7 +202,12 @@ fn can_find_scarb_directory_scripts_without_path() {
         .join("scarb-hello")
         .to_string_lossy()
         .to_string();
+
+    let p = TempDir::new().unwrap();
+    ProjectBuilder::start().build(&p);
+
     Scarb::quick_snapbox()
+        .current_dir(&p)
         .env("SCARB", scarb_path)
         .args(["hello", "beautiful", "world"])
         .assert()