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(solc): add dependency graph implementation #750

Merged
merged 27 commits into from
Jan 5, 2022

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Dec 29, 2021

Motivation

Close #739

Solution

Add a dependency graph implementation, see resolve.rs for docs.

Changes

  • add a Graph implementation that also resolves all imported dependencies that live outside of the project's configured source dir. This applies the configured remappings during lookup
  • Also add those dependencies to the cache file, so changes in a contract/library in the lib folder are detected and trigger a rebuild.
  • parallelize as much as possible (reading and parsing of source) files. parallelizing import resolving is not trivial because it can result in duplicated lookups (Lib1 imports Lib3, Lib2 imports Lib3)

ToDos

  • version checks, find minimum viable solc version applicable for all dependencies
  • integrate in Project::compile

Followup

  • add pretty printing like cargo tree

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

Copy link
Owner

@gakonst gakonst left a 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?

@mattsse
Copy link
Collaborator Author

mattsse commented Dec 30, 2021

@gakonst no idea why abigen tests are failing...
seems unrelated?

Copy link
Owner

@gakonst gakonst left a 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

ethers-solc/src/compile.rs Show resolved Hide resolved
ethers-solc/src/lib.rs Show resolved Hide resolved
ethers-solc/src/lib.rs Outdated Show resolved Hide resolved
ethers-solc/src/resolver.rs Show resolved Hide resolved
ethers-solc/src/resolver.rs Show resolved Hide resolved
ethers-solc/src/resolver.rs Show resolved Hide resolved
ethers-solc/src/resolver.rs Outdated Show resolved Hide resolved
ethers-solc/src/resolver.rs Outdated Show resolved Hide resolved
/// 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
Copy link
Owner

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?

Copy link
Collaborator Author

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.

ethers-solc/src/resolver.rs Show resolved Hide resolved
Copy link
Owner

@gakonst gakonst left a 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

@gakonst gakonst merged commit fc9f66c into gakonst:master Jan 5, 2022
@gakonst
Copy link
Owner

gakonst commented Jan 5, 2022

Let's see if this is the solution to all of our problems!

gakonst added a commit to foundry-rs/foundry that referenced this pull request Jan 5, 2022
gakonst/ethers-rs#750 introduces
a dependency graph and proper libs resolution, which should
ensure that library changes get detected in our caching logic
charisma98 added a commit to charisma98/foundry that referenced this pull request Mar 4, 2023
gakonst/ethers-rs#750 introduces
a dependency graph and proper libs resolution, which should
ensure that library changes get detected in our caching logic
0129general added a commit to 0129general/FoundryProject that referenced this pull request May 8, 2024
gakonst/ethers-rs#750 introduces
a dependency graph and proper libs resolution, which should
ensure that library changes get detected in our caching logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve library/import resolution
2 participants