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
Align cache directory calculation so that the LMDB store location is uniformly configurable #10391
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# Copyright 2020 Pants project contributors (see CONTRIBUTORS.md). | ||
# Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
||
from pants.base.build_environment import get_pants_cachedir, get_pants_configdir | ||
from pants.engine.internals.native import Native | ||
|
||
|
||
def test_get_pants_cachedir() -> None: | ||
assert Native().default_cache_path() == get_pants_cachedir() | ||
|
||
|
||
def test_get_pants_configdir() -> None: | ||
assert Native().default_config_path() == get_pants_configdir() |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
use std::collections::BTreeMap; | ||
use std::path::PathBuf; | ||
|
||
use fs::default_cache_path; | ||
|
||
use crate::RelativePath; | ||
|
||
#[derive(Clone, Debug, Eq, PartialEq, Hash, PartialOrd, Ord)] | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's there already: see the edit I made in that file. |
||
pub fn default_path() -> PathBuf { | ||
default_cache_path().join("named_caches") | ||
} | ||
|
||
/// | ||
/// Returns symlinks to create for the given set of NamedCaches. | ||
/// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 intoBinaryUtil
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe I don't understand what you're suggesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is to stop implementing any custom logic in
base/build_environment.py
. Solely have this function returnNative().default_cache_path()
. Move this implementation tobinary_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, unfortunately that won't work: see my expanded response above.