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.

Sign up for GitHub

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

Jump to bottom

forge build with extra output files doesn't re-run for cached files #6241

Closed
2 tasks done
holic opened this issue Nov 7, 2023 · 4 comments · Fixed by foundry-rs/compilers#88
Closed
2 tasks done

forge build with extra output files doesn't re-run for cached files #6241

holic opened this issue Nov 7, 2023 · 4 comments · Fixed by foundry-rs/compilers#88
Assignees
@Evalir
Labels
T-bug Type: bug

Comments

@holic
Copy link

holic commented Nov 7, 2023 •
edited

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (059aa45 2023-11-07T08:20:43.711020000Z)

What command(s) is the bug in?

forge build

Operating System

macOS (Apple Silicon)

Describe the bug

I ran forge build on a project and then later ran forge build --extra-output-files abi but no .abi.json files were generated.

I was able to workaround this by forge clean then forge build --extra-output-files abi.
@holic holic added the T-bug Type: bug label Nov 7, 2023
@mattsse
Copy link
Member

mattsse commented Nov 7, 2023

I see, I think the issue here is that we don't invoke the ArtifactOutput handler if the artifacts are unchanged @Evalir

https://github.com/foundry-rs/compilers/blob/f00eef81a91a393bf92eb13d47a5221f5e50162f/src/artifact_output/configurable.rs#L235

which is only invoked when we write (new) artifacts

https://github.com/foundry-rs/compilers/blob/f00eef81a91a393bf92eb13d47a5221f5e50162f/src/compile/project.rs#L330-L336

I think we need some check here that applies the extra output files to the unchanged contracts

@Evalir Evalir self-assigned this Nov 7, 2023
@holic
Copy link
Author

holic commented Nov 8, 2023 •
edited

I am also seeing another potential issue that I am wondering if is related.

We have an interface IWorld that extends another interface IMoveSystem. If we update IMoveSystem and run forge build (on a warm cache with foundry.toml configured to output ABI files), then the IMoveSystem.abi.json is updated but not IWorld.abi.json.

Happy to open a new issue if this seems like a separate thing.

This was referenced Nov 8, 2023
Closed
Open
@dk1a dk1a mentioned this issue Nov 13, 2023
Merged
@github-actions github-actions bot mentioned this issue Dec 1, 2023
Merged
@klkvr klkvr mentioned this issue Mar 8, 2024
Merged
mattsse pushed a commit to foundry-rs/compilers that referenced this issue Mar 11, 2024
@klkvr
refactor: extra files logic (#88)
3ed2286 
Resolves foundry-rs/foundry#6241 

Removes `write_extra` logic from `ArtifactOutput` trait
Adds new methods:
- `handle_artifacts` - by default empty implementation which can be
overloaded to implement handling of compiled artifacts.
`ConfigurableArtifacts` implements it to write extra files
- `is_dirty` - method allowing `ArtifactOutput` implementations to
reject cached artifacts and enforce recompilation. Used by
`ConfigurableArtifacts` to reject artifacts which data is not enough to
write required extra output files.
- `handle_cached_artifacts` - same as `handle_artifacts` but for those
which were not recompiled. Used by `ConfigurableArtifacts` to write
extra files from cached artifacts.
@klkvr
Copy link
Member

klkvr commented Mar 11, 2024

@holic yes, it should fix this as well

@holic
Copy link
Author

holic commented Mar 11, 2024

incredible, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Evalir Evalir

Labels
T-bug Type: bug
Projects
No open projects
Foundry
Status: Todo
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: extra files logic klkvr/compilers
4 participants
@holic @mattsse @Evalir @klkvr