Stream: t-compiler/major changes

Topic: Moving `#[cfg(test)]` modules into a sepa… compiler-team#344


triagebot (Aug 13 2020 at 11:43, on Zulip):

A new proposal has been announced: #344. It will be announced at the next meeting to try and draw attention to it, but usually MCPs are not discussed during triage meetings. If you think this would benefit from discussion amongst the team, consider proposing a design meeting.

lzutao (Aug 13 2020 at 11:47, on Zulip):

cc @Ashley Mannix as reviewer of https://github.com/rust-lang/rust/pull/75108
cc @simulacrum do you want to be a mentor of this proposal?

simulacrum (Aug 13 2020 at 11:48, on Zulip):

IMO we don't need an MCP for this, we can just do it. But I'm not opposed to reviewing PRs finishing this (IIRC @Vadim Petrochenkov has done this for several compiler crates already).

Jake Goulding (Aug 13 2020 at 12:09, on Zulip):

One point I’d have here is that it be nice to make the current scheme of the cfg in the file better as a lot of existing code does it outside of the stdlib.

Wesley Wiser (Aug 13 2020 at 13:38, on Zulip):

I feel like I'm missing something, you still have to recompile std with --cfg test to have the tests included.

simulacrum (Aug 13 2020 at 13:47, on Zulip):

iirc the compiler doesn't emit the test.rs into dep-info for the non-test compilation, which means that you don't recompile the library twice with this approach, but I forget how exactly it works

lzutao (Aug 13 2020 at 14:29, on Zulip):

simulacrum said:

IMO we don't need an MCP for this, we can just do it.

I would like to have a consensus about moving cfg(test) tests to files.
So we don't have to discuss whether to move tests in each PR.

simulacrum (Aug 13 2020 at 14:30, on Zulip):

hm well I don't think there was any opposition

simulacrum (Aug 13 2020 at 14:30, on Zulip):

but mcp doesn't seem bad I guess to re-affirm that

lzutao (Aug 13 2020 at 14:37, on Zulip):

I mean moving tests is kind of big changes (in diff) and has disadvantage in git blame.
So reviewer may feel hesitant to accept PR like that.

simulacrum (Aug 13 2020 at 14:38, on Zulip):

I'm not too worried about large PRs that are just moving things around

triagebot (Aug 13 2020 at 18:08, on Zulip):

@T-compiler: Proposal #344 has been seconded, and will be approved in 10 days if no objections are raised.

Vadim Petrochenkov (Aug 13 2020 at 18:09, on Zulip):

This is already done for all crates except for std, which is whitelisted in tidy.

Vadim Petrochenkov (Aug 13 2020 at 18:12, on Zulip):

I didn't migrate std in #63207 because it contained a lot of tests, needed platform-specific testing, and was maintained by the library team.

Vadim Petrochenkov (Aug 13 2020 at 18:13, on Zulip):

It would be great if someone could finish this work now.

Vadim Petrochenkov (Aug 13 2020 at 18:13, on Zulip):

Relevant tidy check - https://github.com/rust-lang/rust/blob/master/src/tools/tidy/src/unit_tests.rs

Jake Goulding (Aug 13 2020 at 20:22, on Zulip):

Why shouldn’t every Rust crate ever follow suit here? What makes the standard library different?

simulacrum (Aug 13 2020 at 20:22, on Zulip):

AFAIK all crates should do this, given current compiler/cargo limitations

Jake Goulding (Aug 13 2020 at 23:08, on Zulip):

I was afraid you’d say that. Do we have any sense of how to fix those limitations? Adding tests in this way has been the de-facto recommendation since Rust 1.0.

simulacrum (Aug 14 2020 at 01:21, on Zulip):

@Jake Goulding to be clear, the difference is between #[cfg(test)] mod foo { ... } and #[cfg(test)] mod foo;, both of which seem pretty canonical to me. It is true that the first option is in some sense more ergonomic (and recommended, e.g., cargo new --lib generates it), though.

