Skip to content

Conversation

@david-luna
Copy link
Contributor

@david-luna david-luna commented Oct 29, 2025

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

  • add a global compilation cache with the cache action https://github.com/actions/cache so its available for all workflow runs
  • compilation step restores that cache and uses it for compilation.
  • if build happens in main branch (push event) workflow updates the global cache
  • on PRs workflow uploads specific cache for test jobs as it was done before

@david-luna david-luna requested a review from a team as a code owner October 29, 2025 18:42
# 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
Copy link
Contributor

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.

Copy link
Contributor Author

@david-luna david-luna Nov 5, 2025

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 main and sets the cache
  • uses the cache for PRs only

Copy link
Contributor Author

@david-luna david-luna Nov 6, 2025

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@david-luna david-luna Nov 6, 2025

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 :)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants