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

Caching not working by default #7379

Closed
2 tasks done
ClaytonNorthey92 opened this issue Mar 12, 2024 · 9 comments · Fixed by #8164
Closed
2 tasks done

Caching not working by default #7379

ClaytonNorthey92 opened this issue Mar 12, 2024 · 9 comments · Fixed by #8164
Labels
T-bug Type: bug

Comments

@ClaytonNorthey92
Copy link

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 (7545c7a 2024-03-12T00:16:54.294788023Z)

What command(s) is the bug in?

No response

Operating System

Linux

Describe the bug

When running forge build twice in a row, with no changes, the previous build isn't cached. This affects other commands as well but is most apparent with build.

with an older release forge 0.2.0 (9f6bb3b 2024-03-11T00:16:01.716842530Z), notice that the cache is recognized and build is not repeated.

forge build
[⠃] Compiling...
No files changed, compilation skipped

but with forge 0.2.0 (7545c7a 2024-03-12T00:16:54.294788023Z) (the buggy version) the build cache is not recognized

forge build
[⠆] Compiling...
[⠰] Compiling 1 files with 0.5.17
[⠔] Compiling 324 files with 0.8.15
[⠑] Compiling 129 files with 0.8.19
[⠊] Solc 0.5.17 finished in 38.53ms
....

note that between running these two commands, I didn't modify any source files.

@ClaytonNorthey92 ClaytonNorthey92 added the T-bug Type: bug label Mar 12, 2024
@onbjerg
Copy link
Member

onbjerg commented Mar 12, 2024

cc @klkvr, related to #7358 maybe?

@ClaytonNorthey92
Copy link
Author

hey @onbjerg , I am realizing that I am requesting build_info in foundry.toml, so I guess this is techncially working as expected, but it threw me off 🤔

would it be appropriate to log a message that caching is disabled if build_info is enabled?

@klkvr
Copy link
Member

klkvr commented Mar 12, 2024

yeah, it could be, @ClaytonNorthey92 do you have build_info = true in your foundry.toml?

upd: front-ran :)

@ClaytonNorthey92
Copy link
Author

yeah, it could be, @ClaytonNorthey92 do you have build_info = true in your foundry.toml?

@klkvr yes I am just realizing that, mentioned here with a question #7379 (comment)

@klkvr
Copy link
Member

klkvr commented Mar 12, 2024

Yeah, I think such warning could make sense at least when build_info = true is appearing in foundry.toml

@ClaytonNorthey92
Copy link
Author

@klkvr also, how do you feel about changing the --build-info flag to a boolean, so you could:

forge build --build-info=0

and that would disable build_info, thus enabling caching

the reason that I ask is that I am running forge on a code base that I don't own, so I don't want to modify their foundry.toml, but we also don't use the build info files, so it would be convenient to override that via the cli. what do you think?

@sam-goldman
Copy link

Just want to point out that #7358 causes a significant slowdown in repos that set build_info=true in their foundry.toml. A full recompilation occurs every time the user runs forge test or forge script. This recompilation can take minutes if the project has a lot of contracts or uses the IR compilation pipeline. I don’t think Hardhat has this issue because they recompile only the necessary contracts and their parent contracts, which solves the original issue without force recompiling

@klkvr

@klkvr
Copy link
Member

klkvr commented Apr 7, 2024

@sam-goldman foundry would not recompile any of the files if them or their imports have not been changed. Thus, unchanged parent contract would not be recompiled. The #7358 was created with idea that expected behavior is to have all sources in build-info which is only possible when no cache is enabled.

I feel like we might want to use similar approach as hardhat here: hardhat stores build-info by default on each compiler run and keeps a mapping from contract artifact to build-info file, removing build-info file once no contract are pointing at it. That way, in the case from original issue, latest build info would still only include child contract, but it would be possible to match parent contract to build-info with data for it.

This would be a bit hardher to parse, but will be more precise and would not reauire disabling cache. Do you think such UX would make sense @sam-goldman?

cc @mattsse

@Magicking
Copy link

I started having issues related to cache as I needed both build info and cache files when #7358 hit; it silently disabled cache when build info was set to true (e.g.: it's a requirement for OZ Upgrades Foundry plugin)

Cache files - Seems to be needed to run a simple script otherwise, it throws

%> forge script SCRIPTNAME --broadcast --rpc-url http://localhost:8545 --unlocked --sender 0xDEADBEEF                                            
[⠊] Compiling...
[⠊] Compiling 105 files with 0.8.20
[⠃] Solc 0.8.20 finished in 26.92s
Compiler run successful!
Error: 
Could not open compiler cache

Context:
- "/home/magicking/PATH_TO/cache/solidity-files-cache.json": No such file or directory (os error 2)

And if the build info is disabled, the OZ Upgrades can't do all the validations.

Quite annoying, activating more build information shall not deactivate the cache; seems very counterintuitive!

klkvr added a commit to foundry-rs/compilers that referenced this issue Jun 11, 2024
ref foundry-rs/foundry#7379
ref foundry-rs/foundry#4981
ref foundry-rs/foundry#2704

The way Solidity assigns `source_id`s is simply by order in which
sources are passed in compiler input. Thus, if we have two sources
`A.sol` and `B.sol`, then on the first (non-cached) compiler run,
`A.sol` will get assigned ID 1 and `B.sol` will have ID 2.

Then, if we change `B.sol` slightly and recompile, it will be the only
source in the input, so it will have ID 1.

The same ID collisions are more often appearing on multi-version and
multi-compiler builds. After many cached runs such discrepancies result
in debugger basically displaying random sources.

This PR adds a way to link an artifact to the build info for input that
produced it, thus allowing us to track correct `source_id -> source`
mapping for cached artifacts.

I've added `BuildContext` which is the foundry context we are tracking
for each compiler invocation. It is getting inlined into `BuildInfo`,
and read along with cached artifacts. `BuildContext`s are indexed by the
same IDs `BuildInfo`s are, and each `ArtifactId` now has a reference to
`build_id` which produced it.

Build info now produced on every compiler run, however, it does not
include complete input and output unless `project.build_info` is true,
to avoid overhead while keeping our internal logic working correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants