Experience report contributing to rust-lang/rust

I rarely make PRs to rust-lang/rust, because the nature of my contribution to the Rust project rarely requires it. My penultimate PR was made in September 2018. I made a PR over the last few months and I've found it orders of magnitude more difficult than in 2018, enough that I will actively avoid making code contributions if I can help it in the future.

I am an extremely informed user, who understands the language, the project's processes, etc very intimately. Therefore, if I find it this daunting, I can only imagine that this is a problem for other would-be small contributors. Since I understand the Rust project would like to be accessible for new and infrequent contributors, I wanted to write about my experience to hopefully give useful feedback to the people who maintain the rust-lang/rust repository.


The fundamental issue is time: everything stems from the fact that I did not want to spend the many hours required to do a complete set up, to run tests locally, etc. My contribution was a small addition to the standard library, purely additive, which I could compile and check separately from the main library.

Ultimately I managed to make my contribution without doing a full bootstrap, but even git operations took unreasonably long. My initial clone took around 2 minutes (the repo is now nearly 500 MB), instantiating submodules took 10.

To avoid bootstrapping, I compiled my additions as their own library outside of the Rust repo. I believed this would accurately predict the status of CI builds because I knew no impls added would've been covered by the orphan rules, so I knew it was analogous to the actual code I would add. However, I was not prepared for the rust repo's tidy check.

Here are the changes that tidy demanded in order to pass CI. Please judge for yourself whether it was appropriate to block CI on these diffs:

-use core::task::{Waker, RawWaker, RawWakerVTable};
+use core::task::{RawWaker, RawWakerVTable, Waker};
-/// 
+///

(There was a trailing space in the first line)

-        unsafe {
-            Waker::from_raw(raw_waker(waker))
-        }
+        unsafe { Waker::from_raw(raw_waker(waker)) }
-    RawWaker::new(Arc::into_raw(waker) as *const (), &RawWakerVTable::new(
-        clone_waker::<W>,
-        wake::<W>,
-        wake_by_ref::<W>,
-        drop_waker::<W>,
-    ))
+    RawWaker::new(
+        Arc::into_raw(waker) as *const (),
+        &RawWakerVTable::new(clone_waker::<W>, wake::<W>, wake_by_ref::<W>, drop_waker::<W>),
+    )

(In my opinion this last one has actively made the code much worse!)


It's very easy to lose sight of the experience new and infrequent contributors have when you are a maintainer and regular contributor to a repository. Contributing to the rust repository now requires hours of set up and significantly expensive checks to write code which passes CI. I am afraid the project is becoming encloistered by the size of its repository, the time it takes to set up, and practices which don't consider the experience for people who do not have a current environment already on their machines.

37 Likes

Thank you for the detailed report!

One question:

Leaving aside the time required to check out the git repository (theoretically a one-time cost for each contributor, though submodules make updates annoying), does tidy take an excessive amount of time to run, or is it excessively complex to run?

(Also, I think this provides additional supporting evidence that, rather than blocking the PR, these changes could potentially be done as fixups by a bot prior to merging.)

2 Likes

As far as I know it takes no longer than building the code. The issue is just that it's not trivial to reproduce outside of a bootstrapped rust repo. My practice has been to avoid bootstrapping on new machines by building my code separately to catch errors and then copying it into std, but this practice can't be applied to tidy.

However it's also not clear to me why tidy requires a bootstrapped compiler to run, perhaps that could be changed.

1 Like

Please judge for yourself whether it was appropriate to block CI on these diffs:

I agree that this was annoying. Happily, the rest of the CI checks are no longer blocked on tidy, so you shouldn't have this issue in the future.

However, I was not prepared for the rust repo's tidy check.

I think for new users, this shouldn't be so much of a problem. It's mentioned explicitly in the contribution guide, for instance. x.py test tidy is very quick in my experience. (Note that passing tidy was always a requirement for rustc — the checks have just become more stringent with the rustfmt requirements.) In addition, the change above should make things less frustrating in this regard.

(In my opinion this last one has actively made the code much worse!)

Forcing rustfmt in rustc has been somewhat a point of contention recently — there's a long thread on the merits and demerits of enforcing rustfmt in the Rust repository in Forced rustfmt is a roadblock to contributing, which you might want to skim through.

I agree with @josh that the one-time cost for setting up the repository gives the contribution experience more overhead the first time: after this, the experience is much faster.

However it's also not clear to me why tidy requires a bootstrapped compiler to run, perhaps that could be changed.

You don't need to compile rustc to use tidy, just the tool itself. I think you should be able to use rustfmt (with the same settings) to essentially reproduce tidy, modulo some features like checking error numbers.

2 Likes

Almost all of my contributions have been changes to std, not rustc. I expect this is common for infrequent contributors: std is much easier to make a small contribution to than rustc. One thing that would make my experience a lot better would be an easy way to build and check libstd with my system rustc.

17 Likes

Running tidy using system rustc seems perfectly reasonable to me; it's only rustfmt that needs to be the same version for everyone.

