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): add compiler subcommand #7909

Merged
merged 20 commits into from
Oct 16, 2024
Merged

Conversation

jpgonzalezra
Copy link
Contributor

@jpgonzalezra jpgonzalezra commented May 11, 2024

Closes: #7656

Motivation

The "issue 7654" it is proposed to add a new command, forge detect-solc (in this PR "forge solc vr" or "forge solc version-resolving"), which returns the detected version of the Solidity compiler based on the project configuration. This command aims to help users identify compatible versions of the Solidity compiler for different folders in large projects by providing insights into the version resolution algorithm used.

Solution

Added a solc command with the subcommand version-resolving, which retrieves the resolved versions of the Solidity compiler (solc) within the project using the SolcVersionManager from the foundry_compilers library.

  • Ex. with solmate project
$ forge solc vr 

Screenshot 2024-05-11 at 18 40 24

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this seems fine,

wdyt @klkvr

Copy link
Member

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

looks good to me

@mds1
Copy link
Collaborator

mds1 commented May 13, 2024

Support for the --skip flag that forge build has would be really nice to allow filtering out directories from the results. When I test this, the output is dominated by e.g. lib/ and test/ files that obscure the src/ files of interest (not a blocker for this PR, can be done in a follow up)

@jpgonzalezra
Copy link
Contributor Author

jpgonzalezra commented May 13, 2024

Thanks for the comments, I can add support for the --skip flag in this PR or another one.
And maybe we can add --silent to only print the version, something link:

one version:

  • 0.8.15

More than one:

  • 0.8.15
  • 0.8.25

What do you think guys?

@zerosnacks
Copy link
Member

zerosnacks commented Jul 15, 2024

Hi @jpgonzalezra thanks for the PR! Would be great to get this ready to be merged

I support adding the --skip flag to this PR. Perhaps rather than --silent it makes sense to add different levels of verbosity for the logs that are emitted where the lowest level yields:

  • 0.8.15
  • 0.8.25

And higher levels of verbosity as proposed by you:

- 0.8.15
  - <path>
  - <path>
  - <path>

- 0.8.25
  - <path>
  - <path>
  - <path>

@jpgonzalezra
Copy link
Contributor Author

@zerosnacks Awesome, I'll fix the conflicts so we can merge. If we want to add anything else, I'll do it in a separate PR. What do you think?

@zerosnacks
Copy link
Member

@zerosnacks Awesome, I'll fix the conflicts so we can merge. If we want to add anything else, I'll do it in a separate PR. What do you think?

Great, we can do this as follow ups - not a blocker for me for this PR

@jpgonzalezra
Copy link
Contributor Author

@zerosnacks I need to make some changes to the PR because there have been significant updates in the compilers repository. See this commit: foundry-rs/compilers@c78e2ea. I believe I need to use the "detect_version" function. I'll work on that. 💪

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 31, 2024
@zerosnacks
Copy link
Member

Thanks @jpgonzalezra! Let me know if you run into any problems

@yash-atreya
Copy link
Member

@jpgonzalezra friendly bump here. Do you still have the bandwidth to complete this?

@zerosnacks
Copy link
Member

Would it make sense to generalize this to forge compiler --list?

@zerosnacks zerosnacks self-assigned this Oct 10, 2024
@zerosnacks zerosnacks changed the title feat(forge): add solc subcommand and utilities feat(forge): add solc subcommand and utilities (forge compiler --list) Oct 10, 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.

@zerosnacks could we add a basic test for it?

@zerosnacks
Copy link
Member

zerosnacks commented Oct 14, 2024

Pending next release of https://github.com/foundry-rs/compilers as it required minor changes upstream to test multi-compiler mode, specifically Vyper.

Command name updated to forge compiler resolve

Updated PR with compiler updates, added JSON compatibility, added verbose / non-verbose mode and added skip flag.

Apart from this blocker, ready for review

@zerosnacks zerosnacks added the T-blocked Type: blocked label Oct 14, 2024
crates/forge/bin/cmd/compiler.rs Outdated Show resolved Hide resolved
@DaniPopes DaniPopes changed the title feat(forge): add solc subcommand and utilities (forge compiler --list) feat(forge): add compiler subcommand Oct 16, 2024
.github/workflows/nextest.yml Outdated Show resolved Hide resolved
crates/forge/bin/cmd/compiler.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/compiler.rs Outdated Show resolved Hide resolved
crates/forge/tests/cli/compiler.rs Outdated Show resolved Hide resolved
crates/forge/tests/cli/compiler.rs Outdated Show resolved Hide resolved
@zerosnacks
Copy link
Member

cc @grandizzy would you mind doing a final review before merging?

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.

LGTM!

wonder if it would be useful to add evm version too at some point, smth like

image

@DaniPopes
Copy link
Member

Like the max supported version by compiler?

@zerosnacks
Copy link
Member

zerosnacks commented Oct 16, 2024

wonder if it would be useful to add evm version too at some point

like the max supported version by compiler?

Added a follow-up ticket here: #9125

@zerosnacks zerosnacks merged commit adb6aba into foundry-rs:master Oct 16, 2024
22 checks passed
@zerosnacks zerosnacks removed the T-blocked Type: blocked label Oct 16, 2024
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
* feat(forge): add solc subcommand and utilities

* style: improve formatting in solc.rs file

* fix: merge

* add json compatible output

* add basic tests

* add basic tests

* clean up

* finish tests

* add skip flag

* add vyper for unit tests

* move tests, pin compiler version, use forgetest!

* update CI test location for target Python / Vyper

* update foundry-compilers crate

* Update crates/forge/bin/cmd/compiler.rs

Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>

* `compiler` command is sync, remove conditions on CI for Vyper / Python installs

* is_jsonlines -> is_json

---------

Co-authored-by: zerosnacks <zerosnacks@protonmail.com>
Co-authored-by: zerosnacks <95942363+zerosnacks@users.noreply.github.com>
Co-authored-by: DaniPopes <57450786+DaniPopes@users.noreply.github.com>
@grandizzy grandizzy added the T-feature Type: feature label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

feat(forge): add command to check solc autodetected version
8 participants