I think fixing this basically isn't possible given the current file-based interface layer between rustc and Cargo; I am not aware of build systems that operate on a more granular level, but perhaps they exist. We could make rustc emit some more detailed information that isolated cfg-removed code as "not important" or Cargo could have a "quick run" feature for rustc where we pass in a hash that makes rustc exit early if, for example, the parsed (and expanded) crate is not different. Maybe incremental will eventually mean that the point here is moot, as recompiling will be sufficiently instant that it won't be a problem, but I think this is not going to be true for a long while -- and perhaps never.

The primary problem at least for most crates is not in the individual case of recompiling the library when running cargo test, but the fact that said recompilation forces Cargo to recompile all downstream crates too.

I guess an alternative is that we get reproducible builds to 100% and incremental far enough to where the recompilation of the library gives cargo an identical binary which it can then say "hey skip recompiling things depending on this".

lzutao (Aug 14 2020 at 02:08, on Zulip):

I read somewhere else that bazel has capabilities to avoid rebuilding leaf files.

lzutao (Aug 14 2020 at 02:08, on Zulip):

But I've never used bazel so

simulacrum (Aug 14 2020 at 02:36, on Zulip):

I'm not sure that is the case, but I also don't really know what the author meant by that - it seems like doing what I suggest wouldn't be possible without a Rust parser and I doubt they have that :)

Jake Goulding (Aug 14 2020 at 13:20, on Zulip):

the difference is between #[cfg(test)] mod foo { ... } and #[cfg(test)] mod foo;

Good to hear, that was what I was envisioning.

but the fact that said recompilation forces Cargo to recompile all downstream crates too.

That sounds promising — I'm not seeing how that impacts a normal user with a single crate or even a workspace. Is this really just a problem for the complicated setup inherent to the compiler itself?

Joshua Nelson (Aug 14 2020 at 13:22, on Zulip):

I think workspaces would have the same problem

Joshua Nelson (Aug 14 2020 at 13:22, on Zulip):

if you modify any file in rusoto_core all 200 downstream crates now have to be recompiled

Joshua Nelson (Aug 14 2020 at 13:22, on Zulip):

and having tests inline means that changing tests counts as changing a source file

simulacrum (Aug 14 2020 at 14:38, on Zulip):

Well, it still impacts people with just one crate, just less - you still invoke rustc twice if they're inline vs not

Jake Goulding (Aug 14 2020 at 19:57, on Zulip):

I'm sure I could figure this out by issue spelunking, but how is this not already trivially handled by incremental? Shouldn't the queries resolve to the exact same thing because all the changes happened in the cfg-gated section?

Or is the problem that we end up relinking, even without any compilation?

Jake Goulding (Aug 14 2020 at 19:59, on Zulip):

both of which seem pretty canonical to me

