SCons: Improve cache purging logic

[Repiteo]

Been banging my head against a wall trying to figure out the nuance of SCons' caching systems, and came out with two key takeaways:

• The current methods.py implementation is very messy and riddled with legacy code
• SCons has been caching files that're labeled as NoCache, particularly text files

Thankfully, these go hand-in-hand, as a refactor of the former allowed me to add a dedicated pass for the latter. After compression, it ends up being a ~5MB reduction in cache size per build. Beyond that, there was general simplification of the code & making it more readable, as well as removing a redundant key check in the cache-restore action. I wholly believe that this can be further improved, particularly if binary files are being erroneously cached as well, but anything beyond this will require a deeper dive

[Repiteo]

Opted to expand the scope of this PR to SCons caching in general, so the following changes were made:

• cache was added as a bool argument to enable the cache from CL
• cache_path argument added, defaults to ".scons_cache". Deprecates SCONS_CACHE environment variable
• cache_limit argument added, defaults to "0" (now means unlimited). Handles inputs as GiB, rather than MiB. Deprecates SCONS_CACHE_LIMIT environment variable
• .gitignore updated to flatly ignore any .*_cache/ folders, as that's the python module cache syntax & also includes our default cache path
• GHA updated to utilize CLI cache arguments

[AThousandShips]

For adding command line arguments, see also:

• [Buildsystem] Improve cache configuration usability #97186

[Repiteo]

I like your implementation of enabling the cache based on supplying a path, so I integrated that instead. Still left the .gitignore change in spite of that, as it's a nice QOL for anyone wanting to use the GHA path format

[akien-mga]

Life is too short to try to understand SCons caching code :P

If it works, I'm happy with it.

[AThousandShips]

Will give this a look tomorrow

[AThousandShips]

Will go over the cache detail code tomorrow but it works correctly for me and looks good at a glance save for these changes

[AThousandShips]

For some reason the caching is broken on Windows in the runners, it uses D:agodotgodot/.scons_cache/, probably because of broken escapes for paths there, something with the cast operation I suspect, didn't notice it at first testing it locally because I already used forward slashes when running things and a relative path

[AThousandShips]

See above

[Repiteo]

I'd be surprised if it was the cast option, but I'll do a version without to see if anything changes

[AThousandShips]

You're right it's not that you need quotes around the argument for Windows just like angle_libs

Looks resolved, will review in depth tomorrow

[AThousandShips]

Looking at CI something odd seems to be going on with the purging, it seems to purge files even if it doesn't seem to have passed the cache limit, will look closer and do some testing to see what is going on there

My bad those were the non-valid cached files mentioned in the original, so that should be safe, will just run some tests to ensure it doesn't break compilation stability

Might want to add a message to the purge of invalidly cached files separately to make it clear that the cache isn't broken when purging while under the limit but just purges files that shouldn't be cached in the first place

Maybe something like:

diff --git a/methods.py b/methods.py
index 33fcca5021..73b49775df 100644
--- a/methods.py
+++ b/methods.py
@@ -858,6 +858,7 @@ def clean_cache(cache_path: str, cache_limit: int, verbose: bool):

     # Remove all text files, store binary files in list of (filename, size, atime).
     purge = []
+    texts = []
     stats = []
     for file in files:
         # Failing a utf-8 decode is the easiest way to determine if a file is binary.
@@ -869,7 +870,18 @@ def clean_cache(cache_path: str, cache_limit: int, verbose: bool):
         except OSError:
             print_error(f'Failed to access cache file "{file}"; skipping.')
         else:
-            purge.append(file)
+            texts.append(file)
+
+    if texts:
+        count = len(texts)
+        for file in texts:
+            try:
+                os.remove(file)
+            except OSError:
+                print_error(f'Failed to remove cache file "{file}"; skipping.')
+                count -= 1
+        if verbose:
+            print("Purging %d text %s from cache..." % (count, "files" if count > 1 else "file"))

     if cache_limit:
         # Sort by most recent access (most sensible to keep) first. Search for the first entry where

Edit: tested that and it works, I think it helps clarify what's going on

Will go through some more of the code but seems to work great, will make a final review today if I can

[AThousandShips]

Apart from my suggestion above this works well and looks correct!

Concerns about the deleted text files causing significant reduction in rebuild time

[AThousandShips]

(My bad pushed to the wrong branch, and github is slow in updating the changes for some reason)

[AThousandShips]

Working through re-compilation tests but initial results are (master and this PR):
Android editor: 58s -> 8m 48s
Android arm32: 36s -> 6m 55s
Android arm64: 39s -> 7m 35s

IOS: 51s -> 7m 06s

Linux t/mono: 39s -> 11m 10s

Will test by removing the text purging and see if that is specifically what breaks this or not

Edit: Did some testing and doesn't seem to be due to the purging of text files, will have to deeper, but something is broken with caching it seems on plain re-runs

[AThousandShips]

I think I've got it, it seems to be due to the moving of the cache initialization to the end of SCons, causing some important configuration files to not be cached, will test a solution

Edit: Tested and moving the initialization of the cache up to where it used to be (where CacheDir was assigned) solves the performance loss, will test some further

Changes required:

diff --git a/SConstruct b/SConstruct
index f7fe98cb5c..8da4246fbd 100644
--- a/SConstruct
+++ b/SConstruct
@@ -1043,6 +1043,8 @@ GLSL_BUILDERS = {
 }
 env.Append(BUILDERS=GLSL_BUILDERS)

+methods.prepare_cache(env)
+
 if env["compiledb"]:
     env.Tool("compilation_db")
     env.Alias("compiledb", env.CompilationDatabase())
@@ -1095,7 +1097,6 @@ if "check_c_headers" in env:


 methods.show_progress(env)
-methods.prepare_cache(env)
 # TODO: replace this with `env.Dump(format="json")`
 # once we start requiring SCons 4.0 as min version.
 methods.dump(env)

Unsure if there's any issues in running some of that code earlier but in my testing this works and solves the problem (will confirm with the text purging restored as well)

[AThousandShips]

See above again

[AThousandShips]

There, lots of back and forth but this would be my final suggestion for changes, this retains the purging of text files, prints them more clearly, and fixes rebuild times:

diff --git a/SConstruct b/SConstruct
index f7fe98cb5c..8da4246fbd 100644
--- a/SConstruct
+++ b/SConstruct
@@ -1043,6 +1043,8 @@ GLSL_BUILDERS = {
 }
 env.Append(BUILDERS=GLSL_BUILDERS)

+methods.prepare_cache(env)
+
 if env["compiledb"]:
     env.Tool("compilation_db")
     env.Alias("compiledb", env.CompilationDatabase())
@@ -1095,7 +1097,6 @@ if "check_c_headers" in env:


 methods.show_progress(env)
-methods.prepare_cache(env)
 # TODO: replace this with `env.Dump(format="json")`
 # once we start requiring SCons 4.0 as min version.
 methods.dump(env)
diff --git a/methods.py b/methods.py
index 33fcca5021..73b49775df 100644
--- a/methods.py
+++ b/methods.py
@@ -858,6 +858,7 @@ def clean_cache(cache_path: str, cache_limit: int, verbose: bool):

     # Remove all text files, store binary files in list of (filename, size, atime).
     purge = []
+    texts = []
     stats = []
     for file in files:
         # Failing a utf-8 decode is the easiest way to determine if a file is binary.
@@ -869,7 +870,18 @@ def clean_cache(cache_path: str, cache_limit: int, verbose: bool):
         except OSError:
             print_error(f'Failed to access cache file "{file}"; skipping.')
         else:
-            purge.append(file)
+            texts.append(file)
+
+    if texts:
+        count = len(texts)
+        for file in texts:
+            try:
+                os.remove(file)
+            except OSError:
+                print_error(f'Failed to remove cache file "{file}"; skipping.')
+                count -= 1
+        if verbose:
+            print("Purging %d text %s from cache..." % (count, "files" if count > 1 else "file"))

     if cache_limit:
         # Sort by most recent access (most sensible to keep) first. Search for the first entry where

Needs some testing to confirm the placement of the cache initialization though, might need to be split into one part setting CacheDir and one doing the rest

[Repiteo]

Hmm, if putting the cache handling later caused longer build times, would placing it even earlier improve it? Might give that a spin

[AThousandShips]

Everything looks good now and my version of the same solution works well

[dustdfg]

So now enabling cache is just about setting these two options?

[Repiteo]

If you just wanna enable it, all you need is the path option. If you don't care about size you can leave the limit option alone