From dda53efd05af21aac6b430196cc20b789e078b16 Mon Sep 17 00:00:00 2001 From: maciektr Date: Tue, 17 Sep 2024 04:34:18 -0700 Subject: [PATCH] Fix single package features in a workspace (#1593) fix #1516 #1599 --- extensions/scarb-doc/tests/features.rs | 70 +++------------- scarb/src/bin/scarb/commands/package.rs | 12 ++- scarb/src/bin/scarb/commands/publish.rs | 6 +- scarb/src/bin/scarb/commands/test.rs | 21 +++-- scarb/src/ops/compile.rs | 8 +- scarb/src/ops/expand.rs | 4 +- scarb/src/ops/resolve.rs | 46 +++++++---- scarb/src/ops/subcommands.rs | 4 +- scarb/tests/features.rs | 104 +++++++++++++++++++++++- 9 files changed, 176 insertions(+), 99 deletions(-) diff --git a/extensions/scarb-doc/tests/features.rs b/extensions/scarb-doc/tests/features.rs index fddd4066f..8ed300e6d 100644 --- a/extensions/scarb-doc/tests/features.rs +++ b/extensions/scarb-doc/tests/features.rs @@ -112,7 +112,7 @@ fn test_workspace_without_features_in_manifest() { error: metadata command failed: `scarb metadata` exited with error stdout: - error: no features in manifest + error: none of the selected packages contains `test_feature` feature note: to use features, you need to define [features] section in Scarb.toml stderr: @@ -125,7 +125,7 @@ fn test_workspace_without_features_in_manifest() { error: metadata command failed: `scarb metadata` exited with error stdout: - error: no features in manifest + error: none of the selected packages contains `test_feature` feature note: to use features, you need to define [features] section in Scarb.toml stderr: @@ -225,7 +225,7 @@ fn test_workspace_with_working_feature_in_root_only() { .lib_cairo(COMMON_CODE_WITH_FEATURE) .build(&child_dir); - let snapbox = Scarb::quick_snapbox() + Scarb::quick_snapbox() .env("RUST_BACKTRACE", "0") .arg("doc") .args([ @@ -237,32 +237,7 @@ fn test_workspace_with_working_feature_in_root_only() { ]) .current_dir(&root_dir) .assert() - .failure(); - - #[cfg(windows)] - snapbox.stdout_matches(indoc! {r#" - error: metadata command failed: `scarb metadata` exited with error - - stdout: - error: no features in manifest - note: to use features, you need to define [features] section in Scarb.toml - - stderr: - - error: process did not exit successfully: exit code: 1 - "#}); - - #[cfg(not(windows))] - snapbox.stdout_matches(indoc! {r#" - error: metadata command failed: `scarb metadata` exited with error - - stdout: - error: no features in manifest - note: to use features, you need to define [features] section in Scarb.toml - - stderr: - - "#}); + .success(); } #[test] @@ -290,7 +265,7 @@ fn test_workspace_with_working_feature_in_sub_package_only() { .lib_cairo(COMMON_CODE_WITH_FEATURE) .build(&child_dir); - let snapbox = Scarb::quick_snapbox() + Scarb::quick_snapbox() .env("RUST_BACKTRACE", "0") .arg("doc") .args([ @@ -302,32 +277,7 @@ fn test_workspace_with_working_feature_in_sub_package_only() { ]) .current_dir(&root_dir) .assert() - .failure(); - - #[cfg(windows)] - snapbox.stdout_matches(indoc! {r#" - error: metadata command failed: `scarb metadata` exited with error - - stdout: - error: no features in manifest - note: to use features, you need to define [features] section in Scarb.toml - - stderr: - - error: process did not exit successfully: exit code: 1 - "#}); - - #[cfg(not(windows))] - snapbox.stdout_matches(indoc! {r#" - error: metadata command failed: `scarb metadata` exited with error - - stdout: - error: no features in manifest - note: to use features, you need to define [features] section in Scarb.toml - - stderr: - - "#}); + .success(); } #[test] @@ -367,7 +317,7 @@ fn test_workspace_without_features_in_manifest_and_present_in_sub_package_code() error: metadata command failed: `scarb metadata` exited with error stdout: - error: no features in manifest + error: none of the selected packages contains `test_feature` feature note: to use features, you need to define [features] section in Scarb.toml stderr: @@ -380,7 +330,7 @@ fn test_workspace_without_features_in_manifest_and_present_in_sub_package_code() error: metadata command failed: `scarb metadata` exited with error stdout: - error: no features in manifest + error: none of the selected packages contains `test_feature` feature note: to use features, you need to define [features] section in Scarb.toml stderr: @@ -426,7 +376,7 @@ fn test_workspace_without_features_in_manifest_and_present_in_root_package_code( error: metadata command failed: `scarb metadata` exited with error stdout: - error: no features in manifest + error: none of the selected packages contains `test_feature` feature note: to use features, you need to define [features] section in Scarb.toml stderr: @@ -439,7 +389,7 @@ fn test_workspace_without_features_in_manifest_and_present_in_root_package_code( error: metadata command failed: `scarb metadata` exited with error stdout: - error: no features in manifest + error: none of the selected packages contains `test_feature` feature note: to use features, you need to define [features] section in Scarb.toml stderr: diff --git a/scarb/src/bin/scarb/commands/package.rs b/scarb/src/bin/scarb/commands/package.rs index e15756bed..1b654b9ea 100644 --- a/scarb/src/bin/scarb/commands/package.rs +++ b/scarb/src/bin/scarb/commands/package.rs @@ -2,11 +2,12 @@ use std::collections::BTreeMap; use anyhow::Result; use camino::Utf8PathBuf; +use itertools::Itertools; use serde::Serializer; use scarb::core::{Config, PackageName}; use scarb::ops; -use scarb::ops::PackageOpts; +use scarb::ops::{validate_features, PackageOpts}; use scarb_ui::Message; use crate::args::PackageArgs; @@ -18,17 +19,20 @@ pub fn run(args: PackageArgs, config: &Config) -> Result<()> { .packages_filter .match_many(&ws)? .into_iter() - .map(|p| p.id) - .collect::>(); + .collect_vec(); + let features_opts = args.features.try_into()?; + validate_features(&packages, &features_opts)?; let opts = PackageOpts { // Disable dirty repository checks when printing package files. allow_dirty: args.list || args.shared_args.allow_dirty, verify: !args.shared_args.no_verify, check_metadata: !args.no_metadata, - features: args.features.try_into()?, + features: features_opts, }; + let packages = packages.into_iter().map(|p| p.id).collect_vec(); + if args.list { let result = ops::package_list(&packages, &opts, &ws)?; ws.config().ui().print(ListMessage(result)); diff --git a/scarb/src/bin/scarb/commands/publish.rs b/scarb/src/bin/scarb/commands/publish.rs index 28c9c7120..dabc49125 100644 --- a/scarb/src/bin/scarb/commands/publish.rs +++ b/scarb/src/bin/scarb/commands/publish.rs @@ -4,7 +4,7 @@ use url::Url; use scarb::core::registry::DEFAULT_REGISTRY_INDEX; use scarb::core::Config; -use scarb::ops::{self, PackageOpts, PublishOpts}; +use scarb::ops::{self, validate_features, PackageOpts, PublishOpts}; use crate::args::PublishArgs; @@ -17,13 +17,15 @@ pub fn run(args: PublishArgs, config: &Config) -> Result<()> { None => Url::from_str(DEFAULT_REGISTRY_INDEX)?, }; + let features_opts = args.features.try_into()?; + validate_features(&[package.clone()], &features_opts)?; let ops = PublishOpts { index_url: index, package_opts: PackageOpts { allow_dirty: args.shared_args.allow_dirty, verify: !args.shared_args.no_verify, check_metadata: true, - features: args.features.try_into()?, + features: features_opts, }, }; diff --git a/scarb/src/bin/scarb/commands/test.rs b/scarb/src/bin/scarb/commands/test.rs index d23dd2c89..091a3e5c1 100644 --- a/scarb/src/bin/scarb/commands/test.rs +++ b/scarb/src/bin/scarb/commands/test.rs @@ -1,18 +1,21 @@ +use crate::args::TestArgs; use anyhow::Result; - +use itertools::Itertools; use scarb::core::Config; use scarb::ops; - -use crate::args::TestArgs; +use scarb::ops::{validate_features, FeaturesOpts}; #[tracing::instrument(skip_all, level = "info")] pub fn run(args: TestArgs, config: &Config) -> Result<()> { let ws = ops::read_workspace(config.manifest_path(), config)?; - args.packages_filter + let packages = args + .packages_filter .match_many(&ws)? - .iter() - .try_for_each(|package| { - ops::execute_test_subcommand(package, &args.args, &ws, args.features.clone()) - .map(|_| ()) - }) + .into_iter() + .collect_vec(); + let features_opts: FeaturesOpts = args.features.clone().try_into()?; + validate_features(&packages, &features_opts)?; + packages.iter().try_for_each(|package| { + ops::execute_test_subcommand(package, &args.args, &ws, args.features.clone()).map(|_| ()) + }) } diff --git a/scarb/src/ops/compile.rs b/scarb/src/ops/compile.rs index c5c1e10f7..3ea5a0755 100644 --- a/scarb/src/ops/compile.rs +++ b/scarb/src/ops/compile.rs @@ -19,7 +19,7 @@ use crate::core::{ FeatureName, PackageId, PackageName, TargetKind, Utf8PathWorkspaceExt, Workspace, }; use crate::ops; -use crate::ops::get_test_package_ids; +use crate::ops::{get_test_package_ids, validate_features}; #[derive(Debug, Clone)] pub enum FeaturesSelector { @@ -115,7 +115,11 @@ where F: FnMut(CompilationUnit, &Workspace<'_>) -> Result<()>, { let resolve = ops::resolve_workspace(ws)?; - + let packages_to_process = ws + .members() + .filter(|p| packages.contains(&p.id)) + .collect_vec(); + validate_features(&packages_to_process, &opts.features)?; // Add test compilation units to build let packages = get_test_package_ids(packages, ws); let compilation_units = ops::generate_compilation_units(&resolve, &opts.features, ws)? diff --git a/scarb/src/ops/expand.rs b/scarb/src/ops/expand.rs index 4af261ac7..76861250a 100644 --- a/scarb/src/ops/expand.rs +++ b/scarb/src/ops/expand.rs @@ -3,7 +3,7 @@ use crate::compiler::helpers::{build_compiler_config, write_string}; use crate::compiler::{CairoCompilationUnit, CompilationUnit, CompilationUnitAttributes}; use crate::core::{Package, PackageId, TargetKind, Workspace}; use crate::ops; -use crate::ops::{get_test_package_ids, FeaturesOpts}; +use crate::ops::{get_test_package_ids, validate_features, FeaturesOpts}; use anyhow::{bail, Context, Result}; use cairo_lang_defs::db::DefsGroup; use cairo_lang_defs::ids::{LanguageElementId, ModuleId, ModuleItemId}; @@ -38,6 +38,8 @@ pub struct ExpandOpts { } pub fn expand(package: Package, opts: ExpandOpts, ws: &Workspace<'_>) -> Result<()> { + validate_features(&[package.clone()], &opts.features)?; + let package_name = package.id.name.to_string(); let resolve = ops::resolve_workspace(ws)?; let compilation_units = ops::generate_compilation_units(&resolve, &opts.features, ws)?; diff --git a/scarb/src/ops/resolve.rs b/scarb/src/ops/resolve.rs index bbe95e806..9865cd6c6 100644 --- a/scarb/src/ops/resolve.rs +++ b/scarb/src/ops/resolve.rs @@ -173,7 +173,12 @@ pub fn generate_compilation_units( ws: &Workspace<'_>, ) -> Result> { let mut units = Vec::with_capacity(ws.members().size_hint().0); - for member in ws.members().filter(|member| !member.is_cairo_plugin()) { + let members = ws + .members() + .filter(|member| !member.is_cairo_plugin()) + .collect_vec(); + validate_features(&members, enabled_features)?; + for member in members { units.extend(generate_cairo_compilation_units( &member, resolve, @@ -207,6 +212,25 @@ pub fn generate_compilation_units( Ok(units) } +pub fn validate_features(members: &[Package], enabled_features: &FeaturesOpts) -> Result<()> { + // Check if any member has features defined. + if let FeaturesSelector::Features(features) = &enabled_features.features { + for feature in features { + if !members + .iter() + .any(|member| member.manifest.features.contains_key(feature)) + { + bail!( + "none of the selected packages contains `{}` feature\n\ + note: to use features, you need to define [features] section in Scarb.toml", + feature + ); + } + } + } + Ok(()) +} + fn generate_cairo_compilation_units( member: &Package, resolve: &WorkspaceResolve, @@ -377,25 +401,15 @@ fn get_cfg_with_features( features_manifest: &BTreeMap>, enabled_features: &FeaturesOpts, ) -> Result> { - if features_manifest.is_empty() { - match &enabled_features.features { - FeaturesSelector::Features(features) if !features.is_empty() => { - bail!( - "no features in manifest\n\ - note: to use features, you need to define [features] section in Scarb.toml", - ) - } - _ => { - return Ok(None); - } - } - } let available_features: HashSet = features_manifest.keys().cloned().collect(); - let mut selected_features: HashSet = match &enabled_features.features { FeaturesSelector::AllFeatures => available_features.clone(), FeaturesSelector::Features(features) => { - let mut features: HashSet = features.iter().cloned().collect(); + let features: HashSet = features.iter().cloned().collect(); + let mut features: HashSet = features + .intersection(&available_features) + .cloned() + .collect(); if !enabled_features.no_default_features { features.extend( features_manifest diff --git a/scarb/src/ops/subcommands.rs b/scarb/src/ops/subcommands.rs index c958ce964..a3f208ad4 100644 --- a/scarb/src/ops/subcommands.rs +++ b/scarb/src/ops/subcommands.rs @@ -13,7 +13,7 @@ use scarb_ui::components::Status; use crate::core::{Config, Package, ScriptDefinition, Workspace}; use crate::internal::fsx::is_executable; -use crate::ops::{self, FeaturesOpts}; +use crate::ops; use crate::process::exec_replace; use crate::subcommands::{get_env_vars, EXTERNAL_CMD_PREFIX, SCARB_MANIFEST_PATH_ENV}; @@ -57,8 +57,6 @@ pub fn execute_test_subcommand( SCARB_MANIFEST_PATH_ENV.into(), package.manifest_path().to_string(), )]); - // Validate features opts. - let _: FeaturesOpts = features.clone().try_into()?; env.extend(features.to_env_vars()); if let Some(script_definition) = package.manifest.scripts.get("test") { debug!("using `test` script: {script_definition}"); diff --git a/scarb/tests/features.rs b/scarb/tests/features.rs index 98f2042cf..90e96b8cb 100644 --- a/scarb/tests/features.rs +++ b/scarb/tests/features.rs @@ -6,6 +6,7 @@ use indoc::indoc; use scarb_metadata::{Cfg, Metadata}; use scarb_test_support::command::{CommandExt, Scarb}; use scarb_test_support::project_builder::ProjectBuilder; +use scarb_test_support::workspace_builder::WorkspaceBuilder; fn build_example_program(t: &TempDir) { ProjectBuilder::start() @@ -201,7 +202,10 @@ fn features_unknown_feature() { .arg("z") .current_dir(&t) .assert() - .stdout_matches("error: unknown features: z\n") + .stdout_matches(indoc! {r#" + error: none of the selected packages contains `z` feature + note: to use features, you need to define [features] section in Scarb.toml + "#}) .failure(); } @@ -216,7 +220,7 @@ fn features_fail_missing_manifest() { .current_dir(&t) .assert() .stdout_matches(indoc! {r#" - error: no features in manifest + error: none of the selected packages contains `x` feature note: to use features, you need to define [features] section in Scarb.toml "#}) .failure(); @@ -363,3 +367,99 @@ fn features_metadata_feature_in_compilation_units() { main_component_cfg.is_some_and(|cfg| cfg.contains(&Cfg::KV("feature".into(), "x".into()))) ); } + +#[test] +fn features_in_workspace_success() { + let t = TempDir::new().unwrap(); + ProjectBuilder::start() + .name("first") + .manifest_extra(indoc! {r#" + [features] + x = [] + y = [] + "#}) + .lib_cairo(indoc! {r#" + #[cfg(feature: 'x')] + fn f() -> felt252 { 21 } + + #[cfg(feature: 'y')] + fn f() -> felt252 { 59 } + + fn main() -> felt252 { + f() + } + "#}) + .build(&t.child("first")); + ProjectBuilder::start() + .name("second") + .lib_cairo(indoc! {r#" + fn main() -> felt252 { + 12 + } + "#}) + .build(&t.child("second")); + WorkspaceBuilder::start() + .add_member("first") + .add_member("second") + .build(&t); + + Scarb::quick_snapbox() + .arg("check") + .arg("--package") + .arg("first") + .arg("--features") + .arg("x") + .current_dir(&t) + .assert() + .success(); +} + +#[test] +fn features_in_workspace_validated() { + let t = TempDir::new().unwrap(); + ProjectBuilder::start() + .name("first") + .manifest_extra(indoc! {r#" + [features] + x = [] + y = [] + "#}) + .lib_cairo(indoc! {r#" + #[cfg(feature: 'x')] + fn f() -> felt252 { 21 } + + #[cfg(feature: 'y')] + fn f() -> felt252 { 59 } + + fn main() -> felt252 { + f() + } + "#}) + .build(&t.child("first")); + ProjectBuilder::start() + .name("second") + .lib_cairo(indoc! {r#" + fn main() -> felt252 { + 12 + } + "#}) + .build(&t.child("second")); + WorkspaceBuilder::start() + .add_member("first") + .add_member("second") + .build(&t); + + Scarb::quick_snapbox() + .arg("check") + .arg("--package") + .arg("second") + .arg("--features") + .arg("x") + .current_dir(&t) + .assert() + .failure() + .stdout_matches(indoc! {r#" + error: none of the selected packages contains `x` feature + note: to use features, you need to define [features] section in Scarb.toml + "#}); +}