1 Like

Using system rustc for tidy alone might be possible, but it's a challenge for building std because that uses unstable features. You can use something like ./configure --local-rust-root=/usr, but the bootstrap flags need this to be either version N or N-1 of what you're trying to compile. For compiling nightly std, that means using either a matching nightly or N-1=beta, which is probably not what you have as a system rustc.

My system rustc is nightly by default, and I'm happy to update to the most recent nightly when building std. Shouldn't that be sufficient to build std most days?

5 Likes

Sure, if your system rustc is a current nightly, that will probably work already.

I suspect most drive-by contributors would have stable though.

1 Like

Since rustc always uses unstable (pre-stable) features, it seems completely reasonable to require nightly. It's easy to add that with rustup, and generally pretty fast unless one is compiling on a Raspberry Pi or something similarly under-resourced.

7 Likes

Generally speaking, it's been a long-standing goal and project to make contributing to specific parts of the compiler easier (e.g., using a just-compiled std without building the compiler and LLVM). But that's hard, and we currently have no people with the time and resources to do so (AFAIK).

std on master is only reliably buildable with either a) a beta compiler, if you pass --cfg bootstrap or b) latest master toolchain (e.g. via rustup-toolchain-install-master).

Nightly may work, too, but it's not really guaranteed to, and indeed I would expect semi-frequent breakage, unless you only start working at the "perfect time" when master is equivalent to the latest nightly.


Also, thank you for this post! I personally think one of the major challenges with making the contribution experience easy is getting feedback, and I think we don't get anywhere near enough of that.

With that in mind, I have a few questions -- you mention that the formatting check was an obstacle for you. You also mentioned that "The issue is just that it's not trivial to reproduce outside of a bootstrapped rust repo." -- I would personally hope that this is not true, in that the reproduction should essentially consist of using our rustfmt.toml and (roughly) the same nightly rustfmt we do (found in src/stage0.txt). Rustfmt changes on nightly quite rarely, so most likely any recent nightly rustfmt would do. Did you find this to not be the case? Are you not using rustfmt on save generally?

(I think getting feedback from users on whether that's the case is super valuable, and if not I'd be interested in hearing why -- if you've posted elsewhere, a link would be fine. I personally find rustfmt suboptimal most of the time but far better than trying to deal with formatting things myself.)

7 Likes

Can't this be "solved" by just advising to base the work on the nightly commit? I imagine for "small and infrequent" contributors the difference usually wouldn't matter.

1 Like

Just a quick thought: why not make std, core, alloc etc submodules and require that they work with yesterday's nightly instead of today? I think this would allow us to move it out of tree and then people could just contribute to that.

8 Likes

Also, I'll share my pain points from contributing:

  • Frequent painful rebasing: I've been doing a bunch of refactoring PRs lately, and I expect that they will have conflicts (I don't think there is any way around that). But I really wish there was a warning when multiple PRs in the queue will conflict with mine so that I can just rebase once and know that I won't have to do it again.
  • Disk space usage: Yesterday, I was doing an unrelated task and I got an ENOSPC error for something. It turns out that my 4 clones of the rust repo were taking up ~100GB of space (over half of my SSD). Most of this was various build artifacts. In particular, the incremental caches never seem to be cleared out, so they just grow monotonically forever until you delete them.
  • Hidden tests (e.g. --compare-mode=nll): as noted here and elsewhere, the full test suite takes forever to run. So I avoid running it like the plague. If anything, I just run ui tests, but often I just wait for CI to give me an error. However, I often find out late about the failure of other test suites like ui --compare-mode=nll (or more recently run-make-fulldeps). This means that after I think my PR is ready (and sometimes even when the PR is already in the queue), I find out otherwise.
  • Long PR queue: a PR can wait in the bors queue for weeks, which makes all of the above worse.

Also, an observation, which I'm not sure is helpful is that many of these issues would be less painful if boostrapping was faster or unnecessary somehow. But I have no ideas on how to do that.

11 Likes

Just a quick thought: why not make std , core , alloc etc submodules and require that they work with yesterday's nightly instead of today? I think this would allow us to move it out of tree and then people could just contribute to that.

This seems to make a lot of sense. I would guess that the contributors for std and the compiler internals are also broadly different groups, so separating this more over time would make sense. (Of course, further librarification in general would potentially improve the situation more.)

4 Likes

We used to have fast-moving bootstrap requirements, but it's challenging to keep that up, especially when it comes to building a final release. It's nicer to have a more stable base for building the next. If nightly breaks for some reason, we still bootstrap from a working beta, and beta from stable.

So really, a quick check of master std is easier with a beta system compiler, since that's what stage0 is designed for.

1 Like

I think there are two related, but very different issues here, and it's best to untangle them:

  • rust-lang/rust have certain extra checks to normalize trivial details (tidy)
  • rust-lang/rust is hard to build

Note that "hard to build" affects tidy, as it adds up to the time until you see a tidy error (one can run tidy separately from the main build/test, but one needs to be an expert user to do this). For this reason, I'd love to dive deeper into the second issue, and only touch the first one.

