Add error-chain errors. #4090

jluner · 2017-05-24T04:40:27Z

Convert CargoResult, CargoError into an implementation provided by error-chain. The previous is_human machinery is mostly removed; now errors are displayed unless of the Internal kind, verbose mode will print all errors.

rust-highfive · 2017-05-24T04:40:38Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

jluner · 2017-05-24T04:51:29Z

@alexcrichton As discussed on the issue thread, here's the PR for review.

Currently http_auth_offered fails because it looks like it used the wrong kind of credential.helper. I'll try to figure out how I did that. After that, it looks like custom_build_script_failed had an extra caused-by line, as do two dependency tests. There might be more behind them, but that's where I'm calling it a night.

alexcrichton

Looking great, thanks so much for taking this on @jluner!

cc @matklad

alexcrichton · 2017-05-24T16:12:21Z

src/cargo/ops/cargo_test.rs

@@ -100,7 +101,7 @@ fn run_unit_tests(options: &TestOptions,
            shell.status("Running", cmd.to_string())
        })?;

-        if let Err(e) = cmd.exec() {
+        if let Err(CargoError(CargoErrorKind::ProcessErrorKind(e), .. )) = cmd.exec() {


This looks like it may accidentally ignore all other errors, is that intended?

alexcrichton · 2017-05-24T16:12:57Z

src/cargo/ops/cargo_rustc/mod.rs

@@ -526,7 +527,7 @@ fn add_plugin_deps(rustc: &mut ProcessBuilder,
    let mut search_path = env::split_paths(&search_path).collect::<Vec<_>>();
    for id in build_scripts.plugins.iter() {
        let key = (id.clone(), Kind::Host);
-        let output = build_state.get(&key).chain_error(|| {
+        let output = build_state.get(&key).ok_or_else(|| {
            internal(format!("couldn't find libs for plugin dep {}", id))


Ideally we could get rid of internal and human as function (I think error-chain just supports strings strings in these closures), do you think that'd be feasible? Or maybe happen in a future PR?

alexcrichton · 2017-05-24T16:13:33Z

src/cargo/sources/directory.rs

                human(format!("failed to load checksum `.cargo-checksum.json` \
                               of {} v{}",
                              pkg.package_id().name(),
                              pkg.package_id().version()))

            })?;
-            let cksum: Checksum = serde_json::from_str(&cksum).chain_error(|| {
+            let cksum: Checksum = serde_json::from_str(&cksum)
+                .map_err(CargoError::from).chain_err(|| {


Is this map_err necessary? Or is an extra foreign_links just needed?

alexcrichton · 2017-05-24T16:13:45Z

src/cargo/sources/git/utils.rs

@@ -132,7 +133,7 @@ impl GitRemote {
        }
        fs::create_dir_all(dst)?;
        let repo = git2::Repository::init_bare(dst)?;
-        fetch(&repo, &url, "refs/heads/*:refs/heads/*", cargo_config)?;
+        fetch(&repo, &url, "refs/heads/*:refs/heads/*", cargo_config)?;            


(trailing whitespace)

alexcrichton · 2017-05-24T16:14:02Z

src/cargo/sources/git/utils.rs

@@ -231,20 +232,22 @@ impl<'a> GitCheckout<'a> {
    fn clone_repo(source: &Path, into: &Path) -> CargoResult<git2::Repository> {
        let dirname = into.parent().unwrap();

-        fs::create_dir_all(&dirname).chain_error(|| {
+        fs::create_dir_all(&dirname).map_err(CargoError::from).chain_err(|| {


Similar to above, these map_err instances shouldn't be necessary, right?

alexcrichton · 2017-05-24T16:14:17Z

src/cargo/sources/git/utils.rs

-        let object = self.repo.find_object(self.revision.0, None)?;
-        self.repo.reset(&object, git2::ResetType::Hard, None)?;
+        self.repo.find_object(self.revision.0, None)
+            .and_then(|obj| {self.repo.reset(&obj, git2::ResetType::Hard, None)})?;


Could this use ? instead of and_then?

alexcrichton · 2017-05-24T16:15:00Z

src/cargo/util/errors.rs

-        self().chain_error(callback)
+error_chain! {
+    types {
+        CargoError, CargoErrorKind, CargoResultExt, CargoResult;


I think eventually we'll want to not override the names here, but I like the idea of leaving this here for now to reduce the churn in the PR

alexcrichton · 2017-05-24T16:15:37Z

src/cargo/util/errors.rs

+                    _ => false
+                }
+            },
+            _ => false


Just to make sure we explicitly list it out, mind expanding out this _ to match everything?

alexcrichton · 2017-05-24T16:16:15Z

src/cargo/util/network.rs

+// NetworkError chain
+error_chain!{
+    types {
+        NetworkError, NetworkErrorKind, NetworkResultExt, NetworkResult;


Was it possible to fold this into the main error_chain chain?

jluner · 2017-05-24T16:34:44Z

Some quick responses, and I can try more stuff tonight: I assumed that mapping the error type was necessary to access `chain_err`. I'll have to double-check my expanded macro contents, to see if error-chain's trait gets impl'd in such a way as to permit chaining on arbitrary `Error + 'static`, or just experiment by pulling one out and seeing if it still compiles. cargo_test.rs: The original method returned only that error kind. For future-safety, I should make sure the test appropriately handles all other error kinds, for whatever appropriate means in context. `internal`/`human`: Right now `human` is a wrapper around the `String` error kind, so that's trivially removable. `internal` is the nice constructor for the `Internal` error kind, which is the replacement for `is_human` when determining when to stop printing non-verbose error information. `is_human` is still there, but solely to support feeding `CliError.unknown`, which decides whether to print ¯\_(ツ)_/¯ or the actual error message. These things can be removed, but not without changing cargo's behavior, so my preference is to work it after I make sure I didn't break anything. network.rs: This is the network error type half-removed (my first cut at it had all the git and curl error types only in `NetworkError`). Since `maybe_spurious` is only used in the retry logic itself, I should be able to define that function where it's needed, then collapse the `NetworkError` the rest of the way into `CargoError`.

bors · 2017-05-24T17:44:48Z

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

matklad

Looks awesome, @jluner !

matklad · 2017-05-24T18:50:37Z

src/cargo/ops/cargo_rustc/fingerprint.rs

-        internal(format!("failed to deserialize json"))
-    })?;
+    let old_fingerprint = serde_json::from_str(&old_fingerprint_json)
+        .chain_err(|| {internal(format!("failed to deserialize json"))})?;


{} can be dropped.

matklad · 2017-05-24T18:53:30Z

src/cargo/lib.rs

+            print(err.to_string(), shell);
+        }
+    }
+    else {


Not that it really matters, but usually we format else as } else { in Cargo.

matklad · 2017-05-24T18:59:56Z

src/cargo/util/toml.rs

@@ -1003,7 +1004,8 @@ impl TomlManifest {

            let dep = replacement.to_dependency(spec.name(), cx, None)?;
            let dep = {
-                let version = spec.version().chain_error(|| {
+                let version = spec.version().ok_or_else(||
+                {


The brace probably wants to be on the previous line :)

matklad · 2017-05-24T19:07:13Z

tests/doc.rs

+        assert!(stderr.contains("☃"), "no snowman\n{}", stderr);
+        assert!(stderr.contains("unknown start of token"), "no message\n{}", stderr);
+    }else{
+        assert!(false, "an error kind other than ProcessErrorKind was encountered\n");


No need for \n because we don't show stderr afterwords.

matklad · 2017-05-24T19:20:09Z

src/cargo/lib.rs

    fn print(error: String, shell: &mut MultiShell) {
        let _ = shell.err().say("\nCaused by:", BLACK);
        let _ = shell.err().say(format!("  {}", error), BLACK);
    }
+
+    unsafe fn extend_lifetime(r: &Error) -> &'static Error {


This could have a safer signature:

unsafe fn extend_lifetime(r: &Error) -> &(Error + 'static) { std::mem::transmute::<&Error, &Error>(r) }

It would be nice to add some short comment here, explaining why we need unsafe.

Convert CargoResult, CargoError into an implementation provided by error-chain. The previous is_human machinery is mostly removed; now errors are displayed unless of the Internal kind, verbose mode will print all errors.

alexcrichton · 2017-05-25T03:34:56Z

These things can be removed, but not without changing cargo's behavior, so my preference is to work it after I make sure I didn't break anything

Ok sounds great, we can pursue future refactorings after this PR. This is already a fantastic cleanup as it is :)

* rebased * removed `human` (deferring removing `internal` to a later PR) * cargo_test.rs - fails on other error kinds * unnecessary `map_err(CargoError::from)` removed * fold NetworkError entirely into CargoError * added justification comment for `extend_lifetime` * various formatting goofs The following tests are currently failing: * `http_auth_offered` * `custom_build_script_failed` * `build_deps_for_the_right_arch` * `dep_with_bad_submodule` * `update_with_shared_deps` * `finds_author_email` * `finds_author_user` * `finds_author_user_escaped` * `finds_author_username` * `finds_git_author` * `exit_code`

jluner · 2017-05-25T04:47:37Z

rebased
removed human (deferring removing internal to a later PR)
cargo_test.rs - fails on other error kinds
unnecessary map_err(CargoError::from) removed
fold NetworkError entirely into CargoError
added justification comment for extend_lifetime
various formatting goofs

The following tests are currently failing:

http_auth_offered
custom_build_script_failed
build_deps_for_the_right_arch
dep_with_bad_submodule
update_with_shared_deps
finds_author_email
finds_author_user
finds_author_user_escaped
finds_author_username
finds_git_author
exit_code

I might have done the rebase + push thing wrong? This PR is showing all the commits between my first checkin and tonight, and I don't think it's supposed to work that way.

jluner · 2017-05-25T05:18:43Z

I just cloned rust-lang/cargo/master locally and ran tests, to make sure I don't bang my head against a wall fixing a test I didn't break, and http_auth_offered, build_deps_for_the_right_arch, and the finds_* tests all failed. I assume these are all sensitive to environment somehow (git settings, arch), so I won't worry about them for now, and I'll focus on the other test failures.

alexcrichton · 2017-05-26T14:52:36Z

@jluner oh that's odd, in theory all these tests should be isolated from the environment but we may have forgotten something to keep isolating them!

alexcrichton · 2017-05-26T14:54:37Z

Also feel free to just push to this branch and let CI take care of running tests, it may take a little longer but should get the job done!

jluner · 2017-05-26T15:34:05Z

Dumb question time:

How do I read the CI test results?

I see that AppVeyor is the Windows builds and Travis is non-Windows. Travis shows lots of green entries; did it just skip trying other configurations after the first one built? Also, the travis failure seems to be formatting stuff, so I guess that has to be fixed before it will run tests?

AppVeyor's errors appear to be failing to download something, rather than actually running the tests:

info: downloading component 'rust-std'
error: component download failed for rust-std-x86_64-pc-windows-msvc
info: caused by: could not download file from 'https://static.rust-lang.org/dist/2017-03-03/rust-std-nightly-x86_64-pc-windows-msvc.tar.gz' to 'C:\Users\appveyor\.rustup\downloads\9a9ed6218e17b975ae48735e5c85f224e4a53b0729c7c99d149de19b5d935b43.partial'
info: caused by: error during download
info: caused by: [18] Transferred a partial file (transfer closed with 32046374 bytes remaining to read)
Command exited with code 1

On the topic of formatting: Is there a specific cargo style guide I should reference? I initially tried running cargo fmt on crates-io, and that changed enough things that I figured it would just be noise, so I reverted.

alexcrichton · 2017-05-26T21:32:52Z

Ah yeah no worries, it's not always trivial to do so :)

On PRs we test only one configuration on AppVeyor and Travis, everything else is basically skipped. An error like you're looking at looks like a spurious error so I'll just retry it. The automation basically runs cargo test so you can just poke around the logs and look for the failing command or build (if any)

alexcrichton · 2017-05-26T21:33:32Z

Also nah we don't yet run rustfmt (I wouldn't recommend doing so) but I'd just in general follow the surrounding style. I can comment on any particular places as well

alexcrichton · 2017-05-26T21:36:39Z

So for example:

This is spurious
This is a legit error (looks like an error is now printed instead of being hidden)
This is also a failure failing the one actual style check we have, but it's just for lines over 100 chars

bors · 2017-05-28T03:34:37Z

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

jluner · 2017-05-28T19:10:36Z

Looks like CI is happy. What's the next step to getting error-chain to land before moving onto the next pass?

alexcrichton · 2017-05-30T14:40:42Z

Nice! I'll take a final pass at reviewing and then we can r+ to land this.

alexcrichton

Holy cow I'm blown away this didn't even change the test suite, very nice work! I'm super excited to land this!

I've left some mostly-just-minor comments, but if you've got any questions don't hesitate to ask. And it goes without saying, but thank you so much for taking this on @jluner!

alexcrichton · 2017-05-30T14:41:48Z

src/bin/cargo.rs

@@ -309,7 +311,7 @@ fn execute_external_subcommand(config: &Config, cmd: &str, args: &[String]) -> C
    let command = match path {
        Some(command) => command,
        None => {
-            return Err(human(match find_closest(config, cmd) {
+            return Err(CargoError::from(match find_closest(config, cmd) {


Where possible I tend to find format!(...).into() a little more ergonomic than wrapping with CargoError::from, but that's not always possible due to inference issues.

In many cases the compiler was just not figuring it out. I can do another pass to make sure into doesn't work. Is there a preference between a turbofished into and from?

Ah unfortunately into doesn't work w/ turbofish as the type parameter is in the wrong place, if .into() doesn't work then CargoError::from is fine, no worries!

alexcrichton · 2017-05-30T14:42:25Z

src/bin/cargo.rs

-        Err(CliError::code(code))
-    } else {
-        Err(CliError::new(Box::new(err), 101))
+    if let CargoError(CargoErrorKind::ProcessErrorKind(ref perr), ..) = err {


I think there's a kind method that should be able to extract this, perhaps that could be used here?

alexcrichton · 2017-05-30T14:43:47Z

src/cargo/core/package_id_spec.rs

-            let mut path = url.path_segments().chain_error(|| {
-                human(format!("pkgid urls must have a path: {}", url))
+            let mut path = url.path_segments().ok_or_else(|| {
+                CargoError::from(format!("pkgid urls must have a path: {}", url))


I think maybe .into() would work here? (just as an example)

alexcrichton · 2017-05-30T14:44:28Z

src/cargo/lib.rs

@@ -30,6 +32,8 @@ extern crate url;

 use std::io;
 use std::fmt;
+use std::error::Error;
+use error_chain::ChainedError;


While you're at it mind throwing a newline above this import of error_chain to separate the std and non-std imports?

alexcrichton · 2017-05-30T14:47:14Z

src/cargo/ops/cargo_test.rs

+        let result = cmd.exec();
+
+        match result {
+            Err(CargoError(CargoErrorKind::ProcessErrorKind(e), .. )) => {


Could the kind method be used here?

This one's a little more awkward, because I have to check that the Result is an Err and then check the kind as well. Unpacking it might get verbose. I could if let Some(foo) = result.err() and then if let blah = foo.kind(). Or I suppose I could result.err().map(kind).

Upon review, we need to move the ProcessError out here, but kind gives a borrow.

alexcrichton · 2017-05-30T14:48:02Z

src/cargo/sources/directory.rs

-                              pkg.package_id().name(),
-                              pkg.package_id().version()))
+            let cksum: Checksum = serde_json::from_str(&cksum)
+                .chain_err(|| {


Could this call to chain_err stay on the previous line? As-is the indentation looks a little odd

alexcrichton · 2017-05-30T14:49:56Z

src/cargo/util/errors.rs

-    fn description(&self) -> &str { self.error.description() }
-}
+impl CargoError {
+    pub fn to_internal(self) -> Self {


Could this be called into_internal to signify the transfer of ownership?

alexcrichton · 2017-05-30T14:50:13Z

src/cargo/util/errors.rs

-}
+impl CargoError {
+    pub fn to_internal(self) -> Self {
+        //This is actually bad, because it loses the cause information for foreign_link


Would it be possible to preserve self.0 somehow?

Ignore original comment, I was on the wrong track.

I'll look into preserving self.0 somehow.

alexcrichton · 2017-05-30T14:50:42Z

src/cargo/util/network.rs

+
+fn maybe_spurious(err: &CargoError) -> bool {
+    match err.kind() {
+            &CargoErrorKind::Git(ref git_err) => {


The indentation here looks a little off?

alexcrichton · 2017-05-30T14:51:18Z

src/cargo/util/network.rs

+use git2;
+
+fn maybe_spurious(err: &CargoError) -> bool {
+    match err.kind() {


Could this be modified to iterate over the error to see if anything along the way is spurious? In theory if there's any spurious error in the chain then this operation should be retried

This would need a trick like the one in lib.rs that walks the chain to print. There, I could convince myself that the transmute-to-static trick is safe because the error was moved into the method and consumed entirely during printing. Here, we start with a &CargoError. I don't have the confidence to say that transmuting the chain to static and downcasting here is similarly ok, just because I'm still too new to Rust. It seems like it might be ok? Maybe? I can at least put it in, and people wiser than I can review, and then just pull it back out if it's no good.

Fix some formatting items. Changes Internal error kind to preserve the original error. Changes network retry logic to inspect full error chain for spurious errors.

alexcrichton · 2017-05-31T18:04:53Z

Alright this is looking great! Let's land this and continue to iterate in future PRs.

@bors: r+

bors · 2017-05-31T18:04:54Z

📌 Commit def249f has been approved by alexcrichton

bors · 2017-05-31T18:05:01Z

⌛ Testing commit def249f with merge 705c30b...

bors · 2017-05-31T18:16:07Z

💔 Test failed - status-travis

jluner · 2017-05-31T19:25:02Z

error: failed to run custom build command for `backtrace-sys v0.1.10`

process didn't exit successfully: `/tmp//target/release/build/backtrace-sys-3989d5c503a9238f/build-script-build` (exit code: 101)

Looks like it might be a cross-compilation issue? Or environment? Other jobs got past that successfully.

alexcrichton · 2017-05-31T20:10:10Z

I'll try to land #4113 first which should unblock this by just removing those configurations, they're all tested upstream in rust-lang/rust anyway.

alexcrichton · 2017-05-31T22:51:29Z

@bors: retry

bors · 2017-05-31T22:51:36Z

⌛ Testing commit def249f with merge 03c0a41...

Add error-chain errors. Fixes #4209 Convert CargoResult, CargoError into an implementation provided by error-chain. The previous is_human machinery is mostly removed; now errors are displayed unless of the Internal kind, verbose mode will print all errors.

bors · 2017-05-31T23:24:16Z

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

alexcrichton · 2017-06-01T02:19:54Z

Woo!

Thanks so much again for this @jluner! This is amazing 💖

jluner · 2017-06-01T02:42:26Z

Exciting! For a first contribution experience, this went well (modulo hardware problems locally). My next few questions are:
If I push later changes back up to my fork, does that mess with this pull request, or is it "closed"?
What path forward are you looking for with error changes? Do you have an idea of what "verbose" should mean after cargo prints all error messages by default? Should some errors still be masked with a generic "cargo error occurred" message using the is_human logic that's still in place?

I'm happy to keep contributing on this issue. Judging by my rate of productivity, I may have to end up handing some of it off or pausing work towards the middle or end of June.

alexcrichton · 2017-06-01T04:46:09Z

Oh typically with github and git you'll do development on branches vs the master branch itself, but yeah it's fine to push to your master branch and it won't mess up this PR (I think). It certainly won't auto-affect this repo, so no worries!

The two code-wise changes I could see are:

Move to the idiomatic Result and Error names of error-chain rather than CargoFoo
Move to the idiomatic use util::errors::* rather than selective imports

More broadly though I've long wanted for a deeper revamp with how Cargo treats errors. My basic thinking is that for every possible error you get out of Cargo we know how it'll be printed, so you've got an amount of context that you'll print out and an amount of context that you'll hide. A good example of this is network errors which should never hide anything, whereas if rustc fails we shouldn't print out the rustc command that failed, just that the crate failed (and rustc's output tells you why).

On the other hand though we'll have bugs in Cargo where an error bubbles up that we didn't expect (e.g. an internal error today). In cases like this Cargo should sort of "ICE" but not quite as hard. I'm thinking that Cargo dumps all error context, a pretty severe message about how this error is a bug in Cargo, and maybe something like a data file in target with everything we can find (args, executable, version, fs layout, etc) and asks you to submit that as a bug report to Cargo itself. This way we'll have an avenue for getting bug reports about errors we should make better.

Finally I'm also thinking that all errors log all their context to the target directory if we hide anything. That way if you want to inspect more you can poke around and try to find more traces or something like that.

That's all pretty vague though and not super well thought out in my head. I'm sure others will have thoughts on this that will want to be voiced before any code actually gets written! Overall I've been not super satisfied with how Cargo's error handling story plays out, I fear that it hides too much by default which ends up being a pain :(

Add error-chain errors

b1b6973

Convert CargoResult, CargoError into an implementation provided by error-chain. The previous is_human machinery is mostly removed; now errors are displayed unless of the Internal kind, verbose mode will print all errors.

rust-highfive assigned brson May 24, 2017

alexcrichton reviewed May 24, 2017

View reviewed changes

matklad reviewed May 24, 2017

View reviewed changes

jluner added 2 commits May 24, 2017 21:14

Add error-chain errors

e95044e

Convert CargoResult, CargoError into an implementation provided by error-chain. The previous is_human machinery is mostly removed; now errors are displayed unless of the Internal kind, verbose mode will print all errors.

Merge branch 'master' of https://github.com/jluner/cargo

5d7ac0d

jluner added 5 commits May 27, 2017 02:21

Fix failing tests

2f3955a

Merge remote-tracking branch 'upstream/master'

6e9161b

Update gcc dependency to match updated gcc elsewhere in cargo

d121e05

Fix formatting issue blocking build

3c91570

Merge remote-tracking branch 'upstream/master'

84667a1

jluner added 2 commits May 27, 2017 23:50

Resolve upstream updates to referenced crates

7f6dbc1

Restore macro_use

68f5584

alexcrichton reviewed May 30, 2017

View reviewed changes

Fixes review comments

def249f

Fix some formatting items. Changes Internal error kind to preserve the original error. Changes network retry logic to inspect full error chain for spurious errors.

bors merged commit def249f into rust-lang:master May 31, 2017

This was referenced May 31, 2017

Move API token into the separate file. #3978

Merged

Add a GNU make jobserver implementation to Cargo #4110

Merged

This was referenced Jun 6, 2017

More clear message on "no space left on device" error #4128

Closed

Migrate to error-chain #4029

Closed

mcarton mentioned this pull request Jun 19, 2017

Fix the test suite after cargo update rust-lang/rust-clippy#1839

Merged

ehuss added this to the 1.19.0 milestone Feb 6, 2022

Add error-chain errors. #4090

Add error-chain errors. #4090

Conversation

jluner commented May 24, 2017

rust-highfive commented May 24, 2017

jluner commented May 24, 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

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

jluner commented May 24, 2017 via email • edited Loading

bors commented May 24, 2017

matklad 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

alexcrichton commented May 25, 2017

jluner commented May 25, 2017

jluner commented May 25, 2017

alexcrichton commented May 26, 2017

alexcrichton commented May 26, 2017

jluner commented May 26, 2017

alexcrichton commented May 26, 2017

alexcrichton commented May 26, 2017

alexcrichton commented May 26, 2017

bors commented May 28, 2017

jluner commented May 28, 2017

alexcrichton commented May 30, 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

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

jluner May 30, 2017 • edited Loading

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

alexcrichton commented May 31, 2017

bors commented May 31, 2017

bors commented May 31, 2017

bors commented May 31, 2017

jluner commented May 31, 2017

alexcrichton commented May 31, 2017

alexcrichton commented May 31, 2017

bors commented May 31, 2017

bors commented May 31, 2017

alexcrichton commented Jun 1, 2017

jluner commented Jun 1, 2017

alexcrichton commented Jun 1, 2017

jluner commented May 24, 2017 via email •

edited

Loading

jluner May 30, 2017 •

edited

Loading