Make some aspects of check/build available as an API. #3427

nrc · 2016-12-19T00:13:31Z

There are two key parts to this commit:

let API clients run cargo check with minimal fuss (ops/cargo_check.rs),
let API clients intercept and customise Cargo's calls to rustc (all the Executor stuff).

requires rust-lang/cargo#3427 closes #82 cc #90

matklad · 2016-12-19T09:22:50Z

src/cargo/ops/cargo_rustc/mod.rs

@@ -289,7 +325,7 @@ fn rustc(cx: &mut Context, unit: &Unit) -> CargoResult<Work> {
        }

        state.running(&rustc);
-        if json_messages {
+        let cont = if json_messages {
            rustc.exec_with_streaming(


Note that this part bypasses the Mutltishell and uses standard streams directly (machine_message::emit and #3410). I don't think you'll hit this when using Cargo from RLS, but it's something to keep in mind.

Yeah I'm slightly worried about this in terms of only bits and pieces of Cargo's API will work with these custom arguments. @nrc is this just avoided by the rls?

bors · 2016-12-19T19:54:04Z

☔ The latest upstream changes (presumably #3413) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2016-12-20T22:25:57Z

src/cargo/ops/cargo_check.rs

+use util::{CliResult, Config};
+
+#[derive(RustcDecodable)]
+pub struct Options {


These structures typically are command-specific, so I'd prefer to leave this in src/bin/check.rs. The functionality though should all be configurable, I believe, through the options generated later. Was something missing though perhaps?

I was trying to keep calls via the API as close to calls via the CLI. I'll take another look and see what I can figure out

Ah yeah the intention was that all the CLI options have their own suite of flags and such but everything "compiles down" to CompileOptions where that's the common functionality amongst everything.

alexcrichton

I like the idea!

Could you elaborate more to the purpose of ContinueBuild? It looks like it may not cancel the entire build but just the copy of artifacts right after the compile? If we do support that I'd expect the job graph to stop executing as soon as possible once it sees a "stop"

alexcrichton · 2016-12-20T22:26:39Z

src/cargo/ops/cargo_compile.rs

+
+pub fn compile_with_exec<'a, E: Executor>(ws: &Workspace<'a>,
+                                          options: &CompileOptions<'a>, 
+                                          exec: &mut E)


Could this take &mut Executor (trait object) to cut down on monomorphization?

Hm so Executor: Clone could throw a wrench into this, but perhaps that could be jerry-rigged with something like:

fn clone(&self) -> Box<Exectuor>;

(or something else manual like that)

Why is monomorphisation a problem? I'd have thought using generics is more efficient here (the common case is to just have a single Executor) and adding an extra param doesn't seem too much of a burden.

The type parameter ends up getting plumbed all the day down throughout cargo_rustc, which is the majority of the code in Cargo. I'd prefer to keep Cargo's own compile times down and there's not really a need for speed here, right?

This shouldn't affect Cargo's compile times at all - there is only one actual type which implements Executor, so unless you're using Cargo as a lib, exactly the same amount of code is processed as before. The only downside is we add a generic parameter in a few places, which is a bit ugly, but is also kind of best practice (IMO). Speed is important here, but you are right in that the speed improvement (vs trait object) is unlikely to be important.

Hm I'd still very much prefer to have this be a trait object, can we work out how to make the trait object safe?

nrc · 2016-12-21T01:44:54Z

re ContinueBuild, the idea is that it applies only to building a single Unit, so if you return Stop, you stop the build process for that unit, but continue to build others. This is useful because Cargo assumes it has created some artefacts and wants to do stuff with them, but if you've done things differently, you might not create those artefacts and thus Cargo errors out.

nrc · 2016-12-21T04:35:22Z

Updated with check's Options back in bin/check.rs

nrc · 2016-12-27T19:59:30Z

ping @alexcrichton for re-review

alexcrichton

Oh oops sorry! I think this fell out of my inbox by accident..

With ContinueBuild I was under the impression that Cargo already handled commands that didn't actually generate any output? I know that cargo rustc, for example, isn't guaranteed to produce output and we should handle that.

I'm just slightly wary of adding that feature which others may expect to fully stop the build on Stop whereas this just stops that step, and likely causes failures down the road for other dependent steps, right?

alexcrichton · 2016-12-27T21:59:28Z

src/cargo/ops/cargo_compile.rs

+
+pub fn compile_with_exec<'a, E: Executor>(ws: &Workspace<'a>,
+                                          options: &CompileOptions<'a>, 
+                                          exec: &mut E)


The type parameter ends up getting plumbed all the day down throughout cargo_rustc, which is the majority of the code in Cargo. I'd prefer to keep Cargo's own compile times down and there's not really a need for speed here, right?

alexcrichton · 2016-12-27T22:00:04Z

src/bin/check.rs

+}
+
+impl Options {
+    pub fn default() -> Options {


This is in theory only ever used in this file (not exported), so I'm curious why this was needed?

Ah, it's probably a remnant of when I had this as API, it can probably be removed now.

alexcrichton · 2016-12-27T22:00:50Z

src/cargo/ops/cargo_check.rs

+    where F: FnOnce(&Workspace) -> CliResult<Option<()>>
+{
+    let root = find_root_manifest_for_wd(flag_manifest_path, config.cwd())?;
+    let ws = Workspace::new(&root, config)?;


This seems like not a lot of code to inline locally perhaps? If we keep this function, though, could it just return a Workspace instead of taking a closure?

I think there was a borrow check issue with doing that - ws has to have a shorter lifetime than root. I agree it is kind of a small function, but there is already a little bit of code that needs to be inlined (to make the config) and it seemed like voodoo from the client's perspective - there seems no reason to allow the flexibility - so I thought it better to present a simpler API

I'm just always hesitant about closure-taking functions and such. Cargo's API already isn't really the most ergonomic and I just prefer to keep most operations as only having one consistent method of being done. For example this PR doesn't update all subcommands to use this method, so it's possible for them to start to diverge in behavior from cargo check sorta...

I'm fine making these sorts of API patterns aliased with something but I'd prefer to avoid the closure protocol wherever possible (e.g. by changing the API of Workspace or something like that) and to ensure that Cargo uses the pattern consistently internally if it does so.

alexcrichton · 2016-12-27T22:01:14Z

src/cargo/ops/cargo_compile.rs

@@ -62,6 +62,29 @@ pub struct CompileOptions<'a> {
    pub target_rustc_args: Option<&'a [String]>,
 }

+impl<'a> CompileOptions<'a> {
+    pub fn with_default<F, T>(config: &Config, mode: CompileMode, f: F) -> T


Could this just be a derive(Default) basically?

It's kind of annoying with the borrows here - there needs to be a lifetime for 'a (which is why this takes a closure, rather than returning a CompileOptions - I think the only way to do this would be to take a bunch of empty slices as args, which is pretty annoying. i don't think we can derive here.

We have Default for &[] though?

I tried again, and I don't think we can implement Default, because we want to take the config and mode as arguments, but we can just add a default method, we can get rid of the taking a closure thing.

alexcrichton · 2016-12-27T22:01:46Z

src/cargo/ops/cargo_rustc/fingerprint.rs

+                if let Ok(meta) = fs::metadata(path) {
+                    let mtime = FileTime::from_last_modification_time(&meta);
+                    *slot.0.lock().unwrap() = Some(mtime);
+                }


How come this was changed?

I think because with the RLS, we don't actually produce the file at path, and so we would get an error (even though we return Stop, there was no way to skip this code.

Hm I'd prefer to not change this in exactly this manner. This seems like it's just waiting to accidentally ignore errors and then not cause rebuilds when they should happen otherwise. Could such an error like this be caught elsewhere and handled?

For example I could imagine something higher up catching this error and leaving behind a tombstone for "this must be recompiled", or something like that.

alexcrichton · 2016-12-27T22:03:45Z

src/cargo/ops/cargo_rustc/mod.rs

@@ -289,7 +325,7 @@ fn rustc(cx: &mut Context, unit: &Unit) -> CargoResult<Work> {
        }

        state.running(&rustc);
-        if json_messages {
+        let cont = if json_messages {
            rustc.exec_with_streaming(


Yeah I'm slightly worried about this in terms of only bits and pieces of Cargo's API will work with these custom arguments. @nrc is this just avoided by the rls?

matklad · 2016-12-28T11:40:57Z

Yeah I'm slightly worried about this in terms of only bits and pieces of Cargo's API will work with these custom arguments.

I am in general worried about the idea of linking Cargo into the rls... I can't put my finger on exactly what I don't like here though. I feel that the project description produced by cargo metadata should be enough for rls.

Of course you'll need cargo to build compiler plugins and execute build scripts, but this can be done outside of rls I think? If user adds a new plugin, or changes build.rs, he would get wrong results from the rls until he runs cargo build manually.

Intercepting compiler calls seems too crude. I don't see how it can be extended to leverage the crucial optimization of the IDEs: analyzing only the files currently opened in the editor and ignoring most of the code.

nrc · 2016-12-28T20:06:00Z

@matklad the way the RLS uses Cargo is only on initial build in order to build dependencies and find out how the project ought to be built. Once we've got that information, we use the compiler directly. I.e., we only do the compiler call intercept once, and then do our own thing.

alexcrichton · 2016-12-28T21:57:20Z

@nrc I wonder if cargo build --message-format json has enough information for you to learn about all that nowadays? That should print paths to all the artifacts and such so you should be able to reconstruct all the relevant flags for --extern and such.

nrc · 2016-12-28T22:05:20Z

@alexcrichton we'd still need to have some API for avoiding the build of the last crate. If we're doing that, it seems we may as well get the data we need directly rather than by parsing JSON. We might also want to further customise in the future.

alexcrichton · 2016-12-29T00:17:13Z

@nrc true yeah, I was just thinking that downside may not be worth linking to all of Cargo and bringing it in, but I'm fine either way.

nrc · 2016-12-29T00:31:46Z

With ContinueBuild I was under the impression that Cargo already handled commands that didn't actually generate any output?

This is not true, at least not in the way in which we are doing things. I believe the difference is that in this case Cargo expects rustc to make an output and we are signalling to Cargo that the build was successful, and in that case, if there isn't an output, Cargo panicked. We could probably fix this by making Cargo less picky - e.g., check for files rather than assuming they are there, and if not silently skipping steps. My assumption was that we'd like to keep this strictness, but if you prefer we could be lax instead. Alternatively, we could try and be more explicit that Stop only applies to that unit.

nrc · 2016-12-29T04:48:57Z

Yeah I'm slightly worried about this in terms of only bits and pieces of Cargo's API will work with these custom arguments. @nrc is this just avoided by the rls?

Yeah, the RLS doesn't ask for JSON messages (we ignore any messages from any crates we compile with Cargo) so this is fine. It is kind of ad hoc, but I think the only way to make this nicer is to complicate the API. I'll take another look and see what I can do though.

nrc · 2016-12-29T05:40:25Z

So, I fixed the json messages stuff by adding an exec_json method to Executor. However, that takes closures and so the trait is no longer object-safe, which means we can't change the various methods from generic to taking trait objects (although I suppose we could take closure objects, rather than using generics).

matklad · 2016-12-29T06:46:38Z

we'd still need to have some API for avoiding the build of the last crate.

And what exactly is "the last crate"? What if you are compiling a workspace, or have path dependencies? That way, several crates may be in "modified" state (their files are changed in the editor, but are not save to disk yet), and they can depend on each other.

nrc · 2016-12-29T08:06:00Z

Honestly, I think we have to leave a lot of these issues till later. The current assumptions are that you'll edit a single crate and save files when changing crates. Clearly this isn't good enough for the long term, but I don't think there is anything fundamental in our approach which prevents us solving this in the future.

The API here allows us to check any crate we like, not just the last one, so it should be extensible in the right direction.

alexcrichton · 2016-12-29T16:37:19Z

src/cargo/core/shell.rs

@@ -62,6 +62,18 @@ impl MultiShell {
        MultiShell { out: out, err: err, verbosity: verbosity }
    }

+    // Create a quiet, basic shell from supplied writers.
+    pub fn from_write(out: Box<Write + Send>, err: Box<Write + Send>) -> MultiShell {


I don't think this is necessary, right? You should be able to create a shell via Shell::create and then use MultiShell::new above, right?

We could, it felt like it was exposing a lot of unnecessary details to clients though (need to know about Shell and ShellConfig and Verbosity, where the abstraction here is "send everything to me instead of the console and let me deal with it").

True yeah. Ok seems fine

alexcrichton · 2016-12-29T16:44:26Z

@nrc I'm ok not solving everything up front, but this currently definitely has a "grafted on at the last minute" feeling which'll be difficult to support in the future as we continue to change Cargo and adapt it to the RLS's needs. If taking the route of cargo metadata and --message-format=json is unacceptable then I just want to make sure that the changes here in Cargo can be accommodated as Cargo itself evolves.

nrc · 2016-12-29T20:23:20Z

@alexcrichton my comment was in reply to @matklad, not to any of your comments.

nrc · 2016-12-29T20:38:42Z

@alexcrichton I must admit I don't have much idea of how Cargo will evolve, so I've not been able to take that into account here. The trouble I think is that what the RLS wants to do is run as much like a normal Cargo run as possible, but tweak one little thing. The trouble is that that one little thing is deep inside Cargo. Therefore it seems very difficult to design an API where the client can just call various functions to get the behaviour it wants (without the client having to recreate a bunch of Cargo's behaviour, which is obviously fragile).

An alternative approach might be to have either a flag for 'skip building the last/primary crate' or to take a list of crates not to build. However, these feel overly specific and thus hacky.

matklad · 2016-12-29T20:47:06Z

An alternative approach might be to have either a flag for 'skip building the last/primary crate' or to take a list of crates not to build. However, these feel overly specific and thus hacky.

You can specify the set of packages to build with --package argument, and you can get a list of all packages via cargo metadata. So you can tell cargo to explicitelly build every package except the last one.

alexcrichton · 2016-12-30T21:25:26Z

Hm yeah what @matklad makes sense to me, and going further it seems like you could do something like:

RUSTC=/my/own/binary cargo rustc --lib -- --my-fancy-argument

That way when your own binary is run with --my-fancy-argument you know it's the "final crate" and you can pick up all the arguments via that method? That's basically guaranteed to always work (and is relatively minimal).

nrc · 2016-12-30T21:41:50Z

@alexcrichton this doesn't quite work - we don't want to use a rustc binary, we want to call bits of rustc as a library. This is also what we did before - we had a bogus argument which caused compilation to fail and parsed the command line - we agreed that that was a pretty gross hack.

It seems rather a lot of work to get all the packages, parse that info, cut something out of the list, pass it back to Cargo, etc. That seems both fragile and hacky to me (plus kind of a lot of work on the RLS side). Furthermore, I would expect the RLS to work more closely with Cargo in the future, not less, so using an API will make more and more sense.

At this stage the API we're adding feels pretty minimal - the only real change is letting clients to, effectively, cargo rustc, but supplying their own rustc, just programmatically. Otherwise, we add like one convenience method and make a couple of functions public. I don't really understand the problem to be honest - it feels that you want to avoid adding this, but I don't understand the motivation so I'm having trouble making this into something you'll like.

matklad · 2017-01-02T18:29:58Z

We want to do this with the same args (env and CLI) that Cargo would have used to build this crate.

What exactly are the arguments you are interesting in? I think there's no a single set of arguments to build the crate. What I mean is that Cargo can pass different arguments depending on the circumstances (profiles in Cargo.toml, --release flag, features). It's the user who (indirectly) controls the arguments passed to the compiler for each call to cargo. So I am not sure that by just intercepting compiler calls you would always get the arguments you really want.

nrc · 2017-01-03T19:43:23Z

What exactly are the arguments you are interesting in? I think there's no a single set of arguments to build the crate. What I mean is that Cargo can pass different arguments depending on the circumstances (profiles in Cargo.toml, --release flag, features). It's the user who (indirectly) controls the arguments passed to the compiler for each call to cargo. So I am not sure that by just intercepting compiler calls you would always get the arguments you really want.

We want to get exactly the arguments that rustc would get if it were called by Cargo. I.e., taking into account the Cargo.toml and any arguments to Cargo etc., then any processing that Cargo does. I.e., exactly what rustc would be passed, which is what we get if we intercept the call to rustc as I'm doing here.

nrc · 2017-01-03T19:46:12Z

@alexcrichton so, if I understand you, you're main concern is that we might be making life in the RLS harder for us, rather than that we are doing bad things to Cargo? Given that for the RLS using this API is easier than not, and the code is already written (in both the RLS and Cargo), it seems conservative to take this route and only do the extra work in the RLS if we need to (i.e., if it turns out to be a problem linking Cargo).

alexcrichton · 2017-01-03T19:50:17Z

@nrc to be clear I still don't mind changing Cargo, I just figured it's probably best to work out the best solution regardless of changes necessary first. I am indeed worried that this approach in this PR may be more painful for the RLS in the long term, yeah. The downsides in this PR I believe are:

Distributing the RLS now requires distributing Cargo twice (statically linked in two locations)
Cargo must now consider the RLS when making any changes. We might, for example, change everything to use async I/O in the future which could have large ramifications on the backend.
The version of Cargo inside the RLS can drift from Cargo itself, which is likely to cause breakage.

My thinking with the cargo-as-command-line approach is that the RLS does:

let mut cmd = Command::new("cargo");
cmd.arg("rustc");
add_target_flags(&mut cmd); // e.g. --lib, --bin foo, etc
cmd.env("RUSTC", env::current_exe());
cmd.env("IS_RUSTC_SHIM", "1");
cmd.env("MY_ARGS", &tmp_file_path);
cmd.arg("--");
cmd.arg("--my-dummy-argument");

run(cmd);

let rustc_args = File::open(&tmp_file_path().read().split(' ');

then later, inside the RLS's main function:

fn main() {
    if env::var("IS_RUSTC_SHIM").is_ok() {
        rustc_shim();
    } else {
        // ...
    }
}

fn rustc_shim() {
    let args = env::args().skip(1).collect::<Vec<_>>();
    if args.contains("--my-dummy-argument") {
        File::create(env::var("MY_ARGS")).write(args.join(" "));
        return
    }
    let mut rustc = Command::new("rustc");
    rustc.args(&args);
    rustc.exec();
}

That is, I don't think this requires IPC, platform-specific code, etc. There's an unambiguous synchronization point of when Cargo ends, and if the MY_ARGS file exists then that's all the arguments you need to run with.

Does that make sense? Or does that snippet seem like it won't work for the RLS's purposes?

nrc · 2017-01-03T21:41:21Z

@alexcrichton

Distributing the RLS now requires distributing Cargo twice (statically linked in two locations)

This is a valid concern. I'm not sure how much weight it is worth putting on an extra 50MB download, but it seems non-trivial at least. I suppose we could dynamic link to get around this, if it is an issue?

Cargo must now consider the RLS when making any changes. We might, for example, change everything to use async I/O in the future which could have large ramifications on the backend.

I feel this is not as a big an issue you make it out to be - in either case we have to consider the RLS - under your proposal we have to be certain we are not changing the semantics of cargo commands which are used in a way which regular users would never do, or change the performance of critical bits, etc. In the API approach, we are using Cargo very much like it would be used from the CLI, I believe that only major changes would affect the RLS and in cases such as moving to async IO, I think considering the RLS's API would be very minor compared to the rest of the work.

The version of Cargo inside the RLS can drift from Cargo itself, which is likely to cause breakage.

I think if we are using Rustup for distribution, this is unlikely to happen.

The code you present is simpler than I expected, using a file is much simpler than the IPC I was expecting. However, I still feel kind of bad about spawning an extra process and writing to a file, seems very inefficient compared to a function call.

It doesn't quite do what we want though - I want to use cargo check, not cargo rustc, and we don't have a variant of cargo check which only passes arguments to the last crate (which, IMO, is kind of odd semantics to have on the CLI in any case).

There are two key parts to this commit: * let API clients run `cargo check` with minimal fuss (ops/cargo_check.rs), * let API clients intercept and customise Cargo's calls to rustc (all the Executor stuff).

Mostly focussing on the ergonomics of the API - removes with_* methods with closures, and replaces generics with trait objects.

Remove the ContinueBuild concept, be lenient about missing files in all cases, and undo the fingerprint changes (not an issue now we're not stopping the build early).

alexcrichton

Looks good! Just two minor nits

alexcrichton · 2017-01-05T17:55:24Z

src/cargo/util/config.rs

@@ -47,7 +47,7 @@ impl Config {
            rustdoc: LazyCell::new(),
            extra_verbose: Cell::new(false),
            frozen: Cell::new(false),
-            locked: Cell::new(false),
+            locked: Cell::new(false),   


trailing whitespace

alexcrichton · 2017-01-05T17:55:32Z

src/cargo/ops/cargo_rustc/fingerprint.rs

+                // if let Ok(meta) = fs::metadata(path) {
+                //     let mtime = FileTime::from_last_modification_time(&meta);
+                //     *slot.0.lock().unwrap() = Some(mtime);
+                // }


commented out?

nrc · 2017-01-05T21:24:41Z

@alexcrichton nits addressed

alexcrichton · 2017-01-05T21:33:00Z

@bors: r+

bors · 2017-01-05T21:33:02Z

📌 Commit 17a0fdc has been approved by alexcrichton

bors · 2017-01-05T22:11:11Z

⌛ Testing commit 17a0fdc with merge 97b698f...

bors · 2017-01-05T22:21:56Z

💔 Test failed - status-travis

nrc · 2017-01-05T22:25:09Z

Updated to fix tidy errors. @alexcrichton please to re-summon bors

alexcrichton · 2017-01-05T22:28:22Z

@bors: retry

sophiajt · 2017-01-06T03:08:15Z

@bors r=alexcrichton

sophiajt · 2017-01-06T03:09:04Z

I apparently don't have sufficient magic powers to resummon the bors :)

alexcrichton · 2017-01-06T06:15:07Z

@bors: r+

oh oops all that and I should have r+'d instead of retry'd

bors · 2017-01-06T06:15:08Z

📌 Commit e50dc1a has been approved by alexcrichton

bors · 2017-01-06T06:15:13Z

⌛ Testing commit e50dc1a with merge afe53bd...

@alexcrichton

Make some aspects of check/build available as an API. There are two key parts to this commit: * let API clients run `cargo check` with minimal fuss (ops/cargo_check.rs), * let API clients intercept and customise Cargo's calls to rustc (all the Executor stuff). r? @alexcrichton

bors · 2017-01-06T07:25:20Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing afe53bd to master...

rust-highfive assigned alexcrichton Dec 19, 2016

nrc added a commit to rust-lang/rls that referenced this pull request Dec 19, 2016

use cargo check via API

aaeec56

requires rust-lang/cargo#3427 closes #82 cc #90

matklad reviewed Dec 19, 2016

View reviewed changes

alexcrichton reviewed Dec 20, 2016

View reviewed changes

nrc force-pushed the api-check-2 branch from 6440007 to 374b60c Compare December 21, 2016 04:34

nrc force-pushed the api-check-2 branch from 374b60c to ed1fc50 Compare December 21, 2016 20:17

alexcrichton reviewed Dec 27, 2016

View reviewed changes

alexcrichton reviewed Dec 29, 2016

View reviewed changes

nrc force-pushed the api-check-2 branch from ed1fc50 to c8bc4bc Compare December 29, 2016 23:21

nrc added 4 commits January 5, 2017 15:58

Make some aspects of check/build available as an API.

8c3b360

There are two key parts to this commit: * let API clients run `cargo check` with minimal fuss (ops/cargo_check.rs), * let API clients intercept and customise Cargo's calls to rustc (all the Executor stuff).

Review changes

69caa63

review changes, round 2

1ab19a5

Mostly focussing on the ergonomics of the API - removes with_* methods with closures, and replaces generics with trait objects.

review changes, round 2 part b

c5619cb

Remove the ContinueBuild concept, be lenient about missing files in all cases, and undo the fingerprint changes (not an issue now we're not stopping the build early).

alexcrichton reviewed Jan 5, 2017

View reviewed changes

nrc force-pushed the api-check-2 branch from 499ca46 to 17a0fdc Compare January 5, 2017 21:24

Add --profile check to cargo rustc

e50dc1a

nrc force-pushed the api-check-2 branch from 17a0fdc to e50dc1a Compare January 5, 2017 22:24

bors merged commit e50dc1a into rust-lang:master Jan 6, 2017

ehuss added this to the 1.16.0 milestone Feb 6, 2022

Make some aspects of check/build available as an API. #3427

Make some aspects of check/build available as an API. #3427

Conversation

nrc commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Dec 21, 2016

nrc commented Dec 21, 2016

nrc commented Dec 27, 2016

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matklad commented Dec 28, 2016 • edited Loading

nrc commented Dec 28, 2016

alexcrichton commented Dec 28, 2016

nrc commented Dec 28, 2016

alexcrichton commented Dec 29, 2016

nrc commented Dec 29, 2016

nrc commented Dec 29, 2016

nrc commented Dec 29, 2016

matklad commented Dec 29, 2016

nrc commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Dec 29, 2016

nrc commented Dec 29, 2016

nrc commented Dec 29, 2016

matklad commented Dec 29, 2016

alexcrichton commented Dec 30, 2016

nrc commented Dec 30, 2016

matklad commented Jan 2, 2017 • edited Loading

nrc commented Jan 3, 2017

nrc commented Jan 3, 2017

alexcrichton commented Jan 3, 2017

nrc commented Jan 3, 2017

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Jan 5, 2017

alexcrichton commented Jan 5, 2017

bors commented Jan 5, 2017

bors commented Jan 5, 2017

bors commented Jan 5, 2017

nrc commented Jan 5, 2017

alexcrichton commented Jan 5, 2017

sophiajt commented Jan 6, 2017

sophiajt commented Jan 6, 2017

alexcrichton commented Jan 6, 2017

bors commented Jan 6, 2017

bors commented Jan 6, 2017

bors commented Jan 6, 2017

matklad commented Dec 28, 2016 •

edited

Loading

matklad commented Jan 2, 2017 •

edited

Loading