I do believe automated tidy checks are valuable. If you don't fix the trailing WS, someone else would fix it later, so this doesn't actually save any work. Moreover, meanwhile people will spend time reverted ws changes from their unrelated PR, which will create more friction overall. Similarly, as much as I don't like pretty-printing approach of rustfmt, it at least normalized the repository to a single consistent style. Adding new code before was horrible, because one needed to decide which one of the 2.5 styles from the current file to use.

But it is important to make this checks seamless and fast. rust-analyzer checks quite a few of code invariants (rustfmt, no trailing ws (rustfmt allows it in some cases), no todo, each module has a doc commt). However, they are all checked by a vanilla cargo test (we just shell-out to rustfmt from a #[test] fn), and the whole test suite takes 15 seconds. I think this is the experience we should strive for in rust-lang/rust

Here is the paragraph where I give praise to everyone behind original implementation and continuing maintenance of x.py and current cargo-based bootstrap. We should not forget that the current complicated build process is waaaaay better than the makefiles we had before.

However, I do believe that the current situation can be improved a lot. But this is from someone who is not intimately familiar with rustc build process, so take each "should" with a pinch of salt. What follows is a brain dump of things that could be done. It probably would read a little bit like criticizing, but I'd love to explicitly say that this is not true, and give :heart: to anyone involved in the build process, as this is a very annoying and fiddly work :slight_smile:

I do agree that rustc and stdlib are two separate worlds and should be kept as such. I wish we had /compiler/src and /runtime/src instead of a single unified /src, if only to make it easier to find the relevant code.

I think that the default ./x.py process is overly geared towards bootstrapping a compiler. We can assume that the person trying to do something with rust-lang/rust probably has rust already, so we should have .rustup-toolchain file which specifies the beta toolchain and use that by default. The "full bootstrap without existing rust" should be a special case for folks packaging rust. Similarly, default config.toml should be geared towards contributors and not packagers. There should be a separate config.release.toml for "production" use.

If we rely on beta toolchain existing by default, I think we can replace x.py with the xtask hack. It is as hacky as it gets, but it works reliably in rust-analyzer.

We should strive to make as much code as possible to be a boring run-of-the mill rust project buildable with cargo build. I think we can organize src/runtime such that cd src/runtime && cargo test just works, by using the current beta compiler?

For /src/compiler, I think we should strive to further subdive it into pure text-processing part (analysis), and "llvm, linkers, io and the rest of the horrible real world" part (emit). Most of the compiler can be a very boring rust workspace which has no need to require anything beyond cargo-check. Crucially, for the most of the compiler, you don't actually care about stdlib, as you are not generating the code.

So, in my ideal imaginary future,

$ git clone git@github.com:rust-lang/rust && cd rust
$ cargo test
  • automatically downlads beta cargo & rustc via rustup, due to toolchain file
  • compiles and tests all the crates in /runtime using the stock beta compiler
  • compiles and tests all the crates in /compiler/analysis using the stock beta compiler and not touching stdlib at all.
  • runs tidy-checks
  • prints a message "hey, we've skipped codegen and bootstrap tests, run cargo test -p emit to test them"

cargo test -p emit would do the really heavy lifting:

  • download/compile LLVM
  • compile the stdlib to rlib
  • compile the analysis, and link it with coden and LLVM
  • assemble sysroot
  • run the resulting compiler on the set of fully integrated tests.

I do belie that we can and should skip codegen tests by default, and that maybe we can defer them even to some tiered bors system, where

  • every merge is a rollup of N PRs
  • rollups run full bootstrapping test suite
  • before getting into a rollup, each PR passes a sans-codegen CI check.
30 Likes

My advice here is to avoid too many dependent PRs, or use --onto when rebasing. Also, prefer smaller PRs. If your PR is larger it reduces the chances of being included in a rollup.

I think we should retain the monorepo setup, as it becomes much harder to review a submodule or crate version bump (e.g., since hashbrown moved out of tree it has become harder).

I've asked about having --pass check --compare-mode=nll in the PR builder, but we have long build times already, so this will have to wait on GHA sadly.

Well there are two ways we can improve that situation: a) more rollups (so fewer toolstate PRs!), b) GHA.

Yeah I agree with this; especially as we're adding more crates to improve parallelism and modularism, it makes it frankly harder to scroll in VSCode.

2 Likes

I think autoformatting is a bad trend in software & I don't even have rustfmt installed (I specifically avoided including it in my toolchain bundle to not waste time downloading it). Of course I'm happy to install it to contribute to rustc, but it wasn't obvious to me that tidy was just running rustfmt or if it involved other checks, and so I didn't try to reproduce the check outside of the rust repo. I didn't investigate deeply, I just let CI tell me what changes to make.

3 Likes

Small correction: CI still requires tidy to pass, it's just that it no longer requires tidy to pass in order to run the rest of the rest suite.

7 Likes