Allow project model to download crates #1020

kngwyu · 2018-08-27T08:48:11Z

matklad

LGTM!

I wonder if the racer might have the same issue? I've originally copied this code from racer, but maybe I've messed up smth along the way :D

matklad · 2018-08-27T08:58:03Z

src/project_model.rs

@@ -20,6 +20,7 @@ use racer;

 #[derive(Debug)]
 pub struct ProjectModel {
+    manifest_to_id: HashMap<PathBuf, Package>,


kngwyu · 2018-08-27T09:29:27Z

@matklad
Racer had the same issue(already resolved) , but I didn't feel it so critical for emacs-racer + flycheck setting(maybe because flycheck calls cargo metadata so frequently?).
Anyway I learned how difficult to support multiple environments is 😓

kngwyu · 2018-08-27T09:31:32Z

Now I noticed racer sometimes throws package query for stdlib, which causes unintended behavior.
So I decided to include racer update to this PR.

matklad · 2018-08-27T09:41:02Z

(maybe because flycheck calls cargo metadata so frequently?).

Yep, cargo metadata does downloading, so this should mask the issue. IIRC, the main reason to use in-process Cargo was precisely avoiding the downloading. If that didn't work out and folks are not complaining about flycheck downloading stuff, perhaps we should switch to cargo-metadata-based model and out-of-process Cargo?

kngwyu · 2018-08-27T09:50:56Z

@matklad

the main reason to use in-process Cargo was precisely avoiding the downloading

I think it's because @nrc said it's not good to allow other process to write Cargo.lock.
But, if we can write Cargo.lock, cargo metadata seems better.

matklad · 2018-08-27T12:56:04Z

src/project_model.rs

@@ -22,6 +22,7 @@ use racer;
 pub struct ProjectModel {
    manifest_to_id: HashMap<PathBuf, Package>,
    packages: Vec<PackageData>,
+    current_lib: Option<(String, PathBuf)>,


current_lib seems like it shouldn't be here ideally? Like, project model is for worksapce, and workspace contains many libraries, none of which is primary or current.

Perhaps some other code should be adjusted to specify the package?

I changed to just have all package's name, so that we can match libname to current package name.

To reduce compile time in CI

kngwyu · 2018-08-27T13:32:29Z

I think now it's ready.
Finally this PR includes

Allow project_model to download crates
Enable completion in tests/example/etc.. directory
Update racer
Also update rustfmt(to reduce compile time)

matklad · 2018-08-27T13:40:10Z

src/project_model.rs

@@ -28,8 +29,7 @@ pub struct Package(usize);

 #[derive(Debug)]
 struct PackageData {
-    manifest: PathBuf,
-    lib: Option<PathBuf>,
+    lib: Option<(PathBuf, String)>,


This String doesn't feel exactly right: package itself does not really know its library name: the same pacakge might be used with different names by different dependencies:

# one crate [dependecies] bar = { package = "foo"} # another [dependecies] baz = { package = "foo"}

Here, foo's library is known under the name of bar or baz, depending on which package are you asking.
If I understand correctly, we use this mainly for self-dependneices in tests. Perhaps we should adjust Cargo to always include self-dependency? Will try to look into that, but, until than, current solution is actually fine!

OMG, I didn't know cargo is such a complex tool 😲

Xanewok

Looks good, thanks! Left some comments inline.

Xanewok · 2018-08-27T20:04:01Z

src/project_model.rs

-        // frozen=true, locked=true
-        config.configure(0, Some(true), &None, true, true, &None, &[])?;
-        let ws = Workspace::new(&manifest, &config)?;
+        // frozen = false, locked = false


Note: It's definitely not making things any more worse/more complicated, since we currently https://github.com/rust-lang-nursery/rls/blob/master/src/build/cargo.rs#L572 a Cargo config with defaults (frozen, locked = false) as well.

Xanewok · 2018-08-27T20:04:05Z

src/project_model.rs

                    .find(|t| t.is_lib())
-                    .map(|t| t.src_path().to_owned()),
+                    .map(|t| (t.src_path().to_owned(), t.name().replace('-', "_"))),


Is this mapping performed because we get a Cargo package name and somehow need to map it to a rust crate name, as Racer would expect to? Could you please add a note why the conversion is necessary?

Is this mapping performed because we get a Cargo package name and somehow need to map it to a rust crate name, as Racer would expect to?

Yes.
I'm going to add some comments.

Xanewok · 2018-08-27T22:00:12Z

@matklad re Cargo.lock writing, is it just that the lockfile is created when absent if one executes cargo metadata (and not somehow updated/overwritten)?

matklad · 2018-08-27T22:04:02Z

@Xanewok cargo metadata uses the usual logic for lockfile update: create if does not exist, if exists, update if Cargo.toml has newer dependencies.

Xanewok · 2018-08-27T23:34:15Z

Okay, thanks. While it seems sensible to do something when there's a mismatch between a lockfile and the manifest, I'm not sure about the part where our Cargo.lock can be overwritten without us knowing - I'll leave it to @nrc to resolve that since he originally voiced concern over that.

@kngwyu meanwhile, if it's something that is reproducible with local path crate dependencies (not sure if we can/should download stuff during Rust CI run), could you also set up a simple regression test under tests/ to see if the external crate autocompletion works?
It'd be great to start building a regression test suite for the 1.0 milestone. 👍

kngwyu · 2018-08-28T05:25:00Z

I added a test.
It works, but since it's my first time to use LSP, I really want my test code reviwed 🙂

matklad · 2018-08-28T07:27:26Z

FWIW, IntelliJ Rust calls cargo metadata liberally and almost everyone is content with that. Some people even like the fact that as soon as you've edited Cargo.toml, lockfile is updated and crates are downloaded.

There was one person who complained about that, so we've added an "auto sync" checkbox for them.

Xanewok · 2018-08-28T17:00:08Z

It'd be great to have this now and we currently use cargo metadata anyway for the build plan package mapping, so I'll merge this. Thanks @kngwyu!

Later on we should vet against and provide an option to enable networking but I'd rather we have deps autocompletion.

EDIT: This mostly refers to Cargo.lock overwriting instead (my bad), however the networking access is still something we might want to think about in the future.

@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

Allow project model to download crates

54e5273

matklad reviewed Aug 27, 2018

View reviewed changes

kngwyu changed the title ~~Allow project model to download crates~~ [WIP] Allow project model to download crates Aug 27, 2018

kngwyu added 3 commits August 27, 2018 20:33

Enable completion in test/example dir

bf66a15

Update racer

1efd7c0

Convert - -> _ in libname

caf2427

matklad reviewed Aug 27, 2018

View reviewed changes

kngwyu added 2 commits August 27, 2018 21:59

Update rustfmt

746ffb5

To reduce compile time in CI

Change the way to complete self-crate name in tests dir

2ae5662

kngwyu changed the title ~~[WIP] Allow project model to download crates~~ Allow project model to download crates Aug 27, 2018

matklad reviewed Aug 27, 2018

View reviewed changes

Xanewok reviewed Aug 27, 2018

View reviewed changes

kngwyu added 2 commits August 28, 2018 14:18

Add a regression test for completion in test dir

1352bdf

Add a line comment about - to _ conversion

a98c9a0

alexheretic mentioned this pull request Aug 28, 2018

Automatically reload RLS when Cargo.toml changes, or when the git branch changes rust-lang/atom-ide-rust#98

Closed

Xanewok merged commit e926d49 into rust-lang:master Aug 28, 2018

kngwyu deleted the racer-pm-fix branch August 29, 2018 02:48

Xanewok mentioned this pull request Aug 31, 2018

Update RLS and Rustfmt rust-lang/rust#53870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow project model to download crates #1020

Allow project model to download crates #1020

kngwyu commented Aug 27, 2018

matklad left a comment

matklad Aug 27, 2018

kngwyu commented Aug 27, 2018 •

edited

Loading

kngwyu commented Aug 27, 2018 •

edited

Loading

matklad commented Aug 27, 2018

kngwyu commented Aug 27, 2018 •

edited

Loading

matklad Aug 27, 2018

kngwyu Aug 27, 2018

kngwyu Aug 27, 2018 •

edited

Loading

kngwyu commented Aug 27, 2018

matklad Aug 27, 2018

kngwyu Aug 27, 2018

Xanewok left a comment

Xanewok Aug 27, 2018 •

edited

Loading

kngwyu Aug 28, 2018

Xanewok Aug 27, 2018

kngwyu Aug 28, 2018

Xanewok commented Aug 27, 2018

matklad commented Aug 27, 2018

Xanewok commented Aug 27, 2018

kngwyu commented Aug 28, 2018

matklad commented Aug 28, 2018

Xanewok commented Aug 28, 2018 •

edited

Loading

Allow project model to download crates #1020

Allow project model to download crates #1020

Conversation

kngwyu commented Aug 27, 2018

matklad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kngwyu commented Aug 27, 2018 • edited Loading

kngwyu commented Aug 27, 2018 • edited Loading

matklad commented Aug 27, 2018

kngwyu commented Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kngwyu Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

kngwyu commented Aug 27, 2018

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

Xanewok Aug 27, 2018 • 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

Xanewok commented Aug 27, 2018

matklad commented Aug 27, 2018

Xanewok commented Aug 27, 2018

kngwyu commented Aug 28, 2018

matklad commented Aug 28, 2018

Xanewok commented Aug 28, 2018 • edited Loading

kngwyu commented Aug 27, 2018 •

edited

Loading

kngwyu commented Aug 27, 2018 •

edited

Loading

kngwyu commented Aug 27, 2018 •

edited

Loading

kngwyu Aug 27, 2018 •

edited

Loading

Xanewok Aug 27, 2018 •

edited

Loading

Xanewok commented Aug 28, 2018 •

edited

Loading