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

Documents managing tools in the Rust repo #27

Merged
merged 1 commit into from
Aug 22, 2017
Merged
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
141 changes: 141 additions & 0 deletions tools-in-rust-repo.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
# Maintaining tools in the Rust repository

This document contains information for tool maintainers and Rust contributors
who need to work with tools that are part of the Rust distribution and
repository. For information on requirements for such tools see the
[stability docs](https://github.com/nrc/dev-tools-team/blob/master/stability/README.md).

When adding tools or making significant changes, please be sure to cc @nrc and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fine being pinged on all Clippy changes

@alexcrichton on PRs. Adding tools to the Rust repo requires approval from the
dev-tools team and sign-off from the core team.


## Repository layout

Tools should live in their own repository which should be in an official
organisation (e.g., [rust-lang-nursery](https://github.com/rust-lang-nursery)).
Tools should be included into the Rust repository as a Git submodule in the
[src/tools](https://github.com/rust-lang/rust/tree/master/src/tools) directory.


## Constraints on tools

It is essential that any commit used as a submodule in the Rust repo is
preserved forever (i.e., is *never* deleted or changed). This ensures that we
will continue to be able to build any historic release of Rust (which is
important for bisecting regressions, etc). In order to do this, any commit
pulled into the Rust repo as a submodule must be from a Rust organisation repo,
not an author's. If the commit is only on a branch, that branch cannot be
deleted. Whether the commit is on a branch or master, it cannot be rebased
(which essentially deletes the commit and creates a new one).

Tools used in the Rust repo cannot use Cargo workspaces. This is because the
tool itself will be treated as a workspace crate and workspaces cannot be
nested.


## Making breaking changes

If making a change to a 'core' part of Rust causes breakage in any of the tools,
then there is a bit of a dance to perform to land the Rust PR.

Communication is key here! If you're a compiler dev, communicate with tool devs
as soon as possible (and vice versa).

At the end of the day, the tool's maintainers are responsible for ensuring that
their tool continues to build and pass tests in the Rust repo. For non-trivial
fixes, tool devs should be happy to help fix breaking changes.

How to land a breaking change:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually assuming that the default would be "stop building the tool, wait for a nightly, fix the tool, re-enable the tool" in the sense that this process is pretty onerous. The other downside of this approach is that it's adding burden to compiler devs to fix tools, where we aren't necessarily willing to shoulder that burden on compiler devs for all commits.

Do you think this'll be more common than the "exceptions" branch though?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we discussed this in the past we had the 'exception' process as what we wanted to do, with some infra in the build system to make this easy to do, to prevent doing it on beta or in the last week of a cycle, and to require a visible opt-in. I still think that is the better long term solution. We decided we should do the 'non-exception' process for the time-being until we add code for the 'exception' process, despite the onerous-ness. I still think that is a good plan. I think encouraging the 'exception' process without the code to help is a bit risky.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But fundamentally this isn't a solution, right? We're forcing all tools included to have broken CI to land changes?

I guess I'm just surprised at this because this is a fundamental problem we can't fix, and I'd be much more opposed to including projects like clippy if we require all developers to fix clippy rather than just those knowledgeable.

It's of course painful to take the "exceptions" route but isn't that the pain of nightly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't see this comment before merging.

The tool CI never breaks on master, only on a branch. So, I think it depends on the breaking change - so far with the RLS, the changes have been trivial to fix (e.g., adding () to every use of span.lo/hi) and the compiler changer has made the change, and I've done the branch dance on the RLS-side (e.g., rust-lang/rust#43968). I imagine that if the change needed more work in the tool, then the tool authors would do the work, but it would be basically the same process (I've tried to stress that it is the tool authors responsibility here).

The trouble with the exceptions route is that it leads to a tool not being built, which means that rustup will look for it and not find it, and presumably error out. We really need infrastructure support before I'm happy recommending that route. (And in practice the non-exception route is working OK, and with the incremental path to enabling tools, we should have time to observe if it is continuing to work OK).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess? At some point I'm not personally responsible for the tools at hand, so I'll let y'all call the shots...

I really am worried about the contributor experience, though. I'd dread wanting to make a change to rustc only realizing it breaks 3 tools so I have to go and update 3 submodules, push to new branches, coordinate with maintainers, etc. If this starts happening a lot it seems like it can become a real drag on iterating in the compiler?


* Make your changes to the Rust repo
* Make changes to the tool (against its own repo, not the submodule in the Rust
repo) to address the breaking changes.
- check that the tool's tests pass inside the Rust repo
- either push a branch to the tool's repo or submit a PR
* Point the Rust submodule at the new tool commit
* Land the PR
* Wait for the PR to be included in a nightly release...
* Merge the tool commit to the tool's master branch
- if the commit was changed or rebased, **do not delete** the commit's branch
(See https://github.com/llvm-mirror/compiler-rt/branches for example of this policy)
* Create a new PR to the Rust repo pointing the tool submodule back to tool's
master tip; land it.

When changing the tool submodule, make sure that any other commits are OK to
land (i.e., they are not work in progress and tests pass, etc.).


### Exceptions

Rarely, a breaking change might be too complex to manage in the above fashion.
In such cases it is OK to temporarily stop building and testing the tool while
the tool devs fix the issue.

* If you think this is necessary, you must communicate this to the tool's
maintainers, the dev-tools team, and the compiler team before landing the PR
(and preferably before starting the work).
* Try and do this early in the release cycle to give the maximum amount of time
to fix the tool before the beta uplift.
* It is never OK to do this on beta or stable branches.
* Avoid doing this, if you can.

The steps to follow are similar to the previous steps, but you must comment out
code in the build system as needed, rather than change the submodule, when
landing your initial PR.


## Building and testing a tool

After adding the tool as a submodule, you'll need to make some changes to the
build system to build and test the tool, and to have it distributed with Rustup.

For all these changes, it is probably easiest to see how it is done for an
existing tool and try and copy that where possible. You can try grepping for
`rls` and `Rls` inside [src/bootstrap](https://github.com/rust-lang/rust/tree/master/src/bootstrap).

To build the tool you'll need to add the tool as a workspace to `src/Cargo.toml`
and make several changes to `src/bootstrap/tool.rs`.

To run a tool's tests, you'll need to make changes to `src/bootstrap/check.rs`.

You probably want the 'tidy' script not to check your tool, to do that you'll
need to add the tool directory to the skip list in `src/tools/tidy/src/lib.rs`.


## Distribution

To include a tool as a Rustup component you have to ensure that a tarball of the
compiled tool is created and put in the right place, and that there is an entry
in the Rustup manifest (generated by the build system). You don't need to make
any changes to Rustup itself. You'll need to edit `src/bootstrap/dist.rs` and
`src/bootstrap/install.rs` to create the tarball, and
`src/tools/build-manifest/src/main.rs` to make an entry in the manifest.


## Tips

Rustbuild will update submodules as part of its build process. If you don't
commit the change, it will be overwritten. Therefore, you should either commit
after updating a submodule and before building, or set `submodules = false` in
`config.toml`.

After making changes to `Cargo.toml`, you need to build to ensure `Cargo.lock`
is updated. Changes to `Cargo.lock` must be committed along with `Cargo.toml`
changes.

To run tests for a tool, use `./x.py test src/tools/rls`. Note that it is
possible for tool tests to pass in their own CI, but fail when run inside the
Rust repo because of the different environment.


## People who can help you

Usernames are for irc/GitHub.

* Any questions - nrc/@nrc or ask in #rust-dev-tools
* Rustbuild - simulacrum/@Mark-Simulacrum, acrichto/@alexcrichton, or ask in #rust-infra
* Compiler issues - ask in #rustc, if you need a specific person try mw or nmatsakis.
* Rust distribution - acrichto/@alexcrichton
* Rustfmt or RLS - nrc/@nrc
* Clippy - Manishearth/@Manishearth or oli-obk/@oli-obk