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

Add profile specific caches #4522

Open
trianglesphere opened this issue Mar 10, 2023 · 5 comments
Open

Add profile specific caches #4522

trianglesphere opened this issue Mar 10, 2023 · 5 comments
Assignees
Labels
A-compiler Area: compiler A-config Area: config C-forge Command: forge Cmd-forge-coverage Command: forge coverage T-feature Type: feature T-perf Type: performance
Milestone

Comments

@trianglesphere
Copy link

Component

Forge

Describe the feature you would like

I'd like the build step in the coverage command to cache the contracts.
forge test does, but forge coverage does not. I understand there is a difference in how the contracts are built between forge test & forge coverage, so it's fine to have separate caches, but it'd be very helpful to have a cache here.

Additional context

No response

@trianglesphere trianglesphere added the T-feature Type: feature label Mar 10, 2023
@gakonst gakonst added this to Foundry Mar 10, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Mar 10, 2023
@mds1 mds1 added C-forge Command: forge Cmd-forge-coverage Command: forge coverage A-compiler Area: compiler labels Mar 13, 2023
@mds1
Copy link
Collaborator

mds1 commented Sep 20, 2023

You're correct that coverage builds with different settings (no optimizer, no via-ir). So perhaps a more generic feature request here is to automatically have a separate cache per-profile. This way you can maintain performance when switching between profiles without needing to manually set different out-dirs?

Two ideas on how to do this:

  1. Change the structure of the out/ dir to include the profile name, so instead of out/Counter.sol/Counter.json it would be out/{profileName}/Counter.sol/Counter.json. This would be a breaking change to scripts that parse the out dir, unless we also maintain the root out/ as containing the most recent build artifacts (i.e. the most recent build artifacts are duplicated/symlinked between out/ and out/{profileName}/
  2. Keep the structure of the out/ dir so it only holds the most recent build, but maintain caches in the default cache dir, e.g. ~/.foundry/cache/out/{dirIdentifier}/{profileName}

@mds1 mds1 added the T-perf Type: performance label Sep 20, 2023
@trianglesphere
Copy link
Author

Out, interesting, different out dirs could be set? I do see how the coverage step doesn't work with pre-building / using a cache.

TBH we've managed to work around this issue & while it seems nice, I don't see it as incredibly important to us.

@zerosnacks zerosnacks added the A-config Area: config label Jul 1, 2024
@zerosnacks zerosnacks changed the title forge coverage: Cache the contracts build step Add profile specific caches Jul 1, 2024
@zerosnacks
Copy link
Member

cc @klkvr besides this being a significant breaking change do you see any blockers on the technical side?

@klkvr
Copy link
Member

klkvr commented Jul 1, 2024

cc @klkvr besides this being a significant breaking change do you see any blockers on the technical side?

I don't think there are any blockers for separating cache/ and out/, though there will probably be blockers for making this to work for caching coverage compilation data. That's because we are relying on AST IDs in coverage which are getting messed up after incremental builds. This should be possible to handle (e.g. with approach similar to foundry-rs/compilers#140), but might become a blocker once we get to it

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@yash-atreya
Copy link
Member

cc @klkvr besides this being a significant breaking change do you see any blockers on the technical side?

I don't think there are any blockers for separating cache/ and out/, though there will probably be blockers for making this to work for caching coverage compilation data. That's because we are relying on AST IDs in coverage which are getting messed up after incremental builds. This should be possible to handle (e.g. with approach similar to foundry-rs/compilers#140), but might become a blocker once we get to it

This issue is resolved in #9366 along with enabling caching for coverage.

@yash-atreya yash-atreya self-assigned this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiler Area: compiler A-config Area: config C-forge Command: forge Cmd-forge-coverage Command: forge coverage T-feature Type: feature T-perf Type: performance
Projects
Status: Todo
Development

No branches or pull requests

5 participants