Skip to content

fix: is_dirty to use additional_files #268

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

Merged
merged 2 commits into from
May 16, 2025

Conversation

grandizzy
Copy link
Contributor

@grandizzy grandizzy commented May 16, 2025

  • when checking for missing artifacts avoid checking files within artifacts / out directory
  • otherwise files like /home/george/work/foundry-issues/issue-preproc/out/Base.sol/CommonBase.json are checked and erroneously flag missing extra files / rebuild sources
  • will add test in foundry with such cases and assert the number of files built

@grandizzy grandizzy marked this pull request as ready for review May 16, 2025 13:48
@grandizzy grandizzy changed the title fix: do not check if artifact files are dirty fix: for extra files do not check if artifact files are dirty May 16, 2025
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.

I think this makes sense but the extra ! is a bit confusing, can we add a note here?

@@ -989,7 +989,9 @@ impl<T: ArtifactOutput<CompilerContract = C::CompilerContract>, C: Compiler>
for artifacts in self.cached_artifacts.values() {
for artifacts in artifacts.values() {
for artifact_file in artifacts {
if self.project.artifacts_handler().is_dirty(artifact_file).unwrap_or(true) {
if !artifact_file.file.starts_with(&self.project.paths.artifacts)
Copy link
Member

Choose a reason for hiding this comment

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

what's an example for an artifact_file that doesn't reside in the artifacts folder?

Copy link
Contributor Author

@grandizzy grandizzy May 16, 2025

Choose a reason for hiding this comment

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

thanks for pushing back, I got this totally wrong. the reason here is that we have additional_values and additional_files and we didn't account the additional files when checking if artifact dirty in cache, changed with abb1371

@grandizzy grandizzy marked this pull request as draft May 16, 2025 18:40
@grandizzy grandizzy marked this pull request as ready for review May 16, 2025 19:50
@grandizzy grandizzy requested a review from mattsse May 16, 2025 19:50
@grandizzy grandizzy changed the title fix: for extra files do not check if artifact files are dirty fix: is_dirty to use additional_files May 16, 2025
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.

ah this makes a lot of sense now

@mattsse mattsse merged commit 6eab181 into foundry-rs:main May 16, 2025
15 checks passed
@grandizzy grandizzy deleted the fix-missing-files-check branch May 16, 2025 20:55
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