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

feat: adds compiler_cache_folder config var #1897

Merged
merged 19 commits into from
Feb 19, 2024

Conversation

mikeshultz
Copy link
Contributor

@mikeshultz mikeshultz commented Jan 31, 2024

What I did

Adds the cache_folder config var for ape_compile. Compiler plugins will leverage this for various temporary build-storage (mostly dependency sources).

Ref: #1335

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

@mikeshultz mikeshultz added category: feature New feature or request category: refactor Refactor of existing code or process labels Jan 31, 2024
@mikeshultz mikeshultz self-assigned this Jan 31, 2024
docs/userguides/compile.md Outdated Show resolved Hide resolved
src/ape/managers/project/manager.py Show resolved Hide resolved
src/ape/managers/project/manager.py Outdated Show resolved Hide resolved
docs/userguides/compile.md Show resolved Hide resolved
@antazoey
Copy link
Member

Related: ApeWorX/ape-solidity#62
When we utilize these changes in ape-solidity, can close that issue

antazoey
antazoey previously approved these changes Jan 31, 2024
Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

I tried using ManagerAccessMixin myself just to make sure and yeah you can't do it because it's loading still.

I can't really think of another way to do it either and this way if this problem again, we at least have a solution for it (rather than 1-off fix each time).

LGTM

src/ape/managers/config.py Show resolved Hide resolved
antazoey
antazoey previously approved these changes Feb 1, 2024
@mikeshultz
Copy link
Contributor Author

mikeshultz commented Feb 1, 2024

DO NOT MERGE this PR. Still working through some stuff and this may not be final.

@antazoey antazoey marked this pull request as draft February 2, 2024 20:14
@antazoey
Copy link
Member

antazoey commented Feb 2, 2024

DO NOT MERGE this PR. Still working through some stuff and this may not be final.

I converted to a Draft just to make it less likely to merge until it's ready

…y, tighten up cache_folder relative path handling, and prevent escape of project root
@mikeshultz mikeshultz marked this pull request as ready for review February 8, 2024 22:29
@mikeshultz mikeshultz requested a review from antazoey February 8, 2024 22:30
antazoey
antazoey previously approved these changes Feb 8, 2024
Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

LGTM - some nits, 1 question

My biggest takeaway is that we should probably change in 0.8 to using the base path as the project path. I know it is a big change; maybe we can offer another config so users can opt in the past approach when necessary.

if self._config_manager.contracts_folder not in self.cache_folder.parents:
return self._config_manager.PROJECT_FOLDER

# Otherwise, we're defaulting to contracts folder for backwards compatibility. Chaning this
Copy link
Member

Choose a reason for hiding this comment

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

question: Thoughts on for 0.8 using the project path as the base regardless, as a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no issues with making this change. We may want to document the old way somehow so users can reconstruct their project to match the old ways (or just instruct them to use an old version of ape and its deps, if possible). It's hard to get a sense how important repeatable builds are.

I also think it's worth discussing the idea in #1912. The functionality in it could be used for some kinds of build stages to assemble the compilation directory structure. Then it wouldn't matter where users put things. And we wouldn't need to really put a dependency cache anywhere semi-permanently. It's possible we could introduce this in a way to not be breaking.

src/ape_compile/__init__.py Outdated Show resolved Hide resolved
src/ape_compile/__init__.py Outdated Show resolved Hide resolved
src/ape_compile/__init__.py Outdated Show resolved Hide resolved
antazoey
antazoey previously approved these changes Feb 9, 2024
@mikeshultz mikeshultz merged commit 410fdb5 into ApeWorX:main Feb 19, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: feature New feature or request category: refactor Refactor of existing code or process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants