Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

RLS only compiles the first member of a Cargo workspace #876

Closed
jnicholls opened this issue May 20, 2018 · 15 comments · Fixed by rust-lang/cargo#5830 or #969
Closed

RLS only compiles the first member of a Cargo workspace #876

jnicholls opened this issue May 20, 2018 · 15 comments · Fixed by rust-lang/cargo#5830 or #969
Labels
Milestone

Comments

@jnicholls
Copy link

I open my Cargo workspace into VSCode (just as a directory, not as an official project or workspace or anything) and RLS will compile and check all members of the Cargo workspace as its first action. However after that, with code changes et al, only the first member of the workspace is inspected. That is, I only see in the bottom left status the first member get recompiled, but not any of the other members including the ones into which I am writing intentional syntax errors to test :) If I do syntax errors in the first member, RLS picks them up and reports them accordingly.

I am also seeing this output regularly:

thread 'request-worker-1' panicked at 'called `Result::unwrap()` on an `Err` value: DoesNotExist("undefined/lib/rustlib/src/rust/src")', libcore/result.rs:945:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
thread 'request-worker-4' panicked at 'Once instance has previously been poisoned', libstd/sync/once.rs:315:21
thread 'request-worker-2' panicked at 'Once instance has previously been poisoned', libstd/sync/once.rs:315:21

I get those request-worker-N panicked messages a lot, every time I type and RLS tries to do type completion it seems.

Also these every time I type:

error: expected one of `,` or `as`, found `::`
 --> bogofile:2:13
  |
2 | 	collections::HashMap,
  | 	           ^^

error: expected one of `,` or `as`, found `::`
 --> bogofile:2:13
  |
2 | 	collections::HashMap,
  | 	           ^^

error: expected one of `,` or `as`, found `::`
 --> bogofile:2:13
  |
2 | 	collections::HashMap,
  | 	           ^^

error: expected one of `,` or `as`, found `::`
 --> bogofile:2:13
  |
2 | 	collections::HashMap,
  | 	           ^^

error: expected one of `,` or `as`, found `::`
 --> bogofile:2:13
  |
2 | 	collections::HashMap,
  | 	           ^^

error: expected one of `,` or `as`, found `::`
 --> bogofile:2:13
  |
2 | 	collections::HashMap,

The RLS that is running is from the nightly toolchain, installed today. Any ideas why I would be seeing these issues? The workspace itself is not that large or special. I am surprised these problems aren't being seen by others as they seem fundamental and are happening on a fresh install.

@jnicholls
Copy link
Author

I'm seeing great success with the vscode-rust plugin in legacy mode (just racer), running a master branch build of racer with the rustc-ap-syntax updates (which appears to work well with nested use imports). Other than type inference, it's very good w/ context sensitive help on token hover, go to..., and autocomplete.

@nrc
Copy link
Member

nrc commented May 20, 2018

The bogofile errors are not important - we deal with them internally. They are fixed on master where we've updated to the latest Racer (the rustc_ap_syntax version).

I'm not sure about the panics, could you run with RUST_BACKTRACE=1 so we can get more detail please?

The workspace issues are worrying and interesting. We should be checking all workspaces which are changed. Could you tell me a bit more about the project structure please? Does it look like a or b or something else:

a:
Project
  |
  |- workspace 1
  |- workspace 2
b:
Project
  |
  |- src
  |- workspace 1
  |- workspace 2

Are there things like multiple binaries, redirected directories, build scripts, etc? When you build the project outside of the RLS, what commandline do you use (just cargo build, or with some arguments)? And which directory do you build in?

Which directory are you opening in VSCode? The top-level (project) directory, one of the workspace directories, or one of the src directories?

@jnicholls
Copy link
Author

jnicholls commented May 20, 2018

Yes I will get a backtrace for you. And I realize I gave very little information, apologies...I was about to do so. But I have never had success with RLS even with a simple project, but that was several months ago and I know a lot of fixes have happened since then.

The structure is more like a, except I would relabel Project as the Cargo workspace and workspace <n> as member <n> or crate <n>. It looks like this:

 root directory
├─  library-crate-1
│  ├─  src/lib.rs
│  └─  Cargo.toml
├─  services
│  ├─  service-crate-1
│  │  ├─  src/main.rs
│  │  └─  Cargo.toml
│  └─  service-crate-2
│     ├─  src/main.rs
│     └─  Cargo.toml
├─  utilities
│  ├─  src
│  │  └─  bin
│  │     ├─  utility1.rs
│  │     └─  utility2.rs
│  └─  Cargo.toml
└─  Cargo.toml

and the root Cargo.toml looks like this:

[workspace]
members = [
	"library-crate-1",
	"services/services-crate-1",
	"services/services-crate-2",
	"utilities",
]

There are multiple binaries as you can see. No redirected directories, but all of the services crates and utilities binaries have a dependency on library-crate-1. No build scripts within any of these crates, yet (but of course their crates.io deps have build scripts).

When I build the project, I build typically from the root directory and specify --package if I'm targeting a specific crate. I either run cargo check --package services-crate-1 or a cargo build equivalent when I need the linked binary.

I open the root directory in VSCode, e.g. cd /path/to/root_directory && code . and I never open up VSCode into a sub-crate's directory.

Hope this helps.

@jnicholls
Copy link
Author

Should I be putting a [workspace] Cargo.toml in the services directory for the two crates within that folder? I do not have a problem on the CLI with cargo recognizing the current setup, but maybe the members with a greater-than-1 folder depth is throwing off RLS? Just spit balling here.

@jnicholls
Copy link
Author

Not that this should change the behavior I am seeing, but I am about to reorganize the utilities crate to be a folder like services with a binary crate per utility, so they don't have to build altogether and share deps.

@nrc
Copy link
Member

nrc commented May 20, 2018

Should I be putting a [workspace] Cargo.toml in the services directory for the two crates within that folder?

You shouldn't have to do anything different for the RLS vs the CLI, as long as you are running the RLS in the same directory you build in (which it sounds like you are). This might be why the RLS is getting stuck though.

Thanks for the additional info!

@nrc
Copy link
Member

nrc commented May 20, 2018

And one more question: when you are seeing the RLS build the "first" workspace but not building the modified workspaces, which are they in your diagram?

@jnicholls
Copy link
Author

It builds the library-crate-1 and stops there, doesn’t continue onward to build the, e.g., service-crate-1 which had its main.rs modified.

@nrc
Copy link
Member

nrc commented May 23, 2018

Is the repo you're building public? If so, could I get a URL please?

alexheretic added a commit to alexheretic/rusttmp that referenced this issue Jun 19, 2018
alexheretic added a commit to alexheretic/rusttmp that referenced this issue Jun 19, 2018
@alexheretic
Copy link
Member

alexheretic commented Jun 19, 2018

I've also been seeing this issue with workspace projects.

For example a project with a lib workspace dependency, and a main binary.

  • Start rls
  • Shows all diagnostics from main & workspace lib project
  • Change something in lib
  • No longer shows main diagnostics, including errors that may have been introduced there.

You can only see the new main diagnostics after a change to main or restarting rls.

I created https://github.com/alexheretic/rusttmp/tree/rls-876 to provide a simple way to reproduce this.

STR

  • git clone https://github.com/alexheretic/rusttmp; cd rusttmp; git checkout rls-876
  • Open project rusttmp in editor with rls
    • rls correctly reports a single diagnostic warning in main.rs for unused import: std::u64...
  • Open lib.rs and change line 1 so the function returns a u64, ie change to pub fn gimme_int() -> u64 {
    • rls shows 1 error inside rusttmp_member but no diagnostics from main.rs.
  • Fix lib.rs test so rusttmp_member compiles.
    • rls shows no errors, but cargo check shows us the main.rs diagnostics
      $ cargo check
      warning: unused import: `std::u64`
       --> src/main.rs:3:5
        |
      3 | use std::u64;
        |     ^^^^^^^^
        |
        = note: #[warn(unused_imports)] on by default
      
      error[E0308]: mismatched types
       --> src/main.rs:6:20
        |
      6 |     let int: u32 = rusttmp_member::gimme_int();
        |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u32, found u64
      

@nrc nrc added this to the 1.0 milestone Jun 21, 2018
@nrc nrc added the P-high label Jul 10, 2018
@Xanewok
Copy link
Member

Xanewok commented Jul 12, 2018

Looked at this today using the STR above:
It seems that after making the u32 -> u64 change, after the initial build we prepare_work, during which the incorrect work WorkStatus::NeedsCargo(Package("rusttmp_member")) is returned - in this case the bin package depends on the member lib, so we'd like to also rebuild the rev deps.

This is caused by package_arg_changed being evaluated to true - this happens, because in compute_package_arg (definition) we eagerly return work only for single package, if dirty file set corresponds only to single package (here), but doesn't look like it's correct, because we ignore rev deps (yikes!).

@nrc this was introduced in 8c77584 to detect edited innermost crate and minimize rebuilding.
Would detecting the innermost crate and using only cached dep graph revdeps solve the problem? The already existing mechanism of collecting minimal set of dirty packages (analyzing the cached build plan) didn't work good enough, because it didn't also analyze tests/benches inter-crate dependencies in this case?

@nrc
Copy link
Member

nrc commented Jul 17, 2018

Would detecting the innermost crate and using only cached dep graph revdeps solve the problem?

It sounds like it should

The already existing mechanism of collecting minimal set of dirty packages (analyzing the cached build plan) didn't work good enough, because it didn't also analyze tests/benches inter-crate dependencies in this case?

I'm not sure what you're saying or asking here? Do you think the above plan won't work? Or that it will work better than the previous attempt?

@jnicholls
Copy link
Author

Thanks for continuing work on this all. Just confirming here that I am still seeing the same behavior as of tonight.

@Xanewok
Copy link
Member

Xanewok commented Jul 29, 2018

Working on it atm.
Got an initial version at https://github.com/Xanewok/rls/tree/issue-876, however I'm mostly ironing out some rough edges now.

bors added a commit to rust-lang/cargo that referenced this issue Jul 29, 2018
Add CompileMode to Executor callbacks

This came up when trying to fix rust-lang/rls#876.

So currently in the RLS we recreate our own dep graph, where we store units with a key `(PackageId, TargetKind)`. This turned out to be not enough since
a) we can have multiple bin target kinds with different names (unrelated to this PR)
b) same package target kind (bin, lib) can be compiled regularly or including the test harness.
With this, we can distinguish these cases and properly rerun both regular compilation check and the one including unit tests.

Without this information we'd need to fall back on guessing whether the rustc invocation has `--test` but having this information makes it accurate and seems useful enough to add it to the callback arguments.

r? @alexcrichton or @matklad
@Xanewok
Copy link
Member

Xanewok commented Aug 1, 2018

While the fix is merged and it seems to be working now (added a regression test), I'd be really glad if folks could try and test it (by using built-from-source RLS) to see if some cases might have slipped through.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
4 participants