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

Set and cache dependency directory for Java build-mode: none #2802

Merged
merged 4 commits into from
Mar 18, 2025

Conversation

mbg
Copy link
Member

@mbg mbg commented Mar 10, 2025

This PR sets an extractor option for the Java build-mode: none extractor (if Java is being analysed and build-mode: none is set) that redirects the dependency jars to a location where it can be included in a cache.

Context

Up until now, the build-mode: none extractor downloaded dependency jars into a scratch directory in the database directory. This gets erased before we get to storing a cache and also is potentially problematic for inclusion in a cache, since caches must be restored to the same directories from which they were created.

We added an extractor option to override the path to the directory where the build-mode: none extractor stores the jar files.

Open questions

I didn't see anything overly similar where we set specific options for other extractors and we also don't store much data outside of the database folder.

  • Do we see any problems with storing the dependencies in temp?
  • Should the extractor option be set elsewhere?

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

Sorry, something went wrong.

@mbg mbg self-assigned this Mar 12, 2025
@mbg mbg force-pushed the mbg/dependency-caching/java-buildless branch from 6c7fbe7 to f8367fb Compare March 13, 2025 11:39
@mbg mbg marked this pull request as ready for review March 13, 2025 11:45
@Copilot Copilot bot review requested due to automatic review settings March 13, 2025 11:45
@mbg mbg requested a review from a team as a code owner March 13, 2025 11:45

Choose a reason for hiding this comment

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

Pull Request Overview

This PR sets a stable dependency directory for the Java build-mode: none extractor so that dependency jars can be reliably cached.

  • Adds an environment variable setting to override the default dependency directory in both JavaScript and TypeScript extraction flows.
  • Exports and utilizes a helper function to determine the temporary dependency directory across multiple modules.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

File Description
lib/analyze.js Imports dependency-caching and sets the env variable for Java caching.
src/analyze.ts Mirrors changes in lib/analyze.js for TypeScript extraction.
src/dependency-caching.ts Exports getJavaTempDependencyDir with accompanying documentation.
lib/dependency-caching.js Exports getJavaTempDependencyDir and integrates the new caching path.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@aeisenberg
Copy link
Contributor

Do we see any problems with storing the dependencies in temp?

On a windows self-hosted runner, there's a chance that continually caching dependencies in the temp folder will cause disk space to fill up (unless we explicitly clean after the run).

In the post-init step, can you add a bit to clean up the directory?

@mbg
Copy link
Member Author

mbg commented Mar 17, 2025

Thanks for highlighting that! I have added some clean-up logic, though to the post step of the analyze Action, rather than init, since the analyze Action is the one that sets the path and also performs the caching, so the dependencies shouldn't be needed afterwards. Happy to move it to the post step of the init Action though if we can think of a reason why it would be better off there.

I also initially thought that this probably wouldn't be much of an issue, because we only enable dependency caching on GH-hosted runners in Default Setup, but we don't currently check whether caching is enabled before setting this extractor option. We could consider adding in that check so that the behaviour is unchanged if dependency caching isn't enabled. What do you think?

@aeisenberg
Copy link
Contributor

Thanks for the change.

Happy to move it to the post step of the init Action though if we can think of a reason why it would be better off there.

I think the analyze step makes more sense.

I also initially thought that this probably wouldn't be much of an issue, because we only enable dependency caching on GH-hosted runners in Default Setup, but we don't currently check whether caching is enabled before setting this extractor option. We could consider adding in that check so that the behaviour is unchanged if dependency caching isn't enabled. What do you think?

I'm not too fussed either way, as long as runs on self-hosted runners don't fill up their temp folders (which both approaches seem to do).

@mbg mbg merged commit 55f0237 into main Mar 18, 2025
270 checks passed
@mbg mbg deleted the mbg/dependency-caching/java-buildless branch March 18, 2025 10:28
@github-actions github-actions bot mentioned this pull request Mar 19, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants