Align cache directory calculation so that the LMDB store location is uniformly configurable #10391
Conversation
…ive` and `build_environment.py`.
| @@ -34,7 +34,9 @@ def get_buildroot() -> str: | |||
|
|
|||
| def get_pants_cachedir() -> str: | |||
There was a problem hiding this comment.
If this is only being used in BinaryUtil now, it would be good to move there. Ditto on pants_configdir. It reduces the risk of people accidentally using this.
Or, maybe even better, keep these, but have them return Native().default_cache_path(). Move this non-Rust version into BinaryUtil.
There was a problem hiding this comment.
It goes much deeper than that, unfortunately... follow the references from the method I linked in the description:
pants/src/python/pants/binaries/binary_util.py
Lines 573 to 574 in 0ad3a56
...and you'll find that it uses the OptionsBootstrapper and a bunch of other APIs, which have lots of dependencies throughout the system.
There was a problem hiding this comment.
But maybe I don't understand what you're suggesting.
There was a problem hiding this comment.
My suggestion is to stop implementing any custom logic in base/build_environment.py. Solely have this function return Native().default_cache_path(). Move this implementation to binary_util.py.
But, actually, don't spend time on this. It's not that big of a deal because you have the new unit tests. It's not worth spending more time on this, imo.
There was a problem hiding this comment.
Move this implementation to binary_util.py.
Yea, unfortunately that won't work: see my expanded response above.
| @@ -54,6 +56,11 @@ impl NamedCaches { | |||
| NamedCaches { local_base } | |||
| } | |||
|
|
|||
| // This default suffix is also hard-coded into the Python options code in global_options.py | |||
There was a problem hiding this comment.
Maybe add to global_options.py a note about the duplication.
There was a problem hiding this comment.
It's there already: see the edit I made in that file.
Problem
Because we have tried to keep it in sync across multiple consumers, the
lmdb_storelocation does not obey theXDG_CACHE_HOMEsetting.Solution
Align the calculation of the default location for the
lmdb_storeacross rust and python. Unfortunately, this currently requires duplicating the relevant methods, in order to dodge a cycle betweenNativeandbuild_environment.py, which is used transitively byBinaryUtil(viaOptionsBootstrapperand options parsing) in this entrypoint to download tools used during rust bootstrap.Result
The
XDG_CACHE_HOMEsetting is respected for all cache-related options.