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

feat(forge build -vvvvv): If verbosity level is 5 or higher show files to compile #9325

Merged

Conversation

mgiagante
Copy link
Contributor

@mgiagante mgiagante commented Nov 14, 2024

Closes #7888

This PR is to address Issue 7888 by leveraging the new global verbosity level flag implemented by #9273

Motivation

This aims to provide users with a way to see which files have changes relative to what's cached, and thus are effectively picked up by the compiler when running forge build.

Solution

If forge build -v is run, this will set the verbosity level to 1. A level of 1 or higher (with additional v's) will display the list of files being compiled.

@mgiagante mgiagante changed the title Draft: If verbosity level is 1 or higher, it shows dirty files. Draft: feat(forge build -v): If verbosity level is 1 or higher, it shows dirty files. Nov 14, 2024
@grandizzy grandizzy changed the title Draft: feat(forge build -v): If verbosity level is 1 or higher, it shows dirty files. feat(forge build -v): If verbosity level is 1 or higher, it shows dirty files. Nov 15, 2024
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! I think this could also close #9304 so when passing -v flag avoids confusion re files that are compiled, @zerosnacks wdyt?

crates/common/src/term.rs Outdated Show resolved Hide resolved
crates/common/src/term.rs Outdated Show resolved Hide resolved
crates/common/src/term.rs Outdated Show resolved Hide resolved
@zerosnacks
Copy link
Member

thank you! I think this could also close #9304 so when passing -v flag avoids confusion re files that are compiled, @zerosnacks wdyt?

Supportive, I think a warning here would still be beneficial as well as this -v flag would indicate the files being compiled but that doesn't offer an explanation as to why they are. I think raising a warning on the fallback trigger (if a path is specified) would still be the right way but this PR is in the right direction

@mgiagante
Copy link
Contributor Author

@zerosnacks @grandizzy is there currently a way to write tests for this? I've tried the tests in crates/forge/tests/cli/build.rs but the output that needs to be asserted on here doesn't seem to be included.

@grandizzy
Copy link
Collaborator

build.rs

@mgiagante I think that's fine without a test, sorry for back and forth, could you remove the newly added variant SpinnerMsg::VerboseMsg (which is not really required) and use same SpinnerMsg::VerboseMsg / send_msg for sending verbose message, then this should be good to send. thank you

@grandizzy grandizzy self-requested a review November 18, 2024 09:08
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! pending one more review before merging

Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some minor modifications to the rendered output

Now renders as (path trimmed, sorted, prefixed):

[⠊] Compiling...
[⠒] Files to compile:
- lib/forge-std/src/Base.sol
- lib/forge-std/src/Script.sol
- lib/forge-std/src/StdAssertions.sol
- lib/forge-std/src/StdChains.sol
- lib/forge-std/src/StdCheats.sol
- lib/forge-std/src/StdError.sol
- lib/forge-std/src/StdInvariant.sol
- lib/forge-std/src/StdJson.sol
- lib/forge-std/src/StdMath.sol
- lib/forge-std/src/StdStorage.sol
- lib/forge-std/src/StdStyle.sol
- lib/forge-std/src/StdToml.sol
- lib/forge-std/src/StdUtils.sol
- lib/forge-std/src/Test.sol
- lib/forge-std/src/Vm.sol
- lib/forge-std/src/console.sol
- lib/forge-std/src/console2.sol
- lib/forge-std/src/interfaces/IERC165.sol
- lib/forge-std/src/interfaces/IERC20.sol
- lib/forge-std/src/interfaces/IERC721.sol
- lib/forge-std/src/interfaces/IMulticall3.sol
- lib/forge-std/src/mocks/MockERC20.sol
- lib/forge-std/src/mocks/MockERC721.sol
- lib/forge-std/src/safeconsole.sol
- script/Counter.s.sol
- src/Counter.sol
- src/Counter2.sol
- src/Counter3.sol
- src/Counter4.sol
- test/Counter.t.sol
[⠒] Compiling 30 files with Solc 0.8.28
[⠆] Solc 0.8.28 finished in 950.06ms
Compiler run successful!

As opposed to

[⠒] Compiling files
[⠊] Compiling...
[⠒] Compiling files
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdChains.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/interfaces/IERC165.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/Test.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/interfaces/IERC20.sol
/home/zerosnacks/Projects/counter/src/Counter.sol
/home/zerosnacks/Projects/counter/src/Counter3.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/interfaces/IERC721.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdStyle.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdToml.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/mocks/MockERC721.sol
/home/zerosnacks/Projects/counter/test/Counter.t.sol
/home/zerosnacks/Projects/counter/src/Counter2.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/Base.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/mocks/MockERC20.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdError.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdMath.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdCheats.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/Vm.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdInvariant.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/Script.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/interfaces/IMulticall3.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/console2.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/console.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdStorage.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdAssertions.sol
/home/zerosnacks/Projects/counter/src/Counter4.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/safeconsole.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdJson.sol
/home/zerosnacks/Projects/counter/lib/forge-std/src/StdUtils.sol
/home/zerosnacks/Projects/counter/script/Counter.s.sol
[⠢] Compiling 30 files with Solc 0.8.28
[⠆] Solc 0.8.28 finished in 984.73ms
Compiler run successful

cc @grandizzy

@zerosnacks zerosnacks changed the title feat(forge build -v): If verbosity level is 1 or higher, it shows dirty files. feat(forge build -v): If verbosity level is 1 or higher show files to compile Nov 18, 2024
@zerosnacks zerosnacks enabled auto-merge (squash) November 18, 2024 09:50
@DaniPopes
Copy link
Member

This is extremely verbose, and it is common for verbosity to be set to 2 or 3 when running test. Unfortunately coupling testing verbosity to everywhere else means that we need to set higher verbosity values for stuff like this.

I'd argue this should be like a trace level log, so 5 or above

@mgiagante mgiagante changed the title feat(forge build -v): If verbosity level is 1 or higher show files to compile feat(forge build -vvvvv): If verbosity level is 5 or higher show files to compile Nov 18, 2024
@mgiagante
Copy link
Contributor Author

This is extremely verbose, and it is common for verbosity to be set to 2 or 3 when running test. Unfortunately coupling testing verbosity to everywhere else means that we need to set higher verbosity values for stuff like this.

I'd argue this should be like a trace level log, so 5 or above

That makes a lot of sense. I'll go with 5 then.
Thanks!

mgiagante and others added 2 commits November 18, 2024 10:19
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
@zerosnacks
Copy link
Member

This is extremely verbose, and it is common for verbosity to be set to 2 or 3 when running test. Unfortunately coupling testing verbosity to everywhere else means that we need to set higher verbosity values for stuff like this.

I'd argue this should be like a trace level log, so 5 or above

Hmm.. yeah this is not great. For now let's do -vvvvv to avoid interference.

Ideally we would have a way to distinguish if the user intentionally runs forge build or it is part of a process like forge test and adapt based on that. I suggest post-V1 we consider how to apply a more uniform scale.

@mgiagante
Copy link
Contributor Author

This is extremely verbose, and it is common for verbosity to be set to 2 or 3 when running test. Unfortunately coupling testing verbosity to everywhere else means that we need to set higher verbosity values for stuff like this.
I'd argue this should be like a trace level log, so 5 or above

Hmm.. yeah this is not great. For now let's do -vvvvv to avoid interference.

Ideally we would have a way to distinguish if the user intentionally runs forge build or it is part of a process like forge test and adapt based on that. I suggest post-V1 we consider how to apply a more uniform scale.

Just created #9340 to track this.

@zerosnacks
Copy link
Member

Just created #9340 to track this.

All good, added it as a note in #8794

@grandizzy grandizzy merged commit 7e323c2 into foundry-rs:master Nov 18, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat(forge build): add flag to show names of files that are not cached and being compiled
4 participants