Likewise in the "if I weren't so lazy" camp, I wonder what a search of github for mod test; vs mod test { would turn up. I'd pretty strongly expect inline test modules to be 90% of the case, if not more.

As a data point, it's what I've encouraged at every training session I've ever given.

simulacrum (Aug 14 2020 at 20:02, on Zulip):

incremental is both not always on and doesn't give you zero-time -- in particular, we still parse and expand code at least

simulacrum (Aug 14 2020 at 20:02, on Zulip):

and incremental isn't perfect even on identical code

simulacrum (Aug 14 2020 at 20:02, on Zulip):

much less when you've actually changed things just in cfg'd out code

Jake Goulding (Aug 14 2020 at 20:13, on Zulip):

Moving the tests to a separate file doesn't give you zero time either, right? You still parse and expand all the non-test code to get to the #[cfg(test)] mod test, yeah?

simulacrum (Aug 14 2020 at 20:26, on Zulip):

no

simulacrum (Aug 14 2020 at 20:26, on Zulip):

if it's a separate file you only run rustc once

lzutao (Aug 18 2020 at 08:54, on Zulip):

I wonder if salsa will help improving this case.

lzutao (Aug 18 2020 at 08:56, on Zulip):

also, anybody want to take this issue after finishing proposal.
If I take this issue, should I do it part-by-part or all-in-once?

matklad (Aug 18 2020 at 08:58, on Zulip):

I wonder if salsa will help improving this case.

Not completely, "has this file changed?" is an important coars-grained granularity used by any build system. No matter how smart you make the compiler to be, it won't help you to determine if you should run the compiler in the first place.

lzutao (Aug 18 2020 at 09:02, on Zulip):

I agree, but will the compiler do less work and run faster?

matklad (Aug 18 2020 at 09:09, on Zulip):

Yes, in a rather general sense that "improving incremental compilation will improve incremental compilation". I don't think there's anything specific to salsa per se -- it uses roughly the same red-green logic as the one already in the compiler.

Jake Goulding (Aug 18 2020 at 15:38, on Zulip):

simulacrum said:

if it's a separate file you only run rustc once

I'm totally missing something then. My understanding is that Cargo invokes rustc src/lib.rs which then has to parse that file to find all the modules to include them.

Is this after that step, when we've output the .d files? So in non-test mode the .d files don't include the separate test.rs files, but the .d files in test mode do?

simulacrum (Aug 18 2020 at 15:41, on Zulip):

so the process if it's in a separate file is:

so the process if it's in the same file is:

simulacrum (Aug 18 2020 at 15:41, on Zulip):

yeah there' separate .d for the --test run and not

Jake Goulding (Aug 18 2020 at 15:41, on Zulip):

(foo.rs?)

simulacrum (Aug 18 2020 at 15:43, on Zulip):

er, lib.rs

Jake Goulding (Aug 18 2020 at 15:44, on Zulip):

And these processes are only for the second run of cargo test, right, as the first run Cargo has no knowledge about the files involved yet?

Jake Goulding (Aug 18 2020 at 15:45, on Zulip):

And is the distinction powered by the .d files?

simulacrum (Aug 18 2020 at 15:46, on Zulip):

hm well the first run would need to build the crate anyway

simulacrum (Aug 18 2020 at 15:46, on Zulip):

but if you're after e.g. cargo build, then it doesn't need to

Eric Huss (Aug 18 2020 at 15:59, on Zulip):

Just to be clear, the first call to rustc lib.rs is only for integration tests and doctests. If you run cargo test --lib, it won't do that step (which I think is relevant here, since we're talking about modifying unit tests). There is no --extern for unit tests pointing to itself.

simulacrum (Aug 18 2020 at 16:01, on Zulip):

ah, yes, that is a good point

simulacrum (Aug 18 2020 at 16:02, on Zulip):

but @Eric Huss that doesn't help speed up rebuild times in the case that your crate also has integration tests, right? Because you'll still wait for the crate to build twice unless passing --lib

simulacrum (Aug 18 2020 at 16:02, on Zulip):

(obviously in parallel to some extent)

Eric Huss (Aug 18 2020 at 16:06, on Zulip):

Yea, in the context of x.py, AFAIK there doesn't exist that fine-grained control, so I think the original proposal is good for the standard library. However, I'm not sure I entirely agree that it should be a general pattern everyone should necessarily use. I generally use targeted flags (like --lib), but maybe most people don't?

simulacrum (Aug 18 2020 at 16:20, on Zulip):

I can't remember the names associated with them (flag itself) or the "target names" needed

simulacrum (Aug 18 2020 at 16:21, on Zulip):

Maybe if we did the x.py thing of cargo test src/foo/test.rs

simulacrum (Aug 18 2020 at 16:21, on Zulip):

(x.py would love to move that logic out to cargo so it works for more than just ui testing)

lzutao (Aug 24 2020 at 13:24, on Zulip):

almost 10 days since the proposal seconded
I'd ask again:
lzutao said:

also, anybody want to take this issue after finishing proposal.
If I take this issue, should I do it part-by-part or all-in-once?

triagebot (Aug 26 2020 at 20:13, on Zulip):

This proposal has been accepted: #344.

lzutao (Aug 27 2020 at 14:00, on Zulip):

Proposal is being implemented in https://github.com/rust-lang/rust/pull/75979 (draft state).

Last update: May 07 2021 at 06:00UTC