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

internal: Do not cache the config directory path #18441

Merged
merged 2 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions crates/load-cargo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ pub fn load_workspace(
.collect()
};

let project_folders = ProjectFolders::new(std::slice::from_ref(&ws), &[]);
let project_folders = ProjectFolders::new(std::slice::from_ref(&ws), &[], None);
loader.set_config(vfs::loader::Config {
load: project_folders.load,
watch: vec![],
Expand Down Expand Up @@ -153,7 +153,11 @@ pub struct ProjectFolders {
}

impl ProjectFolders {
pub fn new(workspaces: &[ProjectWorkspace], global_excludes: &[AbsPathBuf]) -> ProjectFolders {
pub fn new(
workspaces: &[ProjectWorkspace],
global_excludes: &[AbsPathBuf],
user_config_dir_path: Option<&AbsPath>,
) -> ProjectFolders {
let mut res = ProjectFolders::default();
let mut fsc = FileSetConfig::builder();
let mut local_filesets = vec![];
Expand Down Expand Up @@ -291,6 +295,22 @@ impl ProjectFolders {
}
}

if let Some(user_config_path) = user_config_dir_path {
let ratoml_path = {
let mut p = user_config_path.to_path_buf();
p.push("rust-analyzer.toml");
p
};

let file_set_roots = vec![VfsPath::from(ratoml_path.to_owned())];
let entry = vfs::loader::Entry::Files(vec![ratoml_path.to_owned()]);

res.watch.push(res.load.len());
res.load.push(entry);
local_filesets.push(fsc.len() as u64);
fsc.add_file_set(file_set_roots)
}

let fsc = fsc.build();
res.source_root_config = SourceRootConfig { fsc, local_filesets };

Expand Down
35 changes: 10 additions & 25 deletions crates/rust-analyzer/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,7 @@
//! Of particular interest is the `feature_flags` hash map: while other fields
//! configure the server itself, feature flags are passed into analysis, and
//! tweak things like automatic insertion of `()` in completions.
use std::{
env, fmt, iter,
ops::Not,
sync::{LazyLock, OnceLock},
};
use std::{env, fmt, iter, ops::Not, sync::OnceLock};

use cfg::{CfgAtom, CfgDiff};
use hir::Symbol;
Expand Down Expand Up @@ -804,25 +800,14 @@ impl std::ops::Deref for Config {
}

impl Config {
/// Path to the root configuration file. This can be seen as a generic way to define what would be `$XDG_CONFIG_HOME/rust-analyzer/rust-analyzer.toml` in Linux.
/// This path is equal to:
///
/// |Platform | Value | Example |
/// | ------- | ------------------------------------- | ---------------------------------------- |
/// | Linux | `$XDG_CONFIG_HOME` or `$HOME`/.config | /home/alice/.config |
/// | macOS | `$HOME`/Library/Application Support | /Users/Alice/Library/Application Support |
/// | Windows | `{FOLDERID_RoamingAppData}` | C:\Users\Alice\AppData\Roaming |
pub fn user_config_path() -> Option<&'static AbsPath> {
static USER_CONFIG_PATH: LazyLock<Option<AbsPathBuf>> = LazyLock::new(|| {
let user_config_path = if let Some(path) = env::var_os("__TEST_RA_USER_CONFIG_DIR") {
std::path::PathBuf::from(path)
} else {
dirs::config_dir()?.join("rust-analyzer")
}
.join("rust-analyzer.toml");
Some(AbsPathBuf::assert_utf8(user_config_path))
});
USER_CONFIG_PATH.as_deref()
/// Path to the user configuration dir. This can be seen as a generic way to define what would be `$XDG_CONFIG_HOME/rust-analyzer` in Linux.
pub fn user_config_dir_path() -> Option<AbsPathBuf> {
let user_config_path = if let Some(path) = env::var_os("__TEST_RA_USER_CONFIG_DIR") {
std::path::PathBuf::from(path)
} else {
dirs::config_dir()?.join("rust-analyzer")
};
Some(AbsPathBuf::assert_utf8(user_config_path))
}

pub fn same_source_root_parent_map(
Expand Down Expand Up @@ -1262,7 +1247,7 @@ pub struct NotificationsConfig {
pub cargo_toml_not_found: bool,
}

#[derive(Deserialize, Serialize, Debug, Clone)]
#[derive(Debug, Clone)]
pub enum RustfmtConfig {
Rustfmt { extra_args: Vec<String>, enable_range_formatting: bool },
CustomCommand { command: String, args: Vec<String> },
Expand Down
11 changes: 9 additions & 2 deletions crates/rust-analyzer/src/global_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,14 @@ impl GlobalState {
|| !self.config.same_source_root_parent_map(&self.local_roots_parent_map)
{
let config_change = {
let user_config_path = Config::user_config_path();
let user_config_path = (|| {
let mut p = Config::user_config_dir_path()?;
p.push("rust-analyzer.toml");
Some(p)
})();

let user_config_abs_path = user_config_path.as_deref();

let mut change = ConfigChange::default();
let db = self.analysis_host.raw_database();

Expand All @@ -411,7 +418,7 @@ impl GlobalState {
.collect_vec();

for (file_id, (_change_kind, vfs_path)) in modified_ratoml_files {
if vfs_path.as_path() == user_config_path {
if vfs_path.as_path() == user_config_abs_path {
change.change_user_config(Some(db.file_text(file_id)));
continue;
}
Expand Down
8 changes: 6 additions & 2 deletions crates/rust-analyzer/src/reload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,7 @@ impl GlobalState {
}

watchers.extend(
iter::once(Config::user_config_path())
iter::once(Config::user_config_dir_path().as_deref())
.chain(self.workspaces.iter().map(|ws| ws.manifest().map(ManifestPath::as_ref)))
.flatten()
.map(|glob_pattern| lsp_types::FileSystemWatcher {
Expand All @@ -613,7 +613,11 @@ impl GlobalState {
}

let files_config = self.config.files();
let project_folders = ProjectFolders::new(&self.workspaces, &files_config.exclude);
let project_folders = ProjectFolders::new(
&self.workspaces,
&files_config.exclude,
Config::user_config_dir_path().as_deref(),
);

if (self.proc_macro_clients.is_empty() || !same_workspaces)
&& self.config.expand_proc_macros()
Expand Down
19 changes: 6 additions & 13 deletions crates/rust-analyzer/tests/slow-tests/ratoml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ impl RatomlTest {
project = project.with_config(client_config);
}

let server = project.server().wait_until_workspace_is_loaded();
let server = project.server_with_lock(true).wait_until_workspace_is_loaded();

let mut case = Self { urls: vec![], server, tmp_path };
let urls = fixtures.iter().map(|fixture| case.fixture_path(fixture)).collect::<Vec<_>>();
Expand All @@ -72,7 +72,7 @@ impl RatomlTest {
let mut spl = spl.into_iter();
if let Some(first) = spl.next() {
if first == "$$CONFIG_DIR$$" {
path = Config::user_config_path().unwrap().to_path_buf().into();
path = Config::user_config_dir_path().unwrap().into();
} else {
path = path.join(first);
}
Expand Down Expand Up @@ -285,7 +285,6 @@ enum Value {
// }

#[test]
#[ignore = "the user config is currently not being watched on startup, fix this"]
fn ratoml_user_config_detected() {
if skip_slow_tests() {
return;
Expand All @@ -294,7 +293,7 @@ fn ratoml_user_config_detected() {
let server = RatomlTest::new(
vec![
r#"
//- /$$CONFIG_DIR$$/rust-analyzer/rust-analyzer.toml
//- /$$CONFIG_DIR$$/rust-analyzer.toml
assist.emitMustUse = true
"#,
r#"
Expand Down Expand Up @@ -322,7 +321,6 @@ enum Value {
}

#[test]
#[ignore = "the user config is currently not being watched on startup, fix this"]
fn ratoml_create_user_config() {
if skip_slow_tests() {
return;
Expand Down Expand Up @@ -353,10 +351,7 @@ enum Value {
1,
InternalTestingFetchConfigResponse::AssistEmitMustUse(false),
);
server.create(
"//- /$$CONFIG_DIR$$/rust-analyzer/rust-analyzer.toml",
RatomlTest::EMIT_MUST_USE.to_owned(),
);
server.create("//- /$$CONFIG_DIR$$/rust-analyzer.toml", RatomlTest::EMIT_MUST_USE.to_owned());
server.query(
InternalTestingFetchConfigOption::AssistEmitMustUse,
1,
Expand All @@ -365,7 +360,6 @@ enum Value {
}

#[test]
#[ignore = "the user config is currently not being watched on startup, fix this"]
fn ratoml_modify_user_config() {
if skip_slow_tests() {
return;
Expand All @@ -386,7 +380,7 @@ enum Value {
Text(String),
}"#,
r#"
//- /$$CONFIG_DIR$$/rust-analyzer/rust-analyzer.toml
//- /$$CONFIG_DIR$$/rust-analyzer.toml
assist.emitMustUse = true"#,
],
vec!["p1"],
Expand All @@ -407,7 +401,6 @@ assist.emitMustUse = true"#,
}

#[test]
#[ignore = "the user config is currently not being watched on startup, fix this"]
fn ratoml_delete_user_config() {
if skip_slow_tests() {
return;
Expand All @@ -428,7 +421,7 @@ enum Value {
Text(String),
}"#,
r#"
//- /$$CONFIG_DIR$$/rust-analyzer/rust-analyzer.toml
//- /$$CONFIG_DIR$$/rust-analyzer.toml
assist.emitMustUse = true"#,
],
vec!["p1"],
Expand Down
64 changes: 54 additions & 10 deletions crates/rust-analyzer/tests/slow-tests/support.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{
cell::{Cell, RefCell},
fs,
sync::Once,
env, fs,
sync::{Once, OnceLock},
time::Duration,
};

Expand Down Expand Up @@ -127,7 +127,53 @@ impl Project<'_> {
}

pub(crate) fn server(self) -> Server {
static CONFIG_DIR_LOCK: Mutex<()> = Mutex::new(());
Project::server_with_lock(self, false)
}

/// `prelock` : Forcefully acquire a lock that will maintain the path to the config dir throughout the whole test.
///
/// When testing we set the user config dir by setting an envvar `__TEST_RA_USER_CONFIG_DIR`.
/// This value must be maintained until the end of a test case. When tests run in parallel
/// this value may change thus making the tests flaky. As such, we use a `MutexGuard` that locks
/// the process until `Server` is dropped. To optimize parallelization we use a lock only when it is
/// needed, that is when a test uses config directory to do stuff. Our naive approach is to use a lock
/// if there is a path to config dir in the test fixture. However, in certain cases we create a
/// file in the config dir after server is run, something where our naive approach comes short.
/// Using a `prelock` allows us to force a lock when we know we need it.
pub(crate) fn server_with_lock(self, config_lock: bool) -> Server {
static CONFIG_DIR_LOCK: OnceLock<(Utf8PathBuf, Mutex<()>)> = OnceLock::new();

let config_dir_guard = if config_lock {
Some({
let (path, mutex) = CONFIG_DIR_LOCK.get_or_init(|| {
let value = TestDir::new().keep().path().to_owned();
env::set_var("__TEST_RA_USER_CONFIG_DIR", &value);
(value, Mutex::new(()))
});
#[allow(dyn_drop)]
(mutex.lock(), {
Box::new({
struct Dropper(Utf8PathBuf);
impl Drop for Dropper {
fn drop(&mut self) {
for entry in fs::read_dir(&self.0).unwrap() {
let path = entry.unwrap().path();
if path.is_file() {
fs::remove_file(path).unwrap();
} else if path.is_dir() {
fs::remove_dir_all(path).unwrap();
}
}
}
}
Dropper(path.clone())
}) as Box<dyn Drop>
})
})
} else {
None
};

let tmp_dir = self.tmp_dir.unwrap_or_else(|| {
if self.root_dir_contains_symlink {
TestDir::new_symlink()
Expand Down Expand Up @@ -160,13 +206,9 @@ impl Project<'_> {
assert!(mini_core.is_none());
assert!(toolchain.is_none());

let mut config_dir_guard = None;
for entry in fixture {
if let Some(pth) = entry.path.strip_prefix("/$$CONFIG_DIR$$") {
if config_dir_guard.is_none() {
config_dir_guard = Some(CONFIG_DIR_LOCK.lock());
}
let path = Config::user_config_path().unwrap().join(&pth['/'.len_utf8()..]);
let path = Config::user_config_dir_path().unwrap().join(&pth['/'.len_utf8()..]);
fs::create_dir_all(path.parent().unwrap()).unwrap();
fs::write(path.as_path(), entry.text.as_bytes()).unwrap();
} else {
Expand Down Expand Up @@ -269,12 +311,14 @@ pub(crate) struct Server {
client: Connection,
/// XXX: remove the tempdir last
dir: TestDir,
_config_dir_guard: Option<MutexGuard<'static, ()>>,
#[allow(dyn_drop)]
_config_dir_guard: Option<(MutexGuard<'static, ()>, Box<dyn Drop>)>,
}

impl Server {
#[allow(dyn_drop)]
fn new(
config_dir_guard: Option<MutexGuard<'static, ()>>,
config_dir_guard: Option<(MutexGuard<'static, ()>, Box<dyn Drop>)>,
dir: TestDir,
config: Config,
) -> Server {
Expand Down
Loading