Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

fix: pass partial artifact cache to project compiler output #623

Merged
merged 4 commits into from Nov 26, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
such that the decimal places don't get truncated.
[597](https://github.com/gakonst/ethers-rs/pull/597)

## ethers-solc
- Return cached artifacts from project `compile` when the cache only contains some files

### Unreleased

### 0.6.0
Expand Down
36 changes: 27 additions & 9 deletions ethers-solc/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#![doc = include_str!("../README.md")]
#![doc = include_str ! ("../README.md")]

pub mod artifacts;

Expand All @@ -8,9 +8,11 @@ use std::collections::btree_map::Entry;
pub mod cache;

mod compile;

pub use compile::*;

mod config;

pub use config::{
AllowedLibPaths, Artifact, ArtifactOutput, MinimalCombinedArtifacts, ProjectPathsConfig,
SolcConfig,
Expand All @@ -22,6 +24,7 @@ use crate::{artifacts::Source, cache::SolFilesCache};

pub mod error;
pub mod utils;

use crate::artifacts::Sources;
use error::Result;
use std::{
Expand Down Expand Up @@ -237,22 +240,26 @@ impl<Artifacts: ArtifactOutput> Project<Artifacts> {
}

// If there's a cache set, filter to only re-compile the files which were changed
let sources = if self.cached && self.paths.cache.exists() {
let cache = SolFilesCache::read(&self.paths.cache)?;
let (sources, cached_artifacts) = if self.cached && self.paths.cache.exists() {
let mut cache = SolFilesCache::read(&self.paths.cache)?;
let changed_files = cache.get_changed_or_missing_artifacts_files::<Artifacts>(
sources,
Some(&self.solc_config),
&self.paths.artifacts,
);

cache.remove_missing_files();
This conversation was marked as resolved.
Show resolved Hide resolved
let cached_artifacts = cache
.read_artifacts::<Artifacts>(&self.paths.artifacts)
.unwrap_or_else(|_| BTreeMap::default());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unlikely to fail since we checked that those files exists, but could still fail due to some race conditions, like the files got wiped after we checked that they exist...

I feel we should propagate the error here because if that would fail, we end up with a successful output that lacks some artifacts.

Copy link
Author

Choose a reason for hiding this comment

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

The main failure case (outside of a race) for the fallback was when the artifacts folder is deleted, but the cache metadata still exists.
I'll change it to propagate the error, and fall back to empty cache if there is no artifacts folder. This way if some of the files have been wiped, it'll return the error. There's still a chance that the folder gets deleted at the same time still and then there'd be missing artifacts.

// if nothing changed and all artifacts still exist
if changed_files.is_empty() {
let artifacts = cache.read_artifacts::<Artifacts>(&self.paths.artifacts)?;
return Ok(ProjectCompileOutput::from_unchanged(artifacts))
return Ok(ProjectCompileOutput::from_unchanged(cached_artifacts))
}
changed_files
// There are changed files and maybe some cached files
(changed_files, cached_artifacts)
} else {
sources
(sources, BTreeMap::default())
};

// replace absolute path with source name to make solc happy
Expand Down Expand Up @@ -292,7 +299,11 @@ impl<Artifacts: ArtifactOutput> Project<Artifacts> {
if !self.no_artifacts {
Artifacts::on_output(&output, &self.paths)?;
}
Ok(ProjectCompileOutput::from_compiler_output(output, self.ignored_error_codes.clone()))
Ok(ProjectCompileOutput::from_compiler_output_and_cache(
output,
cached_artifacts,
self.ignored_error_codes.clone(),
))
}
}

Expand Down Expand Up @@ -501,6 +512,14 @@ impl<T: ArtifactOutput> ProjectCompileOutput<T> {
}
}

pub fn from_compiler_output_and_cache(
compiler_output: CompilerOutput,
cache: BTreeMap<PathBuf, T::Artifact>,
ignored_error_codes: Vec<u64>,
) -> Self {
Self { compiler_output: Some(compiler_output), artifacts: cache, ignored_error_codes }
}

/// Get the (merged) solc compiler output
/// ```no_run
/// use std::collections::BTreeMap;
Expand Down Expand Up @@ -637,7 +656,6 @@ impl<T: ArtifactOutput> fmt::Display for ProjectCompileOutput<T> {

#[cfg(test)]
mod tests {

#[test]
#[cfg(all(feature = "svm", feature = "async"))]
fn test_build_all_versions() {
Expand Down
5 changes: 5 additions & 0 deletions ethers-solc/test-data/cache-sample/NewContract.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.6.6;

contract NewContract {
}
69 changes: 68 additions & 1 deletion ethers-solc/tests/project.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
//! project tests

use ethers_solc::{cache::SOLIDITY_FILES_CACHE_FILENAME, Project, ProjectPathsConfig};
use std::path::PathBuf;
use std::{
io,
path::{Path, PathBuf},
};
use tempdir::TempDir;

#[test]
Expand Down Expand Up @@ -75,3 +78,67 @@ fn can_compile_dapp_sample() {
assert!(compiled.find("Dapp").is_some());
assert!(!compiled.is_unchanged());
}

#[test]
fn can_compile_dapp_sample_with_cache() {
let tmp_dir = TempDir::new("root").unwrap();
let root = tmp_dir.path();
let cache = root.join("cache").join(SOLIDITY_FILES_CACHE_FILENAME);
let artifacts = root.join("out");

let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
let orig_root = manifest_dir.join("test-data/dapp-sample");
let new_file = manifest_dir.join("test-data/cache-sample/NewContract.sol");
copy_dir_all(orig_root, &tmp_dir).unwrap();
let paths = ProjectPathsConfig::builder()
.cache(cache)
.sources(root.join("src"))
.artifacts(artifacts)
.lib(root.join("lib"))
.root(root)
.build()
.unwrap();

// first compile
let project = Project::builder().paths(paths).build().unwrap();
let compiled = project.compile().unwrap();
assert!(compiled.find("Dapp").is_some());
assert!(!compiled.has_compiler_errors());

// cache is used when nothing to compile
let compiled = project.compile().unwrap();
assert!(compiled.find("Dapp").is_some());
assert!(compiled.is_unchanged());

// deleted artifacts cause recompile even with cache
std::fs::remove_dir_all(&project.paths.artifacts).unwrap();
let compiled = project.compile().unwrap();
assert!(compiled.find("Dapp").is_some());
assert!(!compiled.is_unchanged());

// new file is compiled even with partial cache
std::fs::copy(new_file, root.join("src/NewContract.sol")).unwrap();
let compiled = project.compile().unwrap();
assert!(compiled.find("Dapp").is_some());
assert!(compiled.find("NewContract").is_some());
assert!(!compiled.is_unchanged());
Comment on lines +120 to +124
Copy link
Collaborator

Choose a reason for hiding this comment

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

This hits from_compiler_output_and_cache since we have a cached artifact but need to compile the NewContract.
lgtm


// deleted artifact is not taken from the cache
std::fs::remove_file(&project.paths.sources.join("Dapp.sol")).unwrap();
let compiled = project.compile().unwrap();
assert!(compiled.find("Dapp").is_none());
Comment on lines +127 to +129
Copy link
Collaborator

Choose a reason for hiding this comment

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

this verifies that we clear deleted files from the cache

}

fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> io::Result<()> {
std::fs::create_dir_all(&dst)?;
for entry in std::fs::read_dir(src)? {
let entry = entry?;
let ty = entry.file_type()?;
if ty.is_dir() {
copy_dir_all(entry.path(), dst.as_ref().join(entry.file_name()))?;
} else {
std::fs::copy(entry.path(), dst.as_ref().join(entry.file_name()))?;
}
}
Ok(())
}