feat(build): add option to build specific dir #8149

grandizzy · 2024-06-13T09:51:34Z

Motivation

Not too familiar with build but at a quick look seemed to be a low hanging fruit with big impact for other tools integration, so here it is, hope it makes sense 😄

Adds capabilities to build specific directories and file (no toml config) by specifying --paths switch to forge build e.g.

forge build --paths test/regressions/ test/invariant/Invariant.sol

builds child dirs within specified dirs
works in conjunction with --skip as well

Solution

mattsse

iirc a lot of folks are also requesting this, and this actually looks simple to add. because this is opt-in I belive this is fine

ptal @klkvr

mattsse · 2024-06-13T11:12:47Z

crates/forge/bin/cmd/build.rs

+ let mut files = vec![];
+ if let Some(dirs) = self.args.dirs {
+ for dir in dirs {
+ for entry in fs::read_dir(dir)? {
+ let entry = entry?;
+ let path = entry.path();
+ if path.is_file() {
+ files.push(entry.path());
+ }
+ }
+ }
+ }


I believe there should be utils to find the sol/vy files in a dir @klkvr ?

I believe
https://github.com/foundry-rs/compilers/blob/43da903df7231bf60377c85a6d76ae6b088c723d/src/utils.rs#L113-L113

Right, similarly to this

foundry/crates/forge/bin/cmd/test/mod.rs

Lines 219 to 222 in b7dcf46

test_sources.extend(source_files_iter(

project.paths.sources,

MultiCompilerLanguage::FILE_EXTENSIONS,

));

nice, changed it in 6da9a55

mattsse · 2024-06-13T11:13:43Z

crates/cli/src/opts/build/core.rs

+ /// Child directories are not accounted and should be explicitly added.
+ #[arg(long, num_args(1..))]
+ #[serde(skip)]
+ pub dirs: Option<Vec<PathBuf>>,


should we also support individual files?
maybe we do files:?

yeah, since now it supports single files I renamed it to paths

DaniPopes · 2024-06-13T11:17:33Z

Can't we just pass these as a list of 0.. positional arguments?

klkvr · 2024-06-13T12:45:10Z

perhaps we also want a config key for this? similar to skip from #8061

- use source_files_iter helper, build single files and child dirs as well - rename arg to paths, use 0.. pos arg

grandizzy · 2024-06-13T14:10:36Z

Can't we just pass these as a list of 0.. positional arguments?

hope I get this comment right, pls check 6da9a55#diff-f3c6bdb4ebdb62f63412254e4c9154aff947fc45daf6aaff880aaf6470182d8aR129-R132

grandizzy · 2024-06-13T14:24:31Z

perhaps we also want a config key for this? similar to skip from #8061

I think that would be cool, a difference from skip is that the arg is named paths in BuildArgs but probably the config key should be meaningful named build-paths? Also not sure but would this imply a change in MultiCompiler, I could be wrong but does forge build command uses only solc compiler?

LE: on a 2nd thought, since this cannot be shared with other commands and kind of valid only in build context, do we want to move it as a config key?

klkvr · 2024-06-13T15:12:41Z

I think that would be cool, a difference from skip is that the arg is named paths in BuildArgs but probably the config key should be meaningful named build-paths? Also not sure but would this imply a change in MultiCompiler, I could be wrong but does forge build command uses only solc compiler?

This wouldn't require a change in compilers, should be doable by only using sparse output filters

LE: on a 2nd thought, since this cannot be shared with other commands and kind of valid only in build context, do we want to move it as a config key?

yeah I meant making it valid in all context if specified in config. Since #8061 we respect skip for all context by converting it into sparse output filter directly in Config::create_project:

foundry/crates/config/src/lib.rs

Lines 811 to 813 in b7dcf46

 if !self.skip.is_empty() { 

 let filter = SkipBuildFilters::new(self.skip.clone(), self.root.0.clone()); 

 builder = builder.sparse_output(filter);

grandizzy · 2024-06-13T19:15:19Z

yeah I meant making it valid in all context if specified in config. Since #8061 we respect skip for all context by converting it into sparse output filter directly in Config::create_project:

foundry/crates/config/src/lib.rs

Lines 811 to 813 in b7dcf46

if !self.skip.is_empty() {

let filter = SkipBuildFilters::new(self.skip.clone(), self.root.0.clone());

builder = builder.sparse_output(filter);

I think I got it now, but wouldn't this be an issue with the sparse output filter when we want to include files (instead excluding them?
Let's say I specify file test/Bar.sol is Test as path to build, then I create a FileFilter and implement is_match function that would return true if test/Bar.sol is a match (but false for Test), is there a way to figure out all other deps to build (as rn I don't see the test.sol generated in out dir, while for forge build --paths test/Bar.sol I get them both)

klkvr · 2024-06-13T19:40:04Z

I think I got it now, but wouldn't this be an issue with the sparse output filter when we want to include files (instead excluding them?
Let's say I specify file test/Bar.sol is Test as path to build, then I create a FileFilter and implement is_match function that would return true if test/Bar.sol is a match (but false for Test), is there a way to figure out all other deps to build (as rn I don't see the test.sol generated in out dir, while for forge build --paths test/Bar.sol I get them both)

Right, the way sparse output works is that it only requests artifacts for matched sources, excluding everything else. Such approach reduces compile time but might be a bit unexpected if people are expecting artifacts for imported sources to also be produced.

