Skip to content

Commit

Permalink
feat: disallow nested packages (FuelLabs#4407)
Browse files Browse the repository at this point in the history
## Description

closes FuelLabs#4072 

Disallows package structure such as the following:

```console
proj1
β”œβ”€β”€ proj2
β”‚   β”œβ”€β”€ Forc.lock
β”‚   β”œβ”€β”€ Forc.toml
β”‚   β”œβ”€β”€ out
β”‚   └── src
β”œβ”€β”€ Forc.lock
β”œβ”€β”€ Forc.toml
β”œβ”€β”€ out
β”‚   └── debug
└── src
    └── main.sw
```
  • Loading branch information
kayagokalp authored Apr 13, 2023
1 parent 83d5578 commit 1e63c15
Show file tree
Hide file tree
Showing 13 changed files with 100 additions and 31 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 35 additions & 21 deletions forc-pkg/src/manifest.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::pkg::{manifest_file_missing, parsing_failed, wrong_program_type};
use anyhow::{anyhow, bail, Context, Result};
use forc_tracing::println_yellow_err;
use forc_util::{find_manifest_dir, validate_name};
use forc_util::{find_nested_manifest_dir, find_parent_manifest_dir, validate_name};
use serde::{Deserialize, Serialize};
use std::{
collections::{BTreeMap, HashMap},
Expand Down Expand Up @@ -249,7 +249,7 @@ impl PackageManifestFile {
/// This is short for `PackageManifest::from_file`, but takes care of constructing the path to the
/// file.
pub fn from_dir(manifest_dir: &Path) -> Result<Self> {
let dir = forc_util::find_manifest_dir(manifest_dir)
let dir = forc_util::find_parent_manifest_dir(manifest_dir)
.ok_or_else(|| manifest_file_missing(manifest_dir))?;
let path = dir.join(constants::MANIFEST_FILE_NAME);
Self::from_file(path)
Expand All @@ -273,6 +273,18 @@ impl PackageManifestFile {
self.project.entry
)
}

// Check for nested packages.
//
// `path` is the path to manifest file. To start nested package search we need to start
// from manifest's directory. So, last part of the path (the filename, "/forc.toml") needs
// to be removed.
let mut pkg_dir = self.path.to_path_buf();
pkg_dir.pop();
if let Some(nested_package) = find_nested_manifest_dir(&pkg_dir) {
// remove file name from nested_package_manifest
bail!("Nested packages are not supported, please consider seperating the nested package at {} from the package at {}, or if it makes sense consider creating a workspace.", nested_package.display(), pkg_dir.display())
}
Ok(())
}

Expand Down Expand Up @@ -450,7 +462,8 @@ impl PackageManifest {
/// This is short for `PackageManifest::from_file`, but takes care of constructing the path to the
/// file.
pub fn from_dir(dir: &Path) -> Result<Self> {
let manifest_dir = find_manifest_dir(dir).ok_or_else(|| manifest_file_missing(dir))?;
let manifest_dir =
find_parent_manifest_dir(dir).ok_or_else(|| manifest_file_missing(dir))?;
let file_path = manifest_dir.join(constants::MANIFEST_FILE_NAME);
Self::from_file(&file_path)
}
Expand Down Expand Up @@ -734,24 +747,25 @@ impl WorkspaceManifestFile {
/// This is short for `PackageManifest::from_file`, but takes care of constructing the path to the
/// file.
pub fn from_dir(manifest_dir: &Path) -> Result<Self> {
let dir = forc_util::find_manifest_dir_with_check(manifest_dir, |possible_manifest_dir| {
// Check if the found manifest file is a workspace manifest file or a standalone
// package manifest file.
let possible_path = possible_manifest_dir.join(constants::MANIFEST_FILE_NAME);
// We should not continue to search if the given manifest is a workspace manifest with
// some issues.
//
// If the error is missing field `workspace` (which happens when trying to read a
// package manifest as a workspace manifest), look into the parent directories for a
// legitimate workspace manifest. If the error returned is something else this is a
// workspace manifest with errors, classify this as a workspace manifest but with
// errors so that the erros will be displayed to the user.
Self::from_file(possible_path)
.err()
.map(|e| !e.to_string().contains("missing field `workspace`"))
.unwrap_or_else(|| true)
})
.ok_or_else(|| manifest_file_missing(manifest_dir))?;
let dir =
forc_util::find_parent_manifest_dir_with_check(manifest_dir, |possible_manifest_dir| {
// Check if the found manifest file is a workspace manifest file or a standalone
// package manifest file.
let possible_path = possible_manifest_dir.join(constants::MANIFEST_FILE_NAME);
// We should not continue to search if the given manifest is a workspace manifest with
// some issues.
//
// If the error is missing field `workspace` (which happens when trying to read a
// package manifest as a workspace manifest), look into the parent directories for a
// legitimate workspace manifest. If the error returned is something else this is a
// workspace manifest with errors, classify this as a workspace manifest but with
// errors so that the erros will be displayed to the user.
Self::from_file(possible_path)
.err()
.map(|e| !e.to_string().contains("missing field `workspace`"))
.unwrap_or_else(|| true)
})
.ok_or_else(|| manifest_file_missing(manifest_dir))?;
let path = dir.join(constants::MANIFEST_FILE_NAME);
Self::from_file(path)
}
Expand Down
8 changes: 4 additions & 4 deletions forc-plugins/forc-fmt/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use taplo::formatter as taplo_fmt;
use tracing::{error, info};

use forc_tracing::{init_tracing_subscriber, println_green, println_red};
use forc_util::{find_manifest_dir, is_sway_file};
use forc_util::{find_parent_manifest_dir, is_sway_file};
use sway_core::{BuildConfig, BuildTarget};
use sway_utils::{constants, get_sway_files};
use swayfmt::Formatter;
Expand Down Expand Up @@ -62,8 +62,8 @@ fn run() -> Result<()> {

// If we're formatting a single file, find the nearest manifest if within a project.
// Otherwise, we simply provide 'None' to format_file().
let manifest_file =
find_manifest_dir(file_path).map(|path| path.join(constants::MANIFEST_FILE_NAME));
let manifest_file = find_parent_manifest_dir(file_path)
.map(|path| path.join(constants::MANIFEST_FILE_NAME));

if is_sway_file(file_path) {
format_file(&app, file_path.to_path_buf(), manifest_file, &mut formatter)?;
Expand Down Expand Up @@ -252,7 +252,7 @@ fn format_manifest(app: &App, manifest_file: PathBuf) -> Result<bool> {

/// Format the package at the given directory.
fn format_pkg_at_dir(app: &App, dir: &Path, formatter: &mut Formatter) -> Result<()> {
match find_manifest_dir(dir) {
match find_parent_manifest_dir(dir) {
Some(path) => {
let manifest_path = path.clone();
let manifest_file = manifest_path.join(constants::MANIFEST_FILE_NAME);
Expand Down
1 change: 1 addition & 0 deletions forc-util/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ sway-utils = { version = "0.37.0", path = "../sway-utils" }
tracing = "0.1"
tracing-subscriber = { version = "0.3", features = ["ansi", "env-filter", "json"] }
unicode-xid = "0.2.2"
walkdir = "2.3.3"
38 changes: 34 additions & 4 deletions forc-util/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,37 @@ pub fn format_log_receipts(receipts: &[fuel_tx::Receipt], pretty_print: bool) ->
}
}

/// Continually go down in the file tree until a Forc manifest file is found.
pub fn find_nested_manifest_dir(starter_path: &Path) -> Option<PathBuf> {
find_nested_dir_with_file(starter_path, constants::MANIFEST_FILE_NAME)
}

/// Continually go down in the file tree until a specified file is found.
///
/// Starts the search from child dirs of `starter_path`.
pub fn find_nested_dir_with_file(starter_path: &Path, file_name: &str) -> Option<PathBuf> {
use walkdir::WalkDir;
let starter_dir = if starter_path.is_dir() {
starter_path
} else {
starter_path.parent()?
};
WalkDir::new(starter_path)
.into_iter()
.filter_map(|e| e.ok())
.filter(|entry| entry.path() != starter_dir.join(file_name))
.filter(|entry| entry.file_name().to_string_lossy() == file_name)
.map(|entry| {
let mut entry = entry.path().to_path_buf();
entry.pop();
entry
})
.next()
}

/// Continually go up in the file tree until a specified file is found.
///
/// Starts the search from `starter_path`.
#[allow(clippy::branches_sharing_code)]
pub fn find_parent_dir_with_file(starter_path: &Path, file_name: &str) -> Option<PathBuf> {
let mut path = std::fs::canonicalize(starter_path).ok()?;
Expand All @@ -82,22 +112,22 @@ pub fn find_parent_dir_with_file(starter_path: &Path, file_name: &str) -> Option
None
}
/// Continually go up in the file tree until a Forc manifest file is found.
pub fn find_manifest_dir(starter_path: &Path) -> Option<PathBuf> {
pub fn find_parent_manifest_dir(starter_path: &Path) -> Option<PathBuf> {
find_parent_dir_with_file(starter_path, constants::MANIFEST_FILE_NAME)
}

/// Continually go up in the file tree until a Forc manifest file is found and given predicate
/// returns true.
pub fn find_manifest_dir_with_check<F>(starter_path: &Path, f: F) -> Option<PathBuf>
pub fn find_parent_manifest_dir_with_check<F>(starter_path: &Path, f: F) -> Option<PathBuf>
where
F: Fn(&Path) -> bool,
{
find_manifest_dir(starter_path).and_then(|manifest_dir| {
find_parent_manifest_dir(starter_path).and_then(|manifest_dir| {
// If given check satisifies return current dir otherwise start searching from the parent.
if f(&manifest_dir) {
Some(manifest_dir)
} else if let Some(parent_dir) = manifest_dir.parent() {
find_manifest_dir_with_check(parent_dir, f)
find_parent_manifest_dir_with_check(parent_dir, f)
} else {
None
}
Expand Down
4 changes: 2 additions & 2 deletions forc/src/ops/forc_clean.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::cli::CleanCommand;
use anyhow::{anyhow, bail, Result};
use forc_pkg::manifest::ManifestFile;
use forc_util::{default_output_directory, find_manifest_dir};
use forc_util::{default_output_directory, find_parent_manifest_dir};
use std::path::PathBuf;
use sway_utils::MANIFEST_FILE_NAME;

Expand All @@ -15,7 +15,7 @@ pub fn clean(command: CleanCommand) -> Result<()> {
std::env::current_dir().map_err(|e| anyhow!("{:?}", e))?
};

let manifest_dir = match find_manifest_dir(&this_dir) {
let manifest_dir = match find_parent_manifest_dir(&this_dir) {
Some(dir) => dir,
None => {
bail!(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
out
target
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[project]
authors = ["Fuel Labs <[email protected]>"]
entry = "main.sw"
license = "Apache-2.0"
name = "test_contract"
implicit-std = false
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
contract;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
out
target
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[project]
authors = ["Fuel Labs <[email protected]>"]
entry = "main.sw"
license = "Apache-2.0"
name = "sub_package"
implicit-std = false
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
contract;
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
category = "fail"

# check: $()Nested packages are not supported, please consider seperating the nested package at
# check: $() from the package at
# check: $(), or if it makes sense consider creating a workspace.

0 comments on commit 1e63c15

Please sign in to comment.