-
Notifications
You must be signed in to change notification settings - Fork 794
feat(solc): add dependency graph implementation #750
Conversation
75f9c70
to
79bc6b1
Compare
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.
seems like we can merge the parallel reading perf improvements independently of the dep graph code?
2ebcabe
to
97db900
Compare
@gakonst no idea why abigen tests are failing... |
01ad315
to
314a703
Compare
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.
This is really great. A few nits, but nothing blocking
/// where `C` is a library import, in which case we assign `C` only to the first input file. | ||
/// However, it's not required to include them in the solc `CompilerInput` as they would get | ||
/// picked up by solc otherwise, but we add them so we can create a corresponding | ||
/// cache entry for them as well. This can be optimized however |
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.
We're basically double compiling some files?
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.
I think that's possible in scenarios like:
A(<0.8.10) imports C (>0.4.0)
B(=0.8.11) imports C (>0.4.0)
where A and B are input contracts and C is lib import.
In which case we can't compile A and B together but both require C.
But we only have a single instance of C in our dependency graph.
So I think it's fine to only include it in 1 Solc -> Sources
set, like 0.8.11 -> (B,C)
, because solc will resolve C for A itself if not provided in the standard json input.
the reason why I decided against cloning here is, that these Sources
can cause some overhead when we process the cache entries.
I wanted to optimize that first.
Co-authored-by: Georgios Konstantopoulos <[email protected]>
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.
took another look and looks good, no additional comment to my previous ones
Let's see if this is the solution to all of our problems! |
gakonst/ethers-rs#750 introduces a dependency graph and proper libs resolution, which should ensure that library changes get detected in our caching logic
gakonst/ethers-rs#750 introduces a dependency graph and proper libs resolution, which should ensure that library changes get detected in our caching logic
gakonst/ethers-rs#750 introduces a dependency graph and proper libs resolution, which should ensure that library changes get detected in our caching logic
Motivation
Close #739
Solution
Add a dependency graph implementation, see resolve.rs for docs.
Changes
Graph
implementation that also resolves all imported dependencies that live outside of the project's configured source dir. This applies the configured remappings during lookuplib
folder are detected and trigger a rebuild.ToDos
Project::compile
Followup
cargo tree
PR Checklist