to_abs_ws_path goes too far up

[fasterthanlime]

Running RA tests from within a rust-lang/rust checkout fails, because expect-test climbs all the way up to rust/Cargo.toml instead of being happy with rust/src/tools/rust-analyzer/Cargo.toml, and then the paths are all wrong.

See rust-lang/rust#99444 (comment) for real-world details.

[fasterthanlime]

Relevant code:

expect-test/src/lib.rs

Lines 644 to 668 in 21f04f3

	fn to_abs_ws_path(path: &Path) -> PathBuf {
	if path.is_absolute() {
	return path.to_owned();
	}
	static WORKSPACE_ROOT: OnceCell<PathBuf> = OnceCell::new();
	WORKSPACE_ROOT
	.get_or_try_init(|| {
	let my_manifest = env::var("CARGO_MANIFEST_DIR")?;
	// Heuristic, see https://github.com/rust-lang/cargo/issues/3946
	let workspace_root = Path::new(&my_manifest)
	.ancestors()
	.filter(|it| it.join("Cargo.toml").exists())
	.last()
	.unwrap()
	.to_path_buf();
	Ok(workspace_root)
	})
	.unwrap_or_else(|_: env::VarError| {
	panic!("No CARGO_MANIFEST_DIR env var and the path is relative: {}", path.display())
	})
	.join(path)
	}

[fasterthanlime]

A clean fix for that would require rust-lang/cargo#3946 to be tackled, but the workaround suggested in rust-lang/cargo#3946 (comment) doesn't seem terrible to me:

$ pwd
/home/amos/bearcove/rust/src/tools/rust-analyzer/crates/ide/src/syntax_highlighting
$ cargo locate-project --workspace --message-format plain
/home/amos/bearcove/rust/src/tools/rust-analyzer/Cargo.toml

[Veykril]

Running cargo from a OnceCell initialization sounds a bit weird, though from the issue it does look like the only viable option... Given this is stuff that only runs in test environments it should be fine to swap to that nevertheless.

[flodiebold]

An alternative fix for our specific situation would be to simply set an environment variable (maybe named CARGO_WORKSPACE) from x.py and use that if present 🤔

[Veykril]

That would also be a nice alternative, assuming the people responsible for x.py are fine with adding that

[fasterthanlime]

As another alternative, this hack doesn't involve calling out to cargo: rust-lang/cargo#3946 (comment)

The idea would be to create a rust-analyzer/.cargo/config.toml file with this:

[env]
CARGO_WORKSPACE_DIR = { value = "", relative = true }

Then the env var would be set when running cargo test.

edit: This would still involve changes to expect-test, but not to x.py.

[flodiebold]

Yeah, I like that a bit more than changing x.py since it will also work when you just run cargo test inside the rust-analyzer submodule.

[fasterthanlime]

Yeah, I like that a bit more than changing x.py since it will also work when you just run cargo test inside the rust-analyzer submodule.

Sounds good, I'll open an expect-test PR.

[flodiebold]

... or we could merge #30, I guess? 🤔

[Veykril]

I don't think that PR fixes the issue, if I understand this right the CARGO_MANIFEST_DIR env var is set to a cargo toml that is a lot further up the file tree which manifests this issue here? Nevermind

[fasterthanlime]

... or we could merge #30, I guess? 🤔

Mhh I don't love the adhoc-toml-parsing there, it'll fail for (admittedly fringe) cases like this:

workspace = { members = [
	"xtask/",
	"lib/*",
	"crates/*",
], exclude = [
	"crates/proc-macro-test/imp",
] }

[fasterthanlime]

Mhh I don't love the adhoc-toml-parsing there

I'd like that PR more if it brought a real TOML parser with it but now that's overkill, and that energy is better spent pushing for the cargo bug to be fixed instead imho.

[flodiebold]

That's true. I'm fine with either (or both).

[Veykril]

I think I prefer the .cargo/config.toml approach, our heuristic should work fine for most usages of the lib, and where it breaks its a simple one line addition to the config.toml to get around the heuristic.

[fasterthanlime]

Follow-up RA PR:

• Upgrade to expect-test@1.4.0, add CARGO_WORKSPACE_DIR env var rust-lang/rust-analyzer#12817