JS: Change how TRAP cache is configured

[edoardopirovano]

This PR refactors how we configure the TRAP cache for the JavaScript extractor. Previously, we had two different mechanisms:

1. In the autobuilder, via the LGTM_TRAP_CACHE environment variables.
2. In a direct invocation of the extractor, via the --trap-cache flags.

As far as I can tell, neither of these mechanisms is actually in use at the moment. So, before we create new uses for it, let's take the opportunity to refactor both those code paths to flow through the same place, and be configured by the new extractor options functionality documented here, which is how the Action's upcoming TRAP caching feature will configure the cache.

As part of this, I've also added a new option that configures whether the cache should be written to. This will be needed in the new usage of this cache in the Action, where we will distinguish runs on main (which will update the cache) and runs on PRs (which may read a cache, but will never bother writing to it since it will be discarded).

[esbena]

(which may read a cache, but will never bother writing to it since it will be discarded).

It sounds like we deliberately prevent ourselves from using "our own" cache on a PR where codeql is run multiple times. Is that cache effect considered insignificant compared to the cache on main?

[edoardopirovano]

It sounds like we deliberately prevent ourselves from using "our own" cache on a PR where codeql is run multiple times. Is that cache effect considered insignificant compared to the cache on main?

In this initial prototype, the plan is to only keep a cache from the latest run on main and then always use that on PRs. We could indeed almost certainly get better performance by keeping lots of caches around and then using one for the "most similar" (for some metric) commit to the one we are analyzing. That is considered out of scope for now, both to keep the complexity of this prototype down, and because for now we are relying on the GitHub Actions cache as our storage backend, and this has a fairly limited capacity (10GB per repo) that we are sharing with other potential CI uses by our users, which we don't want to disrupt by claiming too much space for our purposes.

[esbena]

Great, thanks for confirming and explaining.

The implementation LGTM, but I'll defer to @asgerf for the final decision.

Long shot: perhaps @max-schaefer has some LGTM_TRAP_CACHE usage that we do not know about.

[max-schaefer]

Long shot: perhaps @max-schaefer has some LGTM_TRAP_CACHE usage that we do not know about.

I do not, but many thanks for considering it!

[edoardopirovano]

github/codeql-action#1172 is the PR on the Action that will start hitting this code path. It's gated behind a feature flag.

[asgerf]

LGTM, just a weird indentation issue

[asgerf]

Suggested change

	*
	* @return a TRAP cache
	*/
	public static ITrapCache fromExtractorOptions() {
	*
	* @return a TRAP cache
	*/
	public static ITrapCache fromExtractorOptions() {

Not sure what happened to the indentation there

[edoardopirovano]

Good spot, fixed.