-
Notifications
You must be signed in to change notification settings - Fork 22
Description
Proposal
Problem statement
Being able to bless a test failure as expected and have the expected value updated (ie snapshot testing) is a big productivity boon. For example, the Cargo team just put in a lot of work to switch thousands of tests to snapshot testing (rust-lang/cargo#14039). Having that expected value inside the source code makes it easier to manage (rather than dealing with thousands of snapshots that may or may not be used anymore) and allows you to more easily inspect what condition is being tested. You can look at the reverse dependencies of snapbox, expect-test, and insta. The first two primarily work through inline snapshots. I can't say how often people use inline snapshots with insta
.
Similarly, it is can be helpful to be able to look up resources relative to the source file, rather than relative to the manifest root (via CARGO_MANIFEST_DIR
). These can include
- Snapshots (Cargo does this, see https://github.com/rust-lang/cargo/blob/286b6d0efa1db67c17c1fbed1385e9829f09c1e6/tests/testsuite/cargo_add/add_multiple/mod.rs#L36)
- Development-time assets (in theory, unsure if people want this)
Today this done by using file!
(which is unspecified) and assuming it is relative to the workspace root (which isn't always true), and somehow discovering the workspace root. The "somehow" is usually a mixture of
- Walking the path ancestors and assuming the next higher manifest is the workspace root (which it isn't always), https://github.com/assert-rs/snapbox/blob/60c7ce379e36be9d53238d1b936967fe6d28ca3e/crates/snapbox/src/macros.rs#L83-L87
- Dropping a
.cargo/config.toml
with an[env]
that is the workspace root (remember, as I said,file!
isn't always relative to workspace root), https://github.com/rust-lang/cargo/blob/286b6d0efa1db67c17c1fbed1385e9829f09c1e6/.cargo/config.toml#L7-L10
The Cargo team considered adding a CARGO_RUSTC_CURRENT_DIR
(rust-lang/cargo#12996) that would solve this problem but there were concerns over this existing purely for file!
and file!
being unspecified, causing us to push people towards relying on unspecified behavior.
Motivating examples or use cases
Normally, this will be abstracted away in a test like:
#[test]
fn test_no_infostring() {
snapbox::assert_data_eq!(
si!(r#"---
[dependencies]
time="0.1.25"
---
fn main() {}
"#),
str![[r#"
[[bin]]
name = "test-"
path = [..]
[dependencies]
time = "0.1.25"
[package]
autobenches = false
autobins = false
autoexamples = false
autolib = false
autotests = false
build = false
edition = "2021"
name = "test-"
[profile.release]
strip = true
[workspace]
"#]]
);
}
This relies on macros like
#[macro_export]
macro_rules! str {
[$data:literal] => { $crate::str![[$data]] };
[[$data:literal]] => {{
let position = $crate::data::Position {
file: $crate::utils::current_rs!(),
line: line!(),
column: column!(),
};
let inline = $crate::data::Inline {
position,
data: $data,
};
inline
}};
[] => { $crate::str![[""]] };
[[]] => { $crate::str![[""]] };
}
or
#[macro_export]
macro_rules! file {
[_] => {{
let path = $crate::data::generate_snapshot_path($crate::fn_path!(), None);
$crate::Data::read_from(&path, None)
}};
[_ : $type:ident] => {{
let format = $crate::data::DataFormat:: $type;
let path = $crate::data::generate_snapshot_path($crate::fn_path!(), Some(format));
$crate::Data::read_from(&path, Some($crate::data::DataFormat:: $type))
}};
[$path:literal] => {{
let mut path = $crate::utils::current_dir!();
path.push($path);
$crate::Data::read_from(&path, None)
}};
[$path:literal : $type:ident] => {{
let mut path = $crate::utils::current_dir!();
path.push($path);
$crate::Data::read_from(&path, Some($crate::data::DataFormat:: $type))
}};
}
Solution sketch
Like file!
provide a file_absolute!
that fills the role of the above $crate::utils::current_rs!()
(for which a $crate::utils::current_dir!()
can also be made).
Likewise, we should probably make the expectations for file!
more explicit
Open questions
- How will this interact with trim-paths? We were thinking warn or error
- How will this interact with the depinfo file? Unclear. If someone moves their directory, they will want it rebuilt. However, for CI caching where snapshots won't be used, they won't want it tracked.
- Should
file!
be "unspecified, for display purposes only" or be expected to be relative to a certain location (barring trim paths)
Alternatives
- Push back on the Cargo team for
CARGO_RUSTC_CURRENT_DIR
(feat: AddCARGO_RUSTC_CURRENT_DIR
(unstable) cargo#12996)
Links and related work
- Environment variable for Cargo Workspace cargo#3946 which mostly centers on this problem
What happens now?
This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.
Possible responses
The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):
- We think this problem seems worth solving, and the standard library might be the right place to solve it.
- We think that this probably doesn't belong in the standard library.
Second, if there's a concrete solution:
- We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
- We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
Activity
CARGO_RUSTC_CURRENT_DIR
rust-lang/cargo#13644m-ou-se commentedon Nov 18, 2024
We discussed this in last week's libs-api meeting. We're very hesitant to add absolute paths to the source files as a feature in the standard libarry.
As you mention above, it's an open question how this will work with the "trim paths" feature. It's quite possible that trim-paths will become a often used or even default behaviour in the future, so adding a macro that is incompatible with that (e.g. errors/warns), seems like a step in the wrong direction.
There seem to be other possible solutions that can solve your problem without relying on absolute paths being hardcoded in binaries, so we'd like to see more exporation in those directions. (E.g. getting the relevant information from cargo, as you mention above.)
epage commentedon Nov 18, 2024
We previously explored these options already before I came here, including having rust-lang/cargo#13644 for stabilizing it. A core problem is that
file!
is unspecified and changing of it for trim paths could also cause problems, see rust-lang/cargo#13644 (comment)If we could specify
file!
, including a way for callers to tell when its trimmed, then we might be able to bounce this back to Cargo. Is that something t-libs-api is open to?CC @ehuss
std::file!
macro rust-lang/rust#113044RalfJung commentedon Dec 1, 2024
We've had to add awkward work-arounds for this in rust-lang/rust#133533, which also turned out to be problematic so rust-lang/rust#132390 has to use a different work-around.
The current situation is that a libs-api member requested to not add a new cargo env var for this, and suggested
file_absolute!
. But then whenfile_absolute!
is proposed, libs-api says they'd rather see cargo provide this information (I guess via an env var). Would be good to get some clarity from t-libs-api on how this should be resolved, since it is a real issue that's causing problems even for rustc development itself. :)RalfJung commentedon Dec 1, 2024
@rustbot label +I-libs-api-nominated
joshtriplett commentedon Dec 3, 2024
(Speaking for myself, not the Cargo or libs-api team.)
@RalfJung Sympathy for the net outcome of mixed messaging here.
The previously proposed approach on the Cargo side seems like it would have involved Cargo attempting to expose the current directory that rustc used as a base for
file!()
, while keeping the current behavior of that base changing depending on how the compiler was invoked. I don't think that would have been the right approach.I don't think using absolute paths is the right approach either.
I would propose that we should have a "base" directory passed into rustc, so that cargo and other build tools can always consistently pass in a standard base (e.g. the crate root, even when in a workspace). And cases like Rust-for-Linux could (for instance) pass in the root of the kernel tree. We could then have a
file_relative!
or (naming is hard), as well as aninclude_str_relative!
.programmerjake commentedon Dec 3, 2024
well, if the path should be based on the passed-in base directory, how about
file_based!()
?dtolnay commentedon Dec 3, 2024
"Cargo attempting to expose the current directory that rustc used as a base for
file!()
" is opposite to how I think about that proposal, for two reasons:Rustc doesn't think in terms of choosing a base for
file!()
. It uses what paths Cargo tells it, already. If rustc is run asrustc path/to/file.rs
thenfile!()
is "path/to/file.rs". If rustc is run asrustc /absolute/file.rs
thenfile!()
is "/absolute/file.rs". Furthermore, if file containsinclude!($path2)
or#[path = $path2]
then inside that other file,file!()
is formed from well-specifiably joining first file's path with $path2 — there continues to be no notion of a base path having to be chosen for the joined result to be relative to. If $path2 is absolute, it makes sense thatfile!()
would end up being absolute.When using
mod
in straightforward ways, Cargo is the thing that is most in charge of what paths look like, through its choice of current directory for rustc (whether that's workspace root or manifest root, whether it varies between local dependencies and crates.io registry dependencies, whether it is absolute). After factoring in weirdinclude!
or#[path]
usage, the source code being compiled is the thing most in charge of what paths look like. Rustc is the thing least in charge — it is behaving as told by Cargo and by the code being compiled, in a well-specifiable (not necessarily currently well specified) way.Judging based on rust-lang/cargo#13644 (comment), that stabilization stalled out on the fact that
file!()
's behavior is not well specified. If we can fix that, I think the Cargo change is still my preferred approach than having the standard library/rustc provide a dedicated absolute path macro. (The other concern was trim-paths, which would just as much need to be resolved regardingfile_absolute!
, right?)It makes sense to me that it would be up to Cargo to supply context about Cargo's choice of rustc execution directory because the expectation that we can bake a source's absolute path into compiled code (the
snapbox::file!
use case) isn't a thing that generalize well from Cargo to other users of rustc:Bazel: rustc is always invoked inside a sandbox containing copied or symlinked sources, which is blown away as soon as rustc is done. So if you were to bake absolute paths to the sandboxed paths into an executable, then run it, it would observe that those paths never exist.
Buck: similar, but the paths would commonly be on a different machine, or even different OS, between the compilation and test execution.
Sccache: I am less familiar with this one but part of its point is sharing identical compilations across different projects, so putting absolute paths from one of those projects into the shared compiled result does not fly.
file_absolute!()
would be effectively nonsense in each of these systems (it might as well expand to "/asdf/jkl"). Whereasenv!("CARGO_RUSTC_CURRENT_DIR")
generalizes better. For example Buck could arrange to set that in a meaningful way for third-party code like it does for $OUT_DIR and $CARGO_MANIFEST_DIR and other idioms used in crates.io.epage commentedon Dec 4, 2024
As
file!
is a built-in, would specifying its behavior be an ACP, MCP, or both? I'd like to post something about that to have side-by-side with this as we work out what path to go down.17 remaining items