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

fix: better tracking of cache entries #138

Merged
merged 3 commits into from
Jun 6, 2024
Merged

fix: better tracking of cache entries #138

merged 3 commits into from
Jun 6, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Jun 6, 2024

Currently, during sparse compilation (and probably in some other cases) we might end up in a situation when cache contains empty entries which can actually produce artifacts when compied.

For example, if A.sol imports B.sol and sparse filter tells us to only compile A.sol, then B.sol will appear in cache (because we have to track its source hash to invalidate A.sol's cache), however, we will erase its output selection when invoking solc, so it will not have any artifacts.

Thus, if compiler is invoked later without sparse filter, it will simply skip compiling B.sol because it will assume that it just does not contain any contract definitions:

compilers/src/cache.rs

Lines 658 to 663 in 4cf7843

// only check artifact's existence if the file generated artifacts.
// e.g. a solidity file consisting only of import statements (like interfaces that
// re-export) do not create artifacts
if entry.artifacts.is_empty() {
trace!("no artifacts");
return false;

Because of that, artifact for B.sol will be missing until the cache is cleared.

The issue is basically that we might have two situations:

  • Entry does not contain artifacts because it does not produce them when compiled
  • Entry does not contain artifacts because we've never tried to compile it

This PR adds seenByCompiler flag which is set to false by default for all newly created entries and switched when entries are compiled as dirty files (without pruned output selection).

@klkvr klkvr requested review from DaniPopes and Evalir as code owners June 6, 2024 00:42
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.

nice, this is indeed ambiguous without tracking this context manually.

this is technically a new cache format,

we could finally bump and rename the ETHERS_FORMAT_VERSION variable (separately)

)
.unwrap();

tmp.project_mut().sparse_output = Some(Box::new(|f: &Path| f.ends_with("A.sol")));
Copy link
Member

Choose a reason for hiding this comment

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

we could add a setter fn like with_sparse_output separately

src/cache.rs Show resolved Hide resolved
Comment on lines +390 to +396
/// Whether this file was compiled at least once.
///
/// If this is true and `artifacts` are empty, it means that given version of the file does
/// not produce any artifacts and it should not be compiled again.
///
/// If this is false, then artifacts are definitely empty and it should be compiled if we may
/// need artifacts.
Copy link
Member

Choose a reason for hiding this comment

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

this is helpful!

@klkvr klkvr merged commit f1cc978 into main Jun 6, 2024
14 checks passed
@klkvr klkvr deleted the klkvr/seen-by-compiler branch June 7, 2024 17:26
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.

2 participants