However another issue is if we approach this by just using sparse output filters then this wouldn't give users a way to compile files which are not under src/ as they won't be included into input sources set.

Let's keep the config option for a future follow-up

klkvr · 2024-06-13T19:40:41Z

crates/forge/bin/cmd/build.rs

@@ -18,6 +18,9 @@ use watchexec::config::{InitConfig, RuntimeConfig};

 foundry_config::merge_impl_figment_convert!(BuildArgs, args);

+// Extensions accepted by `forge build`
+const BUILD_EXTENSIONS: &[&str] = &["sol", "yul", "vy", "vyi"];


can we use MultiCompilerLanguage::FILE_EXTENSIONS here? It would require including Language trait from compilers

yep, changed in 0e71ca5

klkvr · 2024-06-14T00:26:29Z

Can't we just pass these as a list of 0.. positional arguments?

hope I get this comment right, pls check 6da9a55#diff-f3c6bdb4ebdb62f63412254e4c9154aff947fc45daf6aaff880aaf6470182d8aR129-R132
#8158

@grandizzy sorry missed this, I think Dani meant changing it to positional args to allow forge build file.sol commands. Created small follow-up #8158, just moves paths to BuildArgs basically

grandizzy · 2024-06-14T03:10:49Z

Can't we just pass these as a list of 0.. positional arguments?

hope I get this comment right, pls check 6da9a55#diff-f3c6bdb4ebdb62f63412254e4c9154aff947fc45daf6aaff880aaf6470182d8aR129-R132
#8158

@grandizzy sorry missed this, I think Dani meant changing it to positional args to allow forge build file.sol commands. Created small follow-up #8158, just moves paths to BuildArgs basically

Ah, I see, thank you for following up!

feat(build): add option to build specific dir

d67bbb7

grandizzy requested a review from klkvr June 13, 2024 10:03

grandizzy marked this pull request as ready for review June 13, 2024 10:03

grandizzy requested review from DaniPopes, Evalir and mattsse as code owners June 13, 2024 10:03

mattsse requested changes Jun 13, 2024

View reviewed changes

Changes after review:

6da9a55

- use source_files_iter helper, build single files and child dirs as well - rename arg to paths, use 0.. pos arg

mattsse approved these changes Jun 13, 2024

View reviewed changes

klkvr reviewed Jun 13, 2024

View reviewed changes

Changes after review: reuse MultiCompilerLanguage::FILE_EXTENSIONS

0e71ca5

klkvr approved these changes Jun 14, 2024

View reviewed changes

klkvr merged commit c2e5297 into foundry-rs:master Jun 14, 2024
19 checks passed

klkvr mentioned this pull request Jun 14, 2024

fix: make paths a positional argument #8158

Merged

grandizzy deleted the issue-8071 branch June 14, 2024 03:10

0xalpharush mentioned this pull request Jun 19, 2024

fix integration with echidna and medusa so that caching works crytic/crytic-compile#563

Open

zerosnacks mentioned this pull request Jul 5, 2024

forge build SomeContract.sol (target a single file or glob of files) #6278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(build): add option to build specific dir #8149

feat(build): add option to build specific dir #8149

grandizzy commented Jun 13, 2024 •

edited

Loading

mattsse left a comment

mattsse Jun 13, 2024

klkvr Jun 13, 2024 •

edited

Loading

grandizzy Jun 13, 2024

mattsse Jun 13, 2024

grandizzy Jun 13, 2024

DaniPopes commented Jun 13, 2024 •

edited

Loading

klkvr commented Jun 13, 2024

grandizzy commented Jun 13, 2024

grandizzy commented Jun 13, 2024 •

edited

Loading

klkvr commented Jun 13, 2024

grandizzy commented Jun 13, 2024

klkvr commented Jun 13, 2024

klkvr Jun 13, 2024 •

edited

Loading

grandizzy Jun 13, 2024

klkvr commented Jun 14, 2024

grandizzy commented Jun 14, 2024

	test_sources.extend(source_files_iter(
	project.paths.sources,
	MultiCompilerLanguage::FILE_EXTENSIONS,
	));

feat(build): add option to build specific dir #8149

feat(build): add option to build specific dir #8149

Conversation

grandizzy commented Jun 13, 2024 • edited Loading

Motivation

Solution

mattsse left a comment

Choose a reason for hiding this comment

mattsse Jun 13, 2024

Choose a reason for hiding this comment

klkvr Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

grandizzy Jun 13, 2024

Choose a reason for hiding this comment

mattsse Jun 13, 2024

Choose a reason for hiding this comment

grandizzy Jun 13, 2024

Choose a reason for hiding this comment

DaniPopes commented Jun 13, 2024 • edited Loading

klkvr commented Jun 13, 2024

grandizzy commented Jun 13, 2024

grandizzy commented Jun 13, 2024 • edited Loading

klkvr commented Jun 13, 2024

grandizzy commented Jun 13, 2024

klkvr commented Jun 13, 2024

klkvr Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

grandizzy Jun 13, 2024

Choose a reason for hiding this comment

klkvr commented Jun 14, 2024

grandizzy commented Jun 14, 2024

grandizzy commented Jun 13, 2024 •

edited

Loading

klkvr Jun 13, 2024 •

edited

Loading

DaniPopes commented Jun 13, 2024 •

edited

Loading

grandizzy commented Jun 13, 2024 •

edited

Loading

klkvr Jun 13, 2024 •

edited

Loading