-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
There was a problem hiding this 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
There was a problem hiding this comment.
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?
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!
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?
@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!
please have a second look after the changes.
@DaniPopes friendly ping as u may be opinionated on the approach
from the output project parser
c694ea6
to
118dcdb
Compare
Uh oh!
There was an error while loading. Please reload this page.
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
lint.rs
PR Checklist