Skip to content

Commit

Permalink
chore: implement basic workspace file functionality (#1913)
Browse files Browse the repository at this point in the history
* add `workspace` field to `PackageManifest`

* cringe imp

* fix fail

* remove `dbg!`

* fix fail [2]

* add docs

* split package and workspace

* fix panic

* add workspace to ssa_refactor folder too

---------

Co-authored-by: kevaundray <[email protected]>
  • Loading branch information
kek kek kek and kevaundray authored Jul 13, 2023
1 parent 2b4237d commit 4969da7
Show file tree
Hide file tree
Showing 21 changed files with 193 additions and 21 deletions.
55 changes: 49 additions & 6 deletions crates/nargo/src/manifest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,55 @@ pub struct PackageManifest {
pub dependencies: BTreeMap<String, Dependency>,
}

/// Contains all the information about a package, as loaded from a `Nargo.toml`.
/// Represents a manifest, which can be either a package manifest or a workspace manifest.
#[derive(Debug, Deserialize, Clone)]
#[serde(untagged)]
pub enum Manifest {
/// Represents a package manifest.
Package(PackageManifest),
/// Represents a workspace manifest.
Workspace(Workspace),
}

impl Manifest {
pub fn from_toml_str(toml_as_string: &str) -> Result<Self, InvalidPackageError> {
let manifest = toml::from_str(toml_as_string)?;
Ok(manifest)
}

pub fn to_package(self) -> Option<PackageManifest> {
match self {
Self::Package(v) => Some(v),
_ => None,
}
}
}

impl PackageManifest {
/// Returns whether the package has a local dependency.
// Local paths are usually relative and are discouraged when sharing libraries
// It is better to separate these into different packages.
pub fn has_local_dependency(&self) -> bool {
self.dependencies.values().any(|dep| matches!(dep, Dependency::Path { .. }))
}
}

pub fn from_toml_str(toml_as_string: &str) -> Result<Self, InvalidPackageError> {
let manifest = toml::from_str::<PackageManifest>(toml_as_string)?;
Ok(manifest)
}
/// Configuration of a workspace in a manifest.
/// Indicates that `[workspace]` was present and the members were specified as well.
#[derive(Debug, Deserialize, Clone)]
pub struct Workspace {
#[serde(rename = "workspace")]
pub config: WorkspaceConfig,
}

#[derive(Default, Debug, Deserialize, Clone)]
pub struct WorkspaceConfig {
pub members: Vec<String>,
}

#[allow(dead_code)]
#[derive(Debug, Deserialize, Clone)]
#[derive(Default, Debug, Deserialize, Clone)]
pub struct PackageMetadata {
// Note: a package name is not needed unless there is a registry
authors: Vec<String>,
Expand Down Expand Up @@ -62,5 +95,15 @@ fn parse_standard_toml() {
hello = {path = "./noir_driver"}
"#;

assert!(PackageManifest::from_toml_str(src).is_ok());
assert!(Manifest::from_toml_str(src).is_ok());
}

#[test]
fn parse_workspace_toml() {
let src = r#"
[workspace]
members = ["a", "b"]
"#;

assert!(Manifest::from_toml_str(src).is_ok());
}
3 changes: 2 additions & 1 deletion crates/nargo_cli/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ fn generate_tests(test_file: &mut File, experimental_ssa: bool) {
if config_data["exclude"].contains(&test_name) { "#[ignore]" } else { "" };

let should_fail = config_data["fail"].contains(&test_name);
let is_workspace = test_dir.to_str().map_or(false, |s| s.contains("workspace"));

write!(
test_file,
Expand All @@ -97,7 +98,7 @@ fn prove_and_verify_{test_sub_dir}_{test_name}() {{
let mut cmd = Command::cargo_bin("nargo").unwrap();
cmd.arg("--program-dir").arg(test_program_dir);
cmd.arg("execute");
cmd.arg(if {is_workspace} {{ "test" }} else {{ "execute" }});
if {experimental_ssa} {{
cmd.arg("--experimental-ssa");
}};
Expand Down
8 changes: 7 additions & 1 deletion crates/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,13 @@ fn run_tests<B: Backend>(
compile_options.experimental_ssa,
)?;

let test_functions = context.get_all_test_functions_in_crate_matching(&LOCAL_CRATE, test_name);
let test_functions = match context.crate_graph.crate_type(LOCAL_CRATE) {
noirc_frontend::graph::CrateType::Workspace => {
context.get_all_test_functions_in_workspace_matching(test_name)
}
_ => context.get_all_test_functions_in_crate_matching(&LOCAL_CRATE, test_name),
};

println!("Running {} test functions...", test_functions.len());
let mut failing = 0;

Expand Down
3 changes: 2 additions & 1 deletion crates/nargo_cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ fn find_package_manifest(current_path: &Path) -> Result<PathBuf, InvalidPackageE
.ok_or_else(|| InvalidPackageError::MissingManifestFile(current_path.to_path_buf()))
}

fn lib_or_bin(current_path: &Path) -> Result<(PathBuf, CrateType), InvalidPackageError> {
fn lib_or_bin(current_path: impl AsRef<Path>) -> Result<(PathBuf, CrateType), InvalidPackageError> {
let current_path = current_path.as_ref();
// A library has a lib.nr and a binary has a main.nr
// You cannot have both.
let src_path = find_dir(current_path, "src")
Expand Down
8 changes: 3 additions & 5 deletions crates/nargo_cli/src/manifest.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
use std::path::Path;

use nargo::manifest::{InvalidPackageError, PackageManifest};
use nargo::manifest::{InvalidPackageError, Manifest};

/// Parses a Nargo.toml file from it's path
/// The path to the toml file must be present.
/// Calling this function without this guarantee is an ICE.
pub(crate) fn parse<P: AsRef<Path>>(
path_to_toml: P,
) -> Result<PackageManifest, InvalidPackageError> {
pub(crate) fn parse<P: AsRef<Path>>(path_to_toml: P) -> Result<Manifest, InvalidPackageError> {
let toml_as_string =
std::fs::read_to_string(&path_to_toml).expect("ice: path given for toml file is invalid");

PackageManifest::from_toml_str(&toml_as_string)
Manifest::from_toml_str(&toml_as_string)
}
44 changes: 38 additions & 6 deletions crates/nargo_cli/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{
path::{Path, PathBuf},
};

use nargo::manifest::{Dependency, PackageManifest};
use nargo::manifest::{Dependency, Manifest, PackageManifest};
use noirc_driver::{add_dep, create_local_crate, create_non_local_crate};
use noirc_frontend::{
graph::{CrateId, CrateType},
Expand Down Expand Up @@ -41,6 +41,14 @@ pub(crate) enum DependencyResolutionError {
/// Dependency is not a valid crate
#[error(transparent)]
MalformedDependency(#[from] InvalidPackageError),

/// Workspace does not contain packages
#[error("manifest path `{}` contains no packages", path.display())]
EmptyWorkspace { path: PathBuf },

/// Use workspace as a dependency is not currently supported
#[error("use workspace as a dependency is not currently supported")]
WorkspaceDependency,
}

#[derive(Debug, Clone)]
Expand All @@ -66,15 +74,37 @@ pub(crate) fn resolve_root_manifest(
dir_path: &std::path::Path,
) -> Result<Context, DependencyResolutionError> {
let mut context = Context::default();
let (entry_path, crate_type) = super::lib_or_bin(dir_path)?;

let manifest_path = super::find_package_manifest(dir_path)?;
let manifest = super::manifest::parse(&manifest_path)?;

let crate_id = create_local_crate(&mut context, entry_path, crate_type);
match manifest {
Manifest::Package(package) => {
let (entry_path, crate_type) = super::lib_or_bin(dir_path)?;
let crate_id = create_local_crate(&mut context, entry_path, crate_type);

let pkg_root = manifest_path.parent().expect("Every manifest path has a parent.");
resolve_manifest(&mut context, crate_id, manifest, pkg_root)?;
let pkg_root = manifest_path.parent().expect("Every manifest path has a parent.");
resolve_manifest(&mut context, crate_id, package, pkg_root)?;
}
Manifest::Workspace(workspace) => {
let members = workspace.config.members;
let root = match members.last() {
Some(member) => dir_path.join(member),
None => {
return Err(DependencyResolutionError::EmptyWorkspace { path: manifest_path })
}
};

let (entry_path, _crate_type) = super::lib_or_bin(root)?;
let _local = create_local_crate(&mut context, entry_path, CrateType::Workspace);

for member in members {
let path: PathBuf = dir_path.join(member);
let (entry_path, crate_type) = super::lib_or_bin(path)?;
create_non_local_crate(&mut context, entry_path, crate_type);
}
}
};

Ok(context)
}
Expand Down Expand Up @@ -138,7 +168,9 @@ fn cache_dep(
) -> Result<CachedDep, DependencyResolutionError> {
let (entry_path, crate_type) = super::lib_or_bin(dir_path)?;
let manifest_path = super::find_package_manifest(dir_path)?;
let manifest = super::manifest::parse(manifest_path)?;
let manifest = super::manifest::parse(manifest_path)?
.to_package()
.ok_or(DependencyResolutionError::WorkspaceDependency)?;
Ok(CachedDep { entry_path, crate_type, manifest, remote })
}

Expand Down
2 changes: 2 additions & 0 deletions crates/nargo_cli/tests/test_data/workspace/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[workspace]
members = ["crates/a", "crates/b"]
2 changes: 2 additions & 0 deletions crates/nargo_cli/tests/test_data/workspace/Prover.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x = "1"
y = "0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[package]
authors = [""]
compiler_version = "0.8.0"

[dependencies]
11 changes: 11 additions & 0 deletions crates/nargo_cli/tests/test_data/workspace/crates/a/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
fn main(x : Field, y : pub Field) {
assert(x != y);
}

#[test]
fn a() {
main(1, 2);

// Uncomment to make test fail
// main(1, 1);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[package]
authors = [""]
compiler_version = "0.8.0"

[dependencies]
11 changes: 11 additions & 0 deletions crates/nargo_cli/tests/test_data/workspace/crates/b/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
fn main(x : Field, y : pub Field) {
assert(x != y);
}

#[test]
fn b() {
main(1, 2);

// Uncomment to make test fail
// main(1, 1);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[workspace]
members = ["crates/a", "crates/b"]
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
x = "1"
y = "0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[package]
authors = [""]
compiler_version = "0.8.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
fn main(x : Field, y : pub Field) {
assert(x != y);
}

#[test]
fn a() {
main(1, 2);

// Uncomment to make test fail
// main(1, 1);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[package]
authors = [""]
compiler_version = "0.8.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
fn main(x : Field, y : pub Field) {
assert(x != y);
}

#[test]
fn b() {
main(1, 2);

// Uncomment to make test fail
// main(1, 1);
}
10 changes: 9 additions & 1 deletion crates/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,15 @@ pub fn check_crate(
context.def_interner.enable_slices = enable_slices;

let mut errors = vec![];
CrateDefMap::collect_defs(LOCAL_CRATE, context, &mut errors);
match context.crate_graph.crate_type(LOCAL_CRATE) {
CrateType::Workspace => {
let keys: Vec<_> = context.crate_graph.iter_keys().collect(); // avoid borrow checker
for crate_id in keys {
CrateDefMap::collect_defs(crate_id, context, &mut errors);
}
}
_ => CrateDefMap::collect_defs(LOCAL_CRATE, context, &mut errors),
}

if has_errors(&errors, deny_warnings) {
Err(errors)
Expand Down
1 change: 1 addition & 0 deletions crates/noirc_frontend/src/graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ pub const CHARACTER_BLACK_LIST: [char; 1] = ['-'];
pub enum CrateType {
Library,
Binary,
Workspace,
}

#[derive(Debug, Clone, PartialEq, Eq)]
Expand Down
10 changes: 10 additions & 0 deletions crates/noirc_frontend/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,16 @@ impl Context {
.collect()
}

pub fn get_all_test_functions_in_workspace_matching(&self, pattern: &str) -> Vec<FuncId> {
let mut tests = Vec::new();

for crate_id in self.crate_graph.iter_keys() {
tests.extend(self.get_all_test_functions_in_crate_matching(&crate_id, pattern));
}

tests
}

/// Return a Vec of all `contract` declarations in the source code and the functions they contain
pub fn get_all_contracts(&self, crate_id: &CrateId) -> Vec<Contract> {
self.def_map(crate_id)
Expand Down

0 comments on commit 4969da7

Please sign in to comment.