Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support glob patterns in --skip option (e.g. to skip entire tests dir) #4628

Closed
fubhy opened this issue Mar 23, 2023 · 7 comments · Fixed by #5267
Closed

Support glob patterns in --skip option (e.g. to skip entire tests dir) #4628

fubhy opened this issue Mar 23, 2023 · 7 comments · Fixed by #5267
Labels
A-compiler Area: compiler C-forge Command: forge Cmd-forge-build Command: forge build T-feature Type: feature

Comments

@fubhy
Copy link
Contributor

fubhy commented Mar 23, 2023

Component

Forge

Describe the feature you would like

Currently, you can specify the --skip option when running forge build to ignore certain files specifically. There's also special handlings for scripts and tests which excludes .t.sol and .s.sol files respecitively. It would be great if we could also exclude glob patterns.

My use-case specifically is to run forge build while ignoring the entire tests directory when generating code based on forge build artifacts. Specifically, I'm using the build artifacts to then run cast interface on them to produce interface representations for different solc versions so I can write my integration tests in Solidity >0.8.10). This creates a "chicken and egg" problem where I can't generate those interfaces if my test files currently don't compile but in order for them to compile I'd need to generate the interfaces first.

Additional context

We've solved this for now with a little workaround:

...

$(ARTIFACTS_DIR): Makefile $(shell find $(CONTRACTS_DIR) -type f -name "*.sol")
> mkdir -p $(@D)
> # Remove this once the `forge build` command supports glob patterns in `--skip` option.
> export FOUNDRY_TEST=this-directory-does-not-exist
> forge build --extra-output-files abi
> touch $@ 

...
@fubhy fubhy added the T-feature Type: feature label Mar 23, 2023
@gakonst gakonst added this to Foundry Mar 23, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Mar 23, 2023
@fubhy fubhy changed the title Allow forge build to skip entire tests dir Support glob patterns in --skip option (e.g. to skip entire tests dir) Mar 23, 2023
@mds1
Copy link
Collaborator

mds1 commented Mar 24, 2023

@mattsse do we currently only support skip filters by file name here, and not by paths? If so this seems like a good idea, because helper files in test/script dirs usually do not contain the .t.sol or .s.sol extension

@mds1 mds1 added C-forge Command: forge Cmd-forge-build Command: forge build A-compiler Area: compiler labels Mar 24, 2023
@0xalpharush
Copy link
Contributor

This would be extremely valuable for users running Slither on their foundry codebase. Unfortunately, running --skip test script often still includes other test/script files since they don't have the extension but import scripts/tests.

xref crytic/crytic-compile#435

@mattsse
Copy link
Member

mattsse commented Jun 2, 2023

just to summarize:

--skip should support globs?

Unfortunately, running --skip test script often still includes other test/script files since they don't have the extension but import scripts/tests.

I don't fully understand what this means exactly.

should --skip test script behave as glob that match the file path and exclude those that match?

@mattsse
Copy link
Member

mattsse commented Jun 2, 2023

okay this should actually be very simple:

just need to change how this filter behaves, because rn it operates on file name and expects .(s|t).sol style naming, that's why you observed that sometimes it pulls in contracts from tests scripts folder, gotcha

sorry should have followed up on this issue earlier...

impl FileFilter for SkipBuildFilter {
/// Matches file only if the filter does not apply
///
/// This is returns the inverse of `file.name.contains(pattern)`
fn is_match(&self, file: &Path) -> bool {
fn exclude(file: &Path, pattern: &str) -> Option<bool> {
let file_name = file.file_name()?.to_str()?;
Some(file_name.contains(pattern))
}
!exclude(file, self.file_pattern()).unwrap_or_default()
}
}

@0xalpharush
Copy link
Contributor

What will happen if a skipped file is a dependency of a file in src/? It may be too opinionated but I don't think src/ should take a dependency on tests or scripts. Unless that's enforced, I'd almost rather get an error that the glob pattern cannot be honored due to the dependency as opposed to it getting pulled into the artifacts.

@0xalpharush
Copy link
Contributor

@mattsse If you could point me in the right direction, I could probably knock this out

@mattsse
Copy link
Member

mattsse commented Jun 23, 2023

ah yikes, sorry I totally forgot about this again...

I think what we want here is a variant that supports globs?

/// A filter that excludes matching contracts from the build
#[derive(Debug, Clone, Eq, PartialEq)]
pub enum SkipBuildFilter {
/// Exclude all `.t.sol` contracts
Tests,
/// Exclude all `.s.sol` contracts
Scripts,
/// Exclude if the file matches
Custom(String),
}

or rather interpret the Custom string variant as glob when checking if the file is a match:

impl FileFilter for SkipBuildFilter {
/// Matches file only if the filter does not apply
///
/// This is returns the inverse of `file.name.contains(pattern)`
fn is_match(&self, file: &Path) -> bool {
fn exclude(file: &Path, pattern: &str) -> Option<bool> {
let file_name = file.file_name()?.to_str()?;
Some(file_name.contains(pattern))
}
!exclude(file, self.file_pattern()).unwrap_or_default()
}
}

currently this just checks if the file name includes the value:

SkipBuildFilter::Custom(s) => s.as_str(),

https://github.com/foundry-rs/foundry/blob/8ecdc2af69bb7e61c0cd6a3c994b58230295b406/common/src/compile.rs#L531C48-L532

we could check if this is a glob

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiler Area: compiler C-forge Command: forge Cmd-forge-build Command: forge build T-feature Type: feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants