-
Notifications
You must be signed in to change notification settings - Fork 615
chore(ci): add compilation cache of main branch #3198
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
chore(ci): add compilation cache of main branch #3198
Conversation
…ntelemetry-js-contrib into ci-add-compile-cache-main
| # so NX will only compile what has changed since then. | ||
| # Cache action is used because can be restores from dfferen workfow runs | ||
| - name: Compile Cache Lookup | ||
| uses: actions/cache/restore@v4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to consider not using the cache for pushes to "main"?
IIUC, the way it is now, that compile-cache-main will never start fresh. After a year, it will have a year of possible cruft in it, right? I realize that would mean doing a full clean npm run compile for pushes to main, but we'd still get the benefit of the cache for the more common updates to PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can consider it :)
the way it is now, that compile-cache-main will never start fresh.
you're right in the sense that the whole cache will never be refreshed at once unless a push changes all the packages at once. But even in that scenario we will refresh contents for each package keeping the cache structure. We could call it Theseus' cache.
I think the gains we get for PRs are more than enough and it is possible if we keep the cache for long time we may get som issues form nx if this changes the way it manages caching. Obviously after an update.
I'm going to add a new commit so:
- compiles everything in
mainand sets the cache - uses the cache for PRs only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit 71859dc
however in the last run I got the following mewssage
NX Unrecognized Cache Artifacts
Nx found unrecognized artifacts in the cache directory and will not be able to use them.
Nx can only restore artifacts it has metadata about.
Read about this warning and how to address it here: https://nx.dev/troubleshooting/unknown-local-cache
need to review the troubleshooting guide and test a bit more in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upload/download action works fine but not the cache. If we contain the cache management in this workflow I think we could tell nx to trust the cache with NX_REJECT_UNKNOWN_LOCAL_CACHE=0 (ref).
If we discard that option (which is fine since nx discourages to disable cache checking) I think the alternative is to explicitly tell nx when to compile and test auto-instrumentations-* packages which are the ones that forces to compile the rest of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skipping the check did not work in https://github.com/open-telemetry/opentelemetry-js-contrib/actions/runs/19134336471/job/54682522648?pr=3198 so I guess this is a dead end :(
message
NX_REJECT_UNKNOWN_LOCAL_CACHE=0 is not supported with the new database cache. Read more at https://nx.dev/deprecated/legacy-cache#nxrejectunknownlocalcache.
@trentm I'll close this PR and open a new one with the alternative mentioned in previous comment. Thanks for your review :)
Which problem is this PR solving?
Improve compilation times by adding a compilation cache which contains the artefacts form the last commit in main. Ref: #3136 (comment)
Short description of the changes