window/progress notification #653

algesten · 2018-01-11T13:33:47Z

Attempt to implement window/progress notifications.

Fixes #377

algesten · 2018-01-11T13:35:43Z

There are a bunch of things I need some feedback on. I'll write code comments.

algesten · 2018-01-11T13:37:28Z

Cargo.toml

@@ -14,7 +14,7 @@ cargo = { git = "https://github.com/rust-lang/cargo" }
 env_logger = "0.4"
 failure = "0.1.1"
 jsonrpc-core = "8.0.1"
-languageserver-types = "0.16"
+languageserver-types = { git = "https://github.com/algesten/languageserver-types", branch="progress-params-016" }


This is because the latest languageserver-types needs some work to integrate. I've done this PR against languageserver-types master.

Hmm, given that it might take a while for the next version of the LSP to be released, I would not wait for that and instead use a custom message with an issue and note to use this feature if/when it gets released

The changes for now can be put inside lsp_data.rs

Alright. I make a local version.

algesten · 2018-01-11T13:38:32Z

src/actions/mod.rs

+            }
+
+            ProgressParams {
+                id: format!("progress_{}", ID_COUNTER.fetch_add(1, Ordering::SeqCst)),


Is this ID format enough? Or do we need it to be globally unique, maybe introduce uuid?

How are the ids designed to be used?

As far as I understand they're just to tie together multiple progress notices in one long chain – they have the same ID. Whether or not that ID needs to be globally unique I'm not sure.

algesten · 2018-01-11T13:42:50Z

src/actions/post_build.rs

    fn notify_end(&self);
-    fn notify_publish(&self, PublishDiagnosticsParams);


I've reused this trait to now have notifications for both diagnostics and progress. I've renamed/reorded to make it clear what is what. However it got a bit messier than I wanted. For instance, I left the notifier implementation reference in the PostBuildHandler, which means to notify progress we need a reference to pbh in places like cargo, which isn't great.

Maybe separate into two traits?

The order of the current ones is:

notify_begin starts progress

notify_progress ... repeat for each update

notify_begin_diagnostics

notify_publish_diagnostics

notify_end (ends both diagnostics and progress)

It is unfortunate how much the pbh has to be propagated and would be nice to avoid that, but I don't see how we can do that.

algesten · 2018-01-11T13:45:06Z

src/actions/post_build.rs

+
+pub enum ProgressUpdate {
+    Message(String),
+    Percentage(f64),


Initially I imagined sending a message of what is being built also when we do a percentage. However it seems we can only send either a message OR a percentage. microsoft/vscode-languageserver-node#261 (comment)

algesten · 2018-01-11T13:45:49Z

src/build/cargo.rs


-        if let Err(err) = plan.emplace_dep_with_filter(&unit, &cx, &only_primary) {
+        if let Err(err) = plan.emplace_dep_with_filter(&unit, &cx, &is_primary) {


Hm. This change should be removed...

algesten · 2018-01-11T13:47:41Z

src/build/cargo.rs

+            let progress_sender = self.progress_sender.lock().unwrap();
+            progress_sender.send(ProgressUpdate::Message(format!("{} {}", crate_name, target_name)))
+                .expect("Failed to send progress update");
+        }


Progress in the first (non-cached) build sends messages on the form <crate> <source>, i.e. infer_lib src/lib.rs. Is this a good idea?

I think just the crate name should be enough - the source path is usually obvious from the crate

algesten · 2018-01-11T13:48:41Z

src/build/mod.rs

+            let pbh = build.pbh;
+
+            // window/progress notification that we are about to build
+            pbh.notifier.notify_begin();


As I said above, I guess it's not wonderful the notifier trait impl hangs off pbh for progress updates...

algesten · 2018-01-11T13:59:46Z

It strikes me we don't have any tests for the cached build too see the percentage progress. Would be great to base it on test_multiple_binaries to get more source files and see the percentage.

Any pointers on how to do such a test?

sebastiencs · 2018-01-11T22:38:55Z

Thanks for working on this !
I have tested your branch, here are the notifications I receive when I open a project (with emacs):

23:06:58 [beginBuild]
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’fnv /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/fnv-1.0.5/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’regex_syntax /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/regex-syntax-0.4.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’ansi_term /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/ansi_term-0.9.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’bitflags /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/bitflags-0.8.2/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’void /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/void-1.0.2/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’strsim /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/strsim-0.6.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’log /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/log-0.3.7/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’unicode_segmentation /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/unicode-segmentation-1.2.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’unicode_width /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/unicode-width-0.1.4/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’libc /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.22/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’vec_map /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/vec_map-0.8.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’crossbeam /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/crossbeam-0.2.10/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’utf8_ranges /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/utf8-ranges-1.0.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’lazy_static /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/lazy_static-0.2.8/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’same_file /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/same-file-0.1.3/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’unreachable /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/unreachable-0.1.1/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’memchr /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/memchr-1.0.1/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’atty /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/atty-0.2.2/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’thread_id /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/thread-id-3.0.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’term_size /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/term_size-0.3.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’walkdir /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/walkdir-1.0.7/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’aho_corasick /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/aho-corasick-0.6.3/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’thread_local /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/thread_local-0.3.3/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’clap /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.24.2/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’regex /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/regex-0.2.1/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’globset /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/globset-0.2.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’ignore /home/sebastien/.cargo/registry/src/github.com-1ecc6299db9ec823/ignore-0.2.0/src/lib.rs’
23:07:08 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’fd src/main.rs’
23:07:08 [diagnosticsBegin]
23:07:08 [progress] id: ’progress_1’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’Pending’
23:07:08 [diagnosticsEnd]
23:07:08 [progress] id: ’progress_0’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’
23:07:09 [progress] id: ’progress_1’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’fd src/main.rs’
23:07:09 [diagnosticsBegin]
23:07:09 [diagnosticsEnd]
23:07:09 [progress] id: ’progress_1’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’

A few notes:

The time laps between the notifications rustDocument/beginBuild and the first window/progress is 10 seconds. After the first progress notification I receive all the remainings in less than 1 second (so it's not really usefull from the user POV)
I think the percentage (noted perc in my logs) is something usefull and should be included in the notification
The message includes the library path, as you can see it's not really usefull.
The format of the message could be "Building <crate-name>" ?

algesten · 2018-01-12T03:01:06Z

@sebastiencs Thank you!!!! That's super useful feedback.

The 10 second delay and then all messages at once means I didn't get the threads/messaging right. I fix.

Regarding %, the problem is that first build, RLS delegates the entire build to cargo, and doesn't get an easy way to calculate how many files are left to do. So my first stab just reports which file is being done. However the format with full path is not good.

It would be interesting to know whether you get percentage on the second build, when the deps are cached and RLS runs rustc directly.

Again thanks!

nrc

When we send the last progress update, should we include some message that we're starting the analysis phase?

nrc · 2018-01-12T05:09:38Z

Cargo.toml

@@ -14,7 +14,7 @@ cargo = { git = "https://github.com/rust-lang/cargo" }
 env_logger = "0.4"
 failure = "0.1.1"
 jsonrpc-core = "8.0.1"
-languageserver-types = "0.16"
+languageserver-types = { git = "https://github.com/algesten/languageserver-types", branch="progress-params-016" }


Hmm, given that it might take a while for the next version of the LSP to be released, I would not wait for that and instead use a custom message with an issue and note to use this feature if/when it gets released

nrc · 2018-01-12T05:20:07Z

src/actions/mod.rs

+            }
+
+            ProgressParams {
+                id: format!("progress_{}", ID_COUNTER.fetch_add(1, Ordering::SeqCst)),


How are the ids designed to be used?

nrc · 2018-01-12T05:42:44Z

src/build/cargo.rs

+            let progress_sender = self.progress_sender.lock().unwrap();
+            progress_sender.send(ProgressUpdate::Message(format!("{} {}", crate_name, target_name)))
+                .expect("Failed to send progress update");
+        }


I think just the crate name should be enough - the source path is usually obvious from the crate

nrc · 2018-01-12T05:43:23Z

src/build/cargo.rs

@@ -627,10 +650,19 @@ pub fn make_cargo_config(build_dir: &Path,
    config
 }

-fn parse_arg(args: &[OsString], arg: &str) -> Option<String> {
+fn parse_arg(args: &Vec<&str>, arg: &str) -> Option<String> {


prefer &[&str] to &Vec<&str>

nrc · 2018-01-12T05:46:04Z

src/actions/post_build.rs

    fn notify_end(&self);
-    fn notify_publish(&self, PublishDiagnosticsParams);


It is unfortunate how much the pbh has to be propagated and would be nice to avoid that, but I don't see how we can do that.

algesten · 2018-01-12T23:12:43Z

@nrc @Xanewok executive summary:

I split the Notifier trait into DiagnosticsNotifier and ProgressNotifier. I think this became much cleaner.
Made a local version of ProgressParams.
Use a (new) thread to propagate updates from build/cargo build/plan. Not great, but can't see a way around.
Removed source file from message, keep only crate name.

@sebastiencs if you got time to try again, I believe the updates should arrive in a timely fashion (not 10 seconds later).

sebastiencs · 2018-01-13T07:11:17Z

Glad I can help :)
Notifications arrive as expected:

Logs

08:13:40 [beginBuild]
08:13:40 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’vec_map’
08:13:40 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’regex_syntax’
08:13:40 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’ansi_term’
08:13:40 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’version_check’
08:13:41 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’libc’
08:13:41 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’cfg_if’
08:13:41 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’vec_map’
08:13:41 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’fnv’
08:13:42 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’ansi_term’
08:13:42 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’bitflags’
08:13:42 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’ansi_term’
08:13:42 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’libc’
08:13:43 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’lazy_static’
08:13:43 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’utf8_ranges’
08:13:43 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’bitflags’
08:13:43 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’void’
08:13:43 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’crossbeam’
08:13:43 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’unicode_width’
08:13:44 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’lazy_static’
08:13:44 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’bitflags’
08:13:44 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’winapi’
08:13:44 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’same_file’
08:13:44 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’strsim’ [2 times]
08:13:45 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’build’
08:13:45 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’unicode_width’
08:13:45 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’log’
08:13:45 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’term_size’
08:13:45 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’memchr’ [2 times]
08:13:46 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’num_cpus’
08:13:46 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’atty’
08:13:46 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’nix’
08:13:46 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’unreachable’
08:13:46 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’atty’
08:13:46 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’term_size’
08:13:46 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’walkdir’
08:13:47 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’build_script_build’
08:13:47 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’textwrap’
08:13:47 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’aho_corasick’
08:13:48 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’log’
08:13:48 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’thread_local’
08:13:48 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’textwrap’
08:13:48 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’clap’
08:13:49 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’regex’
08:13:50 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’kernel32’
08:13:50 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’clap’
08:13:52 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’ctrlc’
08:13:52 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’globset’
08:13:53 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’ignore’
08:14:09 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’build_script_build’
08:14:11 [progress] id: ’progress_0’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’fd’
08:14:11 [progress] id: ’progress_0’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’
08:14:12 [diagnosticsBegin]
08:14:12 [progress] id: ’progress_1’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’Pending’
08:14:12 [progress] id: ’progress_1’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’build_script_build’
08:14:12 [diagnosticsEnd]
08:14:13 [progress] id: ’progress_1’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’fd’
08:14:13 [progress] id: ’progress_1’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’
08:14:14 [diagnosticsBegin]
08:14:14 [diagnosticsEnd]
08:14:17 [beginBuild]
08:14:17 [progress] id: ’progress_2’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’Pending’
08:14:17 [beginBuild]
08:14:17 [diagnosticsBegin]
08:14:17 [progress] id: ’progress_2’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’
08:14:18 [beginBuild] [2 times]
08:14:18 [diagnosticsBegin]
08:14:18 [diagnosticsEnd]
08:14:18 [beginBuild]
08:14:18 [diagnosticsBegin]
08:14:18 [diagnosticsEnd] [2 times]
08:14:18 [diagnosticsBegin]
08:14:18 [diagnosticsEnd]
08:14:18 [beginBuild] [2 times]
08:14:18 [diagnosticsBegin]
08:14:18 [diagnosticsEnd]
08:14:18 [beginBuild]
08:14:18 [diagnosticsBegin]
08:14:18 [diagnosticsEnd]
08:14:18 [diagnosticsBegin]
08:14:18 [diagnosticsEnd]
08:14:19 [progress] id: ’progress_9’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’Pending’
08:14:19 [diagnosticsBegin]
08:14:19 [progress] id: ’progress_9’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’
08:14:20 [diagnosticsEnd]
08:14:20 [beginBuild]
08:14:20 [progress] id: ’progress_10’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’Pending’
08:14:20 [beginBuild]
08:14:21 [diagnosticsBegin]
08:14:21 [progress] id: ’progress_10’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’
08:14:21 [beginBuild]
08:14:21 [diagnosticsEnd]
08:14:21 [diagnosticsBegin]
08:14:21 [diagnosticsEnd]
08:14:22 [progress] id: ’progress_12’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’Pending’
08:14:22 [diagnosticsBegin]
08:14:22 [progress] id: ’progress_12’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’
08:14:22 [diagnosticsEnd]
08:14:23 [beginBuild]
08:14:23 [progress] id: ’progress_13’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’Pending’
08:14:23 [beginBuild]
08:14:23 [diagnosticsBegin]
08:14:23 [progress] id: ’progress_13’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’
08:14:24 [diagnosticsEnd]
08:14:24 [progress] id: ’progress_14’ done: ’nil’ perc: ’nil’ title: ’nil’ msg: ’Pending’
08:14:24 [diagnosticsBegin]
08:14:24 [progress] id: ’progress_14’ done: ’  t’ perc: ’1.0’ title: ’nil’ msg: ’nil’
08:14:24 [diagnosticsEnd]

Xanewok · 2018-01-13T07:49:10Z

It’s great to see it working on a real-life example, good job!
One thing I’d like to see removed, with introduction of this generic task progress mechanism, are custom rustDocument/diagnostics{Begin, End} messages. These were designed to do the same thing (indicate progress being made and when it finishes), so we can easily replace that with window/progress messages (same thing goes for rustDocument/beginBuild, I believe).

algesten · 2018-01-13T08:22:33Z

Oh wow @sebastiencs THANKS! That's so cool to see!

I will remove the Pending text. That means the first progress message would have neither message nor percentage, but since your UI indicates something is happening by having the "spinner", it will look better.

@Xanewok happy to remove those events. I do that.

algesten · 2018-01-13T09:50:22Z

@Xanewok the current event order (as can be seen in the tests) is:

            ExpectedMessage::new(Some(0)).expect_contains("capabilities"),
            ExpectedMessage::new(None).expect_contains("beginBuild"),
            ExpectedMessage::new(None).expect_contains("progress"),
            ExpectedMessage::new(None).expect_contains("progress").expect_contains("custom_bin"),
            ExpectedMessage::new(None).expect_contains("progress").expect_contains(r#""done":true"#),
            ExpectedMessage::new(None).expect_contains("diagnosticsBegin"),
            ExpectedMessage::new(None).expect_contains("struct is never used: `UnusedCustomBin`"),
            ExpectedMessage::new(None).expect_contains("diagnosticsEnd"),

When I remove those events, do you expect the order to be: capabilities, progress[], progress[message/percentage], publishDiagnostics, progress[done:true]?

I.e. the final progress comes after the publishDiagnostics.

algesten · 2018-01-13T11:38:11Z

After chatting to @Xanewok on IRC, I've replaced the RLS specificbeginBuild diagnosticsBeging and diagnosticsEnd to use only progress.

Each build will result in two sequential "progress-chains", first one with title:"Build", and then one with title:"Diagnostics". The sequence is documented in contributing.md as:

window/progress: notification, title: "Build". Sent before build starts.
window/progress: notification with title: "Build", repeated for each compile target.
- When total amount of work is not known, has field message set to the current crate name.
- When total amount of work is known, has field percentage set to how much of build has started.
window/progress: notification, title: "Build", "done": true. Sent when build ends.
window/progress: notification, title: "Diagnostics". Sent before analysis of build starts.
... standard LSP publishDiagnostics
window/progress: notification, title: "Diagnostics", "done": true. Sent when analysis ends.

algesten · 2018-01-13T11:39:25Z

The two chains will have unique id fields. I.e. There's currently no way of knowing which build-chain relates to which diagnostics-chain.

Xanewok · 2018-01-13T14:27:34Z

src/build/plan.rs

@@ -352,7 +358,19 @@ impl JobQueue {
                .map(|x| x.into_string().unwrap())
                .collect();

-            args.insert(0, job.get_program().clone().into_string().unwrap());


Why was this line split into let program = ... and args.insert(0, program); with progress update in between?

It's a remnant from when I sent the program also as part of the percentage message in a previous iteration. I'll restore it as it was.

I want to do a git rebase once we've iterated this enough to make the commit log more sensible.

Xanewok · 2018-01-13T14:30:57Z

src/actions/post_build.rs

@@ -76,16 +76,16 @@ impl PostBuildHandler {
                        t.unpark();
                    }

-                    self.notifier.notify_end();


I think we can call self.notifier.notify_begin_diagnostics() self.notifier.notify_end_diagnostics() only here, in case of BuildResult::Success. Since the diagnosticsBegin/End changed meaning some time ago and only indicate indexing operation progress, I think it's fair only to do that when we have something meaningful to do.

Xanewok · 2018-01-13T14:34:00Z

src/lsp_data.rs

+
+    // Optional progress message to display.
+    // If unset, the previous progress message (if any) is still valid.
+    pub message: Option<String>,


It'd be good to add #[serde(skip_serializing_if = "Option::is_none")] here not to send unset/irrelevant values to client

Fair enough

Xanewok · 2018-01-13T14:34:09Z

src/lsp_data.rs

+
+    // Optional progress percentage to display.
+    // If unset, the previous progress percentage (if any) is still valid.
+    pub percentage: Option<f64>,


Same as above

Xanewok · 2018-01-13T14:34:16Z

src/lsp_data.rs

+
+    // Set to true on the final progress update.
+    // No more progress notifications with the same ID should be sent.
+    pub done: Option<bool>,


Xanewok · 2018-01-13T14:45:58Z

I can't shake off a feeling that there's a simpler solution waiting to come out in terms of how we pass and manage progress senders, but I can't come up with anything better, so I'll stick with that.

In #654 we also introduced integration tests and the tests themselves are at tests/tests.rs. Could you also modify those to reflect the changes made? (as CI is failing because of it)

algesten · 2018-01-13T14:46:27Z

@Xanewok that's done.

Xanewok · 2018-01-13T22:48:43Z

macOS job is canceled, retrying (there's been some troubles with macOS CI lately so it'd be good to see it greenlit)

Xanewok

The tests now pass and it's working as intended, however I believe it'd be good to separate introduced traits and implementations to a separate, common place.

Xanewok · 2018-01-14T13:14:31Z

src/actions/mod.rs

-        struct BuildNotifier<O: Output> {
+
+        lazy_static! {
+            static ref PROGRESS_ID_COUNTER: AtomicUsize = {


I think the counter, along with traits implementations using it would benefit from separating into a common file src/actions/progress.rs?

Xanewok · 2018-01-14T13:17:36Z

src/actions/mod.rs

+            };
+        }
+
+        fn new_progress_params(id: String, title: &str) -> ProgressParams {


As mentioned above, I believe this should also be moved together with PROGRESS_ID_COUNTER. When we use this helper function we always generate a new ID atomically, so I'd suggest doing that inside the function instead (so we wouldn't need id: String parameter) and accept title: String if we always take ownership of it

Xanewok · 2018-01-14T13:21:52Z

src/actions/mod.rs

-            fn notify_begin(&self) {
-                self.out.notify(Notification::<DiagnosticsBegin>::new(NoParams {}));
+        // base progress parameters for notification of the analysis.
+        let diganostics_progress_params = {


In general, I would avoid interleaving the type definitions and implementations with variable definitions like done here (struct {...} let = {...}; impl {...}) for clarity, but with moving the traits and/or impls elsewhere, this will no longer be a problem here

Xanewok · 2018-01-14T13:23:26Z

src/actions/mod.rs

-                self.out.notify(Notification::<DiagnosticsBegin>::new(NoParams {}));
+        // base progress parameters for notification of the analysis.
+        let diganostics_progress_params = {
+            let id = format!("diagnostics_{}", PROGRESS_ID_COUNTER.fetch_add(1, Ordering::SeqCst));


re generating ID inside fn new_progress_params, I'd stick to just generating a number, since it's more of a implementation detail and we already distinguish using progress titles

Xanewok · 2018-01-14T13:29:17Z

src/actions/mod.rs

            out: O,
            active_build_count: Arc<AtomicUsize>,
+            progress_params: ProgressParams,


This is always used as a template that's cloned for each message, so it'd be good to either add a doc comment explaining that or storing only relevant progress ID here

Xanewok · 2018-01-14T13:30:48Z

src/actions/mod.rs

+        }
+
+        // notifier of progress for the build (window/progress notifications)
+        struct BuildProgressNotifier<O: Output> {


Similarly to BuildDiagnosticsNotifier, this would probably benefit from moving it together to a separate place

Xanewok · 2018-01-14T13:31:35Z

src/actions/mod.rs

@@ -356,6 +421,16 @@ impl<'ctx> FileWatch<'ctx> {
    }
 }

+pub trait ProgressNotifier: Send {


These defs would also need moving

Xanewok · 2018-01-14T13:32:41Z

src/actions/post_build.rs

-    fn notify_begin(&self);
-    fn notify_end(&self);
-    fn notify_publish(&self, PublishDiagnosticsParams);
+pub trait DiagnosticsNotifier: Send {


Same with this trait

Xanewok · 2018-01-14T13:36:51Z

src/build/plan.rs

+            // FIXME. We could communicate the "program" being built here, but
+            // it seems window/progress notification should have message OR percentage.
+            {
+                let percentage = compiler_messages.len() as f64 / self.0.len() as f64;


Since we're dividing by vector length here, it also might be a good idea to change the above assert!(self.0.is_empty() == false); to equivalent assert!(self.0.len() > 0) to better see that it's not 0 at a quick glance

Xanewok · 2018-01-14T13:38:23Z

tests/tests.rs

@@ -44,10 +44,12 @@ fn test_infer_bin() {

        rls.expect_messages(&[
            ExpectedMessage::new(Some(0)).expect_contains("capabilities"),
-            ExpectedMessage::new(None).expect_contains("beginBuild"),
-            ExpectedMessage::new(None).expect_contains("diagnosticsBegin"),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains("Build"),


It's not really important, but could we change it to check against "title": "Build" for clarity? (and for others below)

Xanewok

Thanks for working on this and for quick iterations! 🙂
I'm happy with what we currently have here. Since we have used rustDocument/diagnostics{Begin, End} for a long time now and the change, despite being conceptually simple, is rather significant, I'd also like for @nrc to re-review it.

Xanewok · 2018-01-14T17:25:59Z

src/actions/post_build.rs

                });
            }
            BuildResult::Squashed => {
                trace!("build - Squashed");
-                self.notifier.notify_end();
+                self.active_build_count.fetch_sub(1, Ordering::SeqCst);


I feel like we shouldn't have to manually decrease the count here (possibly by passing the guard value only in BuildResult::Success?), but that's outside of the scope of the PR now.

Xanewok · 2018-01-14T17:29:27Z

src/actions/progress.rs

+/// the RLS (and on to the client).
+// This trait only really exists to work around the object safety rules (Output
+// is not object-safe).
+pub trait DiagnosticsNotifier: Send {


One idea to deduplicate the traits and impl logic might be to introduce a RAII guard that both ensures that we have an active windows/progress session that we finish it with "done": true, which also exposes a way to report specific progress for it (be it publishing diagnostics or sending a progress message), but that'd be mostly exploratory - I'm fine with what we currently have here.

Xanewok · 2018-01-16T23:55:08Z

@algesten I'm afraid another PR caused a merge conflict - could you please resolve those and rebase this?

algesten · 2018-01-17T18:20:57Z

I fix later.

nrc · 2018-02-19T05:24:36Z

All looks good to me!

@algesten sorry, this needs rebasing again. Could you ping me or Xanewok when you do so we can merge please (GitHub doesn't notify on rebases).

algesten · 2018-02-19T07:47:05Z

I fix

algesten · 2018-02-19T22:32:25Z

src/test/mod.rs

-                .expect_contains(r#"[{"language":"rust","value":"&str"}]"#),
+            ExpectedMessage::new(None).expect_contains("contents"),
+            ExpectedMessage::new(None).expect_contains("progress").expect_contains(r#"title":"Diagnostics""#),
+            // XXX why don't we get this?


The rebase is done. But I couldn't figure out why this test changed. I might have broken something.

Is there another progress message which appears before the hover message? Could you post the actual and expected output as produced by the test please?

algesten · 2018-02-19T22:33:42Z

@nrc that's done. one problem, see comment above.

algesten · 2018-02-24T10:27:55Z

@nrc that test error was indeed a bug that is now fixed.

now it's failing on test_deglob, but i'm not convinced that is to do with my code changes. it seems the codeAction message for the line of deglobbing yields an empty result[] array, i.e. there are no code actions for the array.

algesten · 2018-02-24T10:58:50Z

but then master doesn't have this issue. so. sigh. i fix.

algesten · 2018-02-24T11:17:54Z

@nrc there. i think we're set to land this. tests are passing 🎉

Xanewok · 2018-02-24T19:27:54Z

i686 panicked on one test, retriggering CI to see if it's spurious

thread 'test::test_all_targets' panicked at 'assertion failed: `(left == right)`
  left: `11`,
 right: `10`', src\test\harness.rs:223:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
    test::test_all_targets
test result: FAILED. 42 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--bin rls'
Command exited with code 101

nrc · 2018-02-24T23:44:39Z

Looks like it was spurious. Merged! Thanks for working on this @algesten !

algesten · 2018-02-25T07:05:55Z

🎉

algesten commented Jan 11, 2018

View reviewed changes

nrc reviewed Jan 12, 2018

View reviewed changes

Xanewok reviewed Jan 13, 2018

View reviewed changes

algesten force-pushed the progress-params branch 2 times, most recently from aaf03bf to 658d0f8 Compare January 13, 2018 15:11

algesten mentioned this pull request Jan 13, 2018

accept window/progress instead of beginBuild/diagnosticEnd rust-lang/vscode-rust#231

Merged

Xanewok closed this Jan 13, 2018

Xanewok reopened this Jan 13, 2018

algesten mentioned this pull request Jan 14, 2018

Propagate build errors using window/showMessage #657

Merged

Xanewok suggested changes Jan 14, 2018

View reviewed changes

Xanewok approved these changes Jan 14, 2018

View reviewed changes

algesten force-pushed the progress-params branch from 3719158 to 1d404c3 Compare January 17, 2018 20:21

Xanewok mentioned this pull request Feb 19, 2018

Add a way to tell a rls build was successful #723

Open

algesten force-pushed the progress-params branch from 1d404c3 to 8ab908b Compare February 19, 2018 22:30

algesten commented Feb 19, 2018

View reviewed changes

algesten force-pushed the progress-params branch from 8ab908b to 0529d91 Compare February 24, 2018 10:25

Use window/progress notifications for build progress

8bab552

algesten force-pushed the progress-params branch from 65b1747 to 8bab552 Compare February 24, 2018 11:19

Xanewok closed this Feb 24, 2018

Xanewok reopened this Feb 24, 2018

nrc merged commit 870d294 into rust-lang:master Feb 24, 2018

algesten deleted the progress-params branch February 25, 2018 07:08

alexheretic mentioned this pull request Feb 26, 2018

Adapt to Rls window/progress messages rust-lang/atom-ide-rust#59

Closed

		fn notify_end(&self);
		fn notify_publish(&self, PublishDiagnosticsParams);


		if let Err(err) = plan.emplace_dep_with_filter(&unit, &cx, &only_primary) {
		if let Err(err) = plan.emplace_dep_with_filter(&unit, &cx, &is_primary) {

window/progress notification #653

window/progress notification #653

Conversation

algesten commented Jan 11, 2018 • edited Loading

algesten commented Jan 11, 2018

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

algesten commented Jan 11, 2018

sebastiencs commented Jan 11, 2018

algesten commented Jan 12, 2018 • edited Loading

nrc 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

algesten commented Jan 12, 2018

sebastiencs commented Jan 13, 2018 • edited Loading

Xanewok commented Jan 13, 2018

algesten commented Jan 13, 2018

algesten commented Jan 13, 2018

algesten commented Jan 13, 2018

algesten commented Jan 13, 2018

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

Xanewok commented Jan 13, 2018

algesten commented Jan 13, 2018

Xanewok commented Jan 13, 2018

Xanewok 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

Xanewok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok commented Jan 16, 2018

algesten commented Jan 17, 2018

nrc commented Feb 19, 2018

algesten commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algesten commented Feb 19, 2018

algesten commented Feb 24, 2018

algesten commented Feb 24, 2018

algesten commented Feb 24, 2018

Xanewok commented Feb 24, 2018

nrc commented Feb 24, 2018

algesten commented Feb 25, 2018

algesten commented Jan 11, 2018 •

edited

Loading

algesten commented Jan 12, 2018 •

edited

Loading

sebastiencs commented Jan 13, 2018 •

edited

Loading