Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore(forge): filter out unsupported solidity versions [solar] #12065

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

Open
marcvernet31 wants to merge 8 commits into foundry-rs:master
base: master
Choose a base branch
Loading
from marcvernet31:master

Conversation

Copy link

@marcvernet31 marcvernet31 commented Oct 12, 2025
edited by 0xrusowsky
Loading

Motivation

As commented on this task #11668 (comment), Solar linter is not compatible with Solidity versions older that 0.8.0, which creates errors on execution.

closes #12008

Solution

  • Added a check that disables the linter when any of the compiled files has a version inferior to 0.8.0, displaying a warning message on console.
  • Added unit tests on lint.rs

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

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.

Looks good, pending @grandizzy

cc @0xrusowsky

Copy link
Collaborator

@grandizzy grandizzy left a comment
edited
Loading

Choose a reason for hiding this comment

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

makes sense, should we move the check in forge lint itself so will warn on both build and lint commands?

Copy link
Contributor

0xrusowsky commented Oct 13, 2025
edited
Loading

hey @marcvernet31, thanks for the PR!

just fyi i had this assigned to me, cause its part of a bigger solar-integration workstream. This isn't a linter-specific issue, as it impacts any component of the stack that use the solar compiler.

the proposed solution in the PR works, but the scope is too narrow. What we want is to filter incompatible sources in fn configure_pcx_from_compile_output() so that all commands that use the solar compiler benefit from it automatically.

the issue is that solar currently errors when you try to lower without sources, so we'd need to manually handle conditionals lowering everywhere, which is annoying... because of that, i opened paradigmxyz/solar#572 to make solar treat empty sources as a no-op.

imo we should wait for that to merge, then implement the proper fix at the higher level.
happy to collaborate on this if you want to wait for the solar change, or i can take it from here. Let me know what you prefer!

zerosnacks and marcvernet31 reacted with thumbs up emoji

@zerosnacks zerosnacks dismissed their stale review October 13, 2025 10:14

Per comments above

Copy link
Author

Hey @0xrusowsky, happy to try to finish this one once the solar fix is available. Just to clarify, we want to filter out all sources with incompatible Solidity version on configure_pcx_from_compile_output so that they are ignored right?

Copy link
Contributor

0xrusowsky commented Oct 13, 2025
edited
Loading

@marcvernet31 the solar PR was merged this afternoon, and i went ahead and implemented the fix at the higher level as previously mentioned 23ec372 (#12065)

this means the checks you added in build.rs with fn has_non_supported_solar_solidity_version() are now redundant, since the filtering happens upstream for all commands that use solar (not just linting).

the tests you wrote are great though! we just need to adapt them a bit and remove the redundant code from your initial commit.
imo we want to keep the forge build tests to assert that the cmd runs without errors nor warnings, and also run forge lint to assert that the "no sources" warning is logged.

could u take care of that, please? thanks a lot!

@0xrusowsky 0xrusowsky changed the title (削除) chore: disable linter for solidity versions not supported by solar (削除ここまで) (追記) chore(forge): filter out unsupported solidity versions [solar] (追記ここまで) Oct 14, 2025
Copy link
Contributor

0xrusowsky commented Oct 15, 2025
edited
Loading

please have a second look after the changes.

@DaniPopes friendly ping as u may be opinionated on the approach

@0xrusowsky 0xrusowsky marked this pull request as ready for review October 16, 2025 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@0xrusowsky 0xrusowsky Awaiting requested review from 0xrusowsky 0xrusowsky is a code owner

@zerosnacks zerosnacks Awaiting requested review from zerosnacks zerosnacks is a code owner

@grandizzy grandizzy Awaiting requested review from grandizzy grandizzy is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Projects

Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

chore(lint): disable linter when solidity versions are not supported by solar

AltStyle によって変換されたページ (->オリジナル) /