Support simple external builds #988

Xanewok · 2018-08-14T10:41:44Z

@nrc this is the implementation of external build we talked about.
What's left is resolving the timeout issue (otherwise RLS will be stuck waiting 'building' if the build command loops) and resolving the pathbuf (de)serialize in rls-data (rustc serializes it with rustc-serialize, added a Deserialize impl for Serde backend deserializing with rustc-serialize format).

Since I worked on integrating this with Buck, here's a simple script that I used:

#!/bin/bash

# $1 - input file
# Output - directories of rebuilt json files
BUCK=~/repos/buck/bin/buck

RULE=$("$BUCK" query "owner($1)")
DEPS=$("$BUCK" query "kind('^rust_.*$', deps($RULE))")
for dep in $DEPS; do
	# Outputs save-analysis file for each dependency lib/bin
	$BUCK build "$dep#save-analysis" --show-output | awk '{print $2}'
done

which for "rust.build_command": "/home/xanewok/.../script.sh server/src/main.rs, for example, outputs:

buck-out/gen/server/server#default,save-analysis/save-analysis/server-e9d875ca101d0939.json
buck-out/gen/project1/project1#default,save-analysis/save-analysis/libproject1-f0ccf02843c7368b.json
buck-out/gen/project2/project2#default,save-analysis/save-analysis/libproject2-a1b98c5ddb92a465.json

As of now, build command does not support diagnostics, but I imagine we could tweak it further and support diagnostics/save-analysis via different message JSON payloads.

@nrc do you think this is good for now as-is?

jsgf · 2018-08-14T18:19:36Z

This looks reasonable to me.

BTW, you can combine some of those buck operations to amortize its startup cost:

DEPS=$("$BUCK" query "kind('^rust_.*$', deps(owner($1)))")

and

$BUCK build --show-output $(for dep in $DEPS; do echo "$dep#save-analysis"; done) | awk '{print $2}'

Xanewok · 2018-08-16T23:00:09Z

r? @nrc

This is somewhat simplistic for now but do you think we could land this?

In addition to supporting diagnostics, at some point in the future it might be a good idea to also pass rustc invocations used in the build itself.

nrc

Comments inline, I think the only major one is about implementing AnalysisLoader.

nrc · 2018-08-24T08:17:53Z

src/config.rs

@@ -221,6 +229,10 @@ impl Config {
            self.build_bin = Inferrable::Inferred(None);
            self.build_lib = Inferrable::Inferred(false);
            self.cfg_test = false;
+            self.build_command = None;
+        } else if !self.build_on_save {


Instead of requiring build_on_save, you could set build_on_save to true if there is a build command

nrc · 2018-08-24T08:30:36Z

src/config.rs

@@ -157,6 +157,13 @@ pub struct Config {
    /// local variable declaration. When set to false, the content is only availabe when
    /// holding the `ctrl` key in some editors.
    pub show_hover_context: bool,
+    /// EXPERIMENTAL (needs unstable features)


It doesn't look like you're enforcing the 'unstable features' requirement

I am resetting the option here if the unstable features are not enabled; am I missing something else?

Are sorry, missed that. Looks fine.

nrc · 2018-08-24T08:32:06Z

src/build/external.rs

+/// to be reloaded by the RLS.
+/// Note: This is *very* experimental and preliminary - this can viewed as
+/// an experimentation until a more complete solution emerges.
+pub(super) fn build_with_external_cmd<S: AsRef<str>>(path: S, build_dir: PathBuf) -> BuildResult {


path is the command line, rather than an actual path, correct? In which case it should have a more accurate name

nrc · 2018-08-24T08:32:48Z

src/build/external.rs

+        },
+    };
+
+    // TODO: Timeout?


I don't think we need to timeout (given how long builds can take, I don't see how we can).

nrc · 2018-08-24T08:33:06Z

src/build/external.rs

+
+    // TODO: Timeout?
+    let reader = std::io::BufReader::new(child.stdout.unwrap());
+    use std::io::BufRead;


Style nit: lift this to module scope.

nrc · 2018-08-24T08:36:48Z

src/build/external.rs

+/// Reads and deserializes given save-analysis JSON files into corresponding
+/// `rls_data::Analysis` for each file. If an error is encountered, a `String`
+/// with the error message is returned.
+fn read_analysis_files<I>(files: I) -> Result<Vec<Analysis>, String>


I think a better approach is to add another AnalysisLoader (see https://github.com/nrc/rls-analysis/blob/master/src/loader.rs) implementation to do the loading, that would keep the reading and deserialising of save-analysis encapsulated in rls-analysis rather than both there and the rls.

I originally didn't go with that, since it seems you can't incrementally load analysis with a different AnalysisLoader - we can only create a fresh one with a different loader.

I imagine we'd have to create new rls-analysis API that would allow us to incrementally load-once with a &dyn AnalysisLoader or allow us to replace the AnalysisHost with a new one, retaining current index, but with a different generic AnalysisLoader?

Hmm, that sounds like a problem with the AnalysisLoader API, could you file an issue with details to the rls-analysis crate please?

This is currently enabled via `build_command` config option. The specified command runs as a build procedure; it should output a list of save-analysis .json files to be reloaded by the RLS so that the usual features, e.g. type info on hover, work. The `build_command` requires `build_on_save` (otherwise RLS needs a lot of dummy rebuilds to sync the files and treat them as fresh)

nrc · 2018-08-30T04:47:10Z

Thanks for the changes!

@nrc

Update RLS and Rustfmt RLS * Allow project model to download crates ([#1020](rust-lang/rls#1020)) * Support simple external builds ([#988](rust-lang/rls#988)) * Support using external Rustfmt ([#990](rust-lang/rls#990)) Rustfmt (0.99.4) * Format chains with comment ([#2899](https://github.com/rust-lang-nursery/rls/pull/2899)) * Do not show wildcard pattern in slice pattern ([#2912](https://github.com/rust-lang-nursery/rls/pull/2912)) * Impl only use ([#2951](https://github.com/rust-lang-nursery/rls/pull/2951)) * ... and [more](rust-lang/rustfmt@5c9a2b6...1c40881) Bumped in tandem to pull a single version of `rustc-ap-*` libs. r? @nrc

Added in rust-lang/rls#988.

damienmg · 2018-12-01T21:17:14Z

@Xanewok: I took a deep look at this change this week to try to integrate with Bazel but I was wondering why go with list of Analysis vs cargo build --build-plan format, especially since the latter seems to be the format of RawInvocation, that is the format read by RLS.

I ask this specifically because the Analysis format does not support env variables nor declaring non-rust file dependencies (e.g. BUILD files or proto files) and it also seems to drop dependencies on crates that are not compiled in this project (i.e. external dependencies).

Added in rust-lang/rls#988.

Xanewok force-pushed the external-build branch from 986a371 to 5ab3df4 Compare August 16, 2018 18:09

Xanewok mentioned this pull request Aug 16, 2018

Integrate with large (non-cargo) build systems #401

Open

Xanewok force-pushed the external-build branch from 5ab3df4 to 22f7b50 Compare August 16, 2018 23:47

nrc reviewed Aug 24, 2018

View reviewed changes

Xanewok added 2 commits August 28, 2018 18:28

build_command now implies build_on_save: true

9bd8d1c

Xanewok force-pushed the external-build branch from 22f7b50 to 9bd8d1c Compare August 28, 2018 16:39

Address most comments

e8b1d04

nrc merged commit 783bfd0 into rust-lang:master Aug 30, 2018

This was referenced Aug 31, 2018

Allow to use AnalysisLoader dynamically with AnalysisHost rust-dev-tools/rls-analysis#149

Open

Update RLS and Rustfmt rust-lang/rust#53870

Merged

Xanewok deleted the external-build branch September 5, 2018 09:21

Xanewok added a commit to Xanewok/rls-vscode that referenced this pull request Sep 5, 2018

Add "rust.build_command" config option

af9b409

Added in rust-lang/rls#988.

Xanewok mentioned this pull request Sep 5, 2018

Add "rust.build_command" config option rust-lang/vscode-rust#421

Merged

Xanewok mentioned this pull request Sep 19, 2018

TockOS subproject errors #1057

Open

dc740 mentioned this pull request Jan 5, 2019

External builds integration eclipse-corrosion/corrosion#135

Open

fj pushed a commit to fj/rls-vscode that referenced this pull request Feb 17, 2019

Add "rust.build_command" config option

0c13c8a

Added in rust-lang/rls#988.

Xanewok mentioned this pull request Feb 25, 2019

Allow to use AnalysisLoader dynamically with AnalysisHost #1375

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support simple external builds #988

Support simple external builds #988

Xanewok commented Aug 14, 2018

jsgf commented Aug 14, 2018

Xanewok commented Aug 16, 2018

nrc left a comment

nrc Aug 24, 2018

Xanewok Aug 28, 2018

nrc Aug 24, 2018

Xanewok Aug 28, 2018

nrc Aug 30, 2018

nrc Aug 24, 2018

nrc Aug 24, 2018

nrc Aug 24, 2018

nrc Aug 24, 2018

Xanewok Aug 28, 2018

nrc Aug 30, 2018

nrc commented Aug 30, 2018

damienmg commented Dec 1, 2018

Support simple external builds #988

Support simple external builds #988

Conversation

Xanewok commented Aug 14, 2018

jsgf commented Aug 14, 2018

Xanewok commented Aug 16, 2018

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

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 Aug 30, 2018

damienmg commented Dec 